-
Notifications
You must be signed in to change notification settings - Fork 386
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
fix(synthetic-shadow): conservatively disable focus navigation routine upon focus #2077
Conversation
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarks@lwc/engine-dom
@lwc/engine-server
|
.shadowRoot.querySelector('integration-child') | ||
.shadowRoot.querySelector('button'); | ||
}); | ||
button.click(); // click into input |
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.
clicks the input or the button?
static delegatesFocus = true; | ||
|
||
handleFocus(event) { | ||
event.target.focus(); |
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.
does the button (event.target
) already have the focus before calling .focus()
?
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.
Yeah
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.
what confuses me is that the button will get the focus even if we don't execute this line. but i also guess that executing this line without the fix, will make the test fail.
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.
Yeah, it doesn’t make sense but we should be able to handle that!
🥳 Performance ImprovementBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarks@lwc/engine-dom
@lwc/engine-server
|
Details
In a nutshell, this is how we disable keyboard focus navigation when the user clicks on an element:
mousedown
:letBrowserHandleFocus
is set totrue
focusin
: short circuit the keyboard focus navigation routine ifletBrowserHandleFocus
is set totrue
mouseup
:letBrowserHandleFocus
is set tofalse
The problem here is that, during a
click
, allfocus
events are processed beforefocusin
events, so if there is a focus handler that runs between themousedown
andfocusin
events above, and it invokes thefocus()
method on an element (which is a very strange behavior), theletBrowserHandleFocus
flag is incorrectly set tofalse
before ourfocusin
handler runs, causing the "next" element to be incorrectly focused.Does this PR introduce breaking changes?
No, it does not introduce breaking changes.
GUS work item
N/A yet