-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Restore Inserter menu focus by tracking opening node #786
Conversation
@@ -19,7 +19,6 @@ function Button( { href, isPrimary, isLarge, isToggled, className, buttonRef, .. | |||
...tagProps, | |||
...additionalProps, | |||
className: classes, | |||
ref: buttonRef, |
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.
I like this change. The buttonRef
prop was a bit weird.
editor/inserter/menu.js
Outdated
@@ -56,7 +56,7 @@ class InserterMenu extends wp.element.Component { | |||
selectBlock( slug ) { | |||
return () => { | |||
this.props.onInsertBlock( slug ); | |||
this.props.onSelect(); | |||
this.props.closeMenu(); |
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.
I think the cloneMenu
prop should receive a nullable selected block (or slug) as a parameter (even if it's not used right now).
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.
I think the
cloneMenu
prop should receive a nullable selected block (or slug) as a parameter (even if it's not used right now).
So consolidating to onSelect( slug: ?string )
in place of the onInsertBlock
and closeMenu
combo?
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, or onClose( slug: ?string )
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.
👍 This is cleaner for now. If we ever want to open the inserter via keyboard shortcut, we will need to revisit this, but for our current needs, this is a better solution.
Sure. The main objective with these changes it to push for components to be self-sufficient and not depend on particular DOM structures of other components. To this point, with keyboard shortcuts, it seems sensible that we'd expose this in a way that the component can define its intent to respond to global keyboard events (perhaps through a higher-order component), rather than have the keyboard event handler call on the inserter(s) directly. |
Avoids need to pass ref callback through to button
Not on click outside
7f92bbb
to
55829f1
Compare
In the rebased c98a7bb, I consolidated further to a single I also found that shifting focus has a tricky gotcha which is to be exempt from click outside behavior; clicking a blank spot on the page shouldn't be forcing the button to be focused. This is accounted for in 55829f1 to only react to the escape key press (or, more accurately, the menu closing without a selected slug). |
Related: #578
This pull request seeks to revise the Inserter Menu close-on-escape behavior to focus the original opening element by tracking the node when the opening toggle event occurs. A goal with this change is to eliminate ancestor access to a button's rendered node through its
buttonRef
prop (removed by the changes here).Implementation notes:
event.currentTarget
could be interchangeable withdocument.activeElement
, but I chose the former since it feels more reliable to the knowledge that the current target is the rendered button we intend to refocus. The downside is thattoggle
must be used as an event callback (the only current usage).Testing instructions:
Repeat testing instructions from #578.