-
Notifications
You must be signed in to change notification settings - Fork 153
feat: restrict user to enter space in signup component input field #1107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f5c0c8f
f5934d0
8745e4c
44e56a3
2fc4a32
afd4908
218123e
b1615b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
azeemuddinaziz marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,31 @@ export default class SignupComponent extends Component { | |
| return LABEL_TEXT[currentStep]; | ||
| } | ||
|
|
||
| @action inputFieldChanged({ target: { value } }) { | ||
| @action inputFieldChanged(event) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do see issue When you remove whitespace and calculate the new cursor position, you're using: But nonWhitespaceBeforeCursor is the count of non-whitespace characters, not accounting for the actual position in the sanitized string. This can cause the cursor to jump incorrectly. textBeforeCursor = "abc d" The real issue is that you're setting the value and cursor position synchronously, but the value update might not be immediate, causing cursor position issues.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const { onChange, currentStep } = this.args; | ||
| onChange(currentStep, value); | ||
|
|
||
| const rawValue = event.target.value; | ||
|
|
||
| if (/\s/.test(rawValue)) { | ||
| const cursorPosition = event.target.selectionStart; | ||
| const sanitizedInput = rawValue.replace(/\s/g, ''); | ||
|
|
||
| const textBeforeCursor = rawValue.substring(0, cursorPosition); | ||
| const spacesBeforeCursor = (textBeforeCursor.match(/\s/g) || []).length; | ||
| const newCursorPosition = cursorPosition - spacesBeforeCursor; | ||
|
|
||
| event.target.value = sanitizedInput; | ||
| event.target.setSelectionRange(newCursorPosition, newCursorPosition); | ||
|
|
||
| onChange(currentStep, sanitizedInput); | ||
| } else { | ||
| onChange(currentStep, rawValue); | ||
| } | ||
| } | ||
|
|
||
| @action handleKeydown(event) { | ||
| if (/\s/.test(event.key)) { | ||
| event.preventDefault(); | ||
| } | ||
| } | ||
| } | ||
Suvidh-kaushik marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add sanitization during the signup form submit for first and last name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to add sanitization on submit because it is guaranteed that no spaces will be present at submission time (already sanitized on input change).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding sanitization to the from button will be redundant. The user who bypasses the
onChangeinput sanitization by changing the value via browser dev tools will still ultimately receive an HTTP 400 Bad Request error response from the backend.No, there will be no changes in the backend API. It is already handling the input validation by using
validateGenerateUsernameQuerymiddleware.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@azeemuddinaziz can you please reply to these question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MayankBansal12 and @Suvidh-kaushik great observation, let me drill down this
In Ember (especially with tracked state + one-way data flow), direct DOM mutation can desync the input value from the tracked property backing that input.
What can go wrong
• Cursor jumps to the end while typing
• Value reverts on re-render
• Input appears to accept spaces briefly and then snaps back
• Fails when used with @value={{this.someTrackedProp}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamitprakash Thanks for detailed explanation.
keydownevent thatpreventDefaultsspaces. This completely stops the cursor jumping and desync issues since the space never hits the DOM.I have added the test cases for the keydown implementation.
- I have updated the test coverage in PR description as well.