Editor: Extract snackbars into a separate component #33355
Conversation
|
Size Change: +128 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
| import { useSelect, useDispatch } from '@wordpress/data'; | ||
| import { store as noticesStore } from '@wordpress/notices'; | ||
|
|
||
| export default function EditorSnackbars() { |
There was a problem hiding this comment.
Do you think it should be EditorSnackbarNotices (I'm not great with naming, just asking :) )
There was a problem hiding this comment.
I'm not great with naming, just asking :)
Same here 😄
Since "Snackbars" or "Toasts" are notice types themself, I don't think there's a need for the Notices suffix IMO.
|
@gziolo Any thoughts on a11y and order here? |
| </div> | ||
| ) | ||
| <> | ||
| <EditorSnackbars /> |
There was a problem hiding this comment.
I think this breaks the UI a bit in "reduced interface mode" (and a couple other modes too). Basically the "footer" is not considered empty any more and an "empty" white area is shown.
There was a problem hiding this comment.
Maybe we should move it to the actions section?
There was a problem hiding this comment.
I'm not sure about the implications, it could be worth a try 🤷
There was a problem hiding this comment.
Done. Works as expected actions area also has z-index, so stacking context is good for us. And If I understand correctly other elements rendered here are also popovers.
|
This is probably unrelated but I noticed that in Firefox, running
in the console triggers JS errors. |
|
This seems to work but it's highlighting something else. When you click a notice to dismiss it, it shows the hidden "open publish panel" button which is in the "actions" area and historically is something required for a11y to allow scree reader users to open that panel using the keyboard. Two things here:
|
|
I think that we should add new It also won't require extracting |
|
@Mamaduka works for me 👍 |
We have a similar button for the block/documents/settings sidebar. I don't know how helpful they are in practice as I'm not using tabbing so often when composing posts. I don't remember exactly now, but those buttons might also exist to ensure that landmark navigation works correctly. You can test landmark navigation it with Screen.Recording.2021-07-14.at.10.19.53.movIt doesn't look like snackbars have their own region/landmark in the editor. It also seems like landmark navigation works in Chrome even when the region is empty as long as it is always rendered - the wrapping HTML element with a role. |
I didn't know about this; thanks for sharing, @gziolo. I found an alternative/simple fix for this issue - #33415. But I think we can introduce the "notices" landmark in the future. This will need some CSS adjustments, so notifications are correctly stacked above the content. |
| @@ -242,6 +243,7 @@ function Editor( { initialSettings } ) { | |||
| } | |||
| actions={ | |||
| <> | |||
| <EditorSnackbars /> | |||
There was a problem hiding this comment.
One thing that you should test is whether it is possible to tab to the snackbar after you complete the action like publishing. In general, it's a challenge because the order of HTML elements matters here. I don't know how often we create snackbars as of today and how many of them have actionable items like links or inner buttons. Ideally, it's only a few tabs away when you want to use them.
There was a problem hiding this comment.
I tried but not able to tab to the snackbar in Gutenberg 11.0.
|
I came across a different issue while working on the new "notices" landmark region. The I think this is doable with CSS Grid, but that's probably a task for another PR. I think that maybe we should go with the original idea + "notices" landmark. Then, we can place Snackbars in the new landmark region and move other notices in the future. Mockup |
|
Just snackbars in the notices area works for me. I'd have loved if we could move the entire component for consistency but we can try it later and just fix the pressing 5.8 issue for now. |
Me too, but I'm afraid I will break something if I hack around the flex-direction Snackbars in the new "notices" landmark region are ready for testing. |
| { !! notices && ( | ||
| <div | ||
| className="interface-interface-skeleton__notices" | ||
| role="region" |
There was a problem hiding this comment.
I wonder if it should be labelled region, because right now, when you use "region navigations" (ctrl + @ on my Mac but I know it can be different in different keyboard layouts), you'll see an "empty region" stop. It's not the only one though, it seems there's another empty region somewhere.
Maybe we can start without a region role label and tabIndex
There was a problem hiding this comment.
We can "promote" it to a full region once all notifications are rendered here.
There was a problem hiding this comment.
Should I remove the aria-label as well?
There was a problem hiding this comment.
yep it's useless if it's not a region and no tabIndex
There was a problem hiding this comment.
Updated. I think the new tab region also caused the e2e test failure. Hopefully, that's resolved now.
|
btw, I see the previews now (I had to rebuild) |
I was thinking the same and will create a follow-up issue 👍 |
Editor snackbars are no rendered in the InterfaceSkeleton footer to avoid z-index context issues.
db17241 to
ebd0f5b
Compare
|
@alexstine asked me to review this Pull Request for him, since he was tagged, but he wasn't able to figure out the topic of discussion. |
|
Thanks for chiming in, the final solution we landed in doesn't include a region as while trying the region navigation, it was not clear what region was highlighted, we're going to continue exploring this in a follow-up. |
* Add notices container to InterfaceSkeleton. * Move the `EditorSnackbars` component to the notices container.
* Add notices container to InterfaceSkeleton. * Move the `EditorSnackbars` component to the notices container.
* Add notices container to InterfaceSkeleton. * Move the `EditorSnackbars` component to the notices container.
Description
Extracts snackbar notices list into a new component -
EditorSnackbars,which is rendered in theInterfaceSkeletonfooter to avoid z-index context issues with Block Inserter.Fixes #33114.
How has this been tested?
Screenshots
Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).