Focus inserter toggle when closing the inserter sidebar #61467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jeryj
added
[Feature] Inserter
scruffian
reviewed
May 8, 2024
| return ( | ||
| <div className="editor-inserter-sidebar"> | ||
| // eslint-disable-next-line jsx-a11y/no-static-element-interactions |
There was a problem hiding this comment.
We should address this in a future PR.
There was a problem hiding this comment.
The fix would be moving this interaction up to where the role for the region on the wrapper already is.
There was a problem hiding this comment.
moving this interaction up
Why not do it here? Is it complex?
There was a problem hiding this comment.
I think it'll be better to do it when refactoring the list view and inserter to share the same sidebar.
|
Size Change: +30 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
scruffian
approved these changes
May 8, 2024
mikachan
approved these changes
May 8, 2024
afercia
added
[Focus] Accessibility (a11y)
This was referenced May 8, 2024
scruffian
force-pushed
the
fix/inserter-sidebar-close-focus
branch
from
ddaea17
to
3425cd7
Compare
ntsekouras
reviewed
May 9, 2024
ntsekouras
reviewed
May 9, 2024
|
Could someone please link me to the discussion where the block inserter and list view will share the same sidebar? I want the background info on why that change is being considered. It will create a jumbled mess for screen reader users unless we do this right and really consider how we're going to drive separation. |
kevin940726
reviewed
May 9, 2024
| @@ -75,6 +75,9 @@ export const getInsertionPoint = createRegistrySelector( ( select ) => | |||
| export function getListViewToggleRef( state ) { | |||
| return state.listViewToggleRef; | |||
| } | |||
| export function getInserterSidebarToggleRef( state ) { | |||
| return state.inserterSidebarToggleRef; | |||
There was a problem hiding this comment.
I know this is an existing pattern, but I just want to share my concerns about this approach again. I still think storing non-serializable data in the store is a bad practice.
First of all, it breaks the Redux Devtools; I think I have had to disable the extension ever since we merged this approach the first time. Even if we special case this by making it serializable in the extension settings, we can't make sure that new features of the extensions will continue support this pattern.
Second, I think using a context provider is a much simpler and intuitive solution. It might mean an extra wrapper and private settings in the editor component, but it also matches more closely to what React expects. We can also simply group the same patterns over and over again and create an abstraction around it, so it feels less "hacky."
This is not a blocker and I just want to re-share this for visibility 😆.
| * @param {Object} state | ||
| * @return {Object} Reference to the inserter sidebar toggle button. | ||
| */ | ||
| export function inserterSidebarToggleRef( state = { current: null } ) { |
There was a problem hiding this comment.
Not sure if it matters, but every time this function is called, the state will be a different instance of ref. Probably better this way:
Suggested change
| export function inserterSidebarToggleRef( state = { current: null } ) { | |
| const INITIAL_REF = createRef( null ); | |
| export function inserterSidebarToggleRef( state = INITIAL_REF ) { |
|
@alexstine - I'm not sure where discussion was. The idea is to refactor them to share the same wrapping component instead of three or four that are almost identical but using different components with repeated work. The behavior of List View should not be changed. |
|
@jeryj So is this combining code or are we going to have all these components imported into one sidebar? Combining code makes sense, combining UI does not. |
I'd agree with @alexstine as I'd like to have more background info and context about this change. I'm not sure pushing important changes without an open, broad, discussion is any ideal in a collaborative open source project. Alas, this is only one of the multitude of changes that are just pushed too fast and, in my opinion, too lightly in this project. That said, I do realize this PR is not the right place for this kind of discussion. Anyways, I think there's the need for a follow-up to address a couple long standing issues:
|
jeryj
force-pushed
the
fix/inserter-sidebar-close-focus
branch
from
5237640
to
95e9bf8
Compare
|
This PR doesn't do anything to combine the sidebar code, it's fixing an accessibility issue with focus management using the same method that the list view uses. I think they would be easier to maintain if they shared the same code. I agree it needs to be discussed before doing it. Here's an issue to discuss it: #61549 |
|
Thanks @jeryj . Just wanted to make sure it didn't get lost in the shuffle. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Brings the inserter sidebar closing behavior inline with List View. The inserter sidebar can be closed with escape or the visible close button. Doing this always focuses the inserter toggle button.
Why?
Consistency in the editor.
How?
Uses the same method as the List View sidebar.
Testing Instructions