Fix region navigation in the site editor #46525
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
youknowriad
added
[Type] Bug
|
Size Change: -331 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
Mamaduka
approved these changes
Dec 14, 2022
There was a problem hiding this comment.
The Site Editor changes test well for me. I also tested the post editor and couldn't spot any regressions.
It would be nice to get feedback from folks with more experience in a11y. cc @afercia, @alexstine
P.S. I noticed that when the "Editor top bar" region is focused in the site editor, it has a black background.
Screenshot
|
Thanks ❤️ I'll try to have a look as soon as possible. I'm afraid it won't happen before Monday though. |
|
@Mamaduka The issue was that the trick used to draw the borders in region navigation relied on using z-index: -1 in a "stacker" div. which means the background of the parent element would take over and hide the inner element. In the last commit I simplify the region navigation styles by removing that stacker div (like it used to be I think) and using a pseudo element to add the borders. |
|
@youknowriad, the changes introduced by this PR #45369, right? It might be a good idea to re-test for regressions. I can do that later today. |
|
Yes, in my tests the borders are always clear and visible but maybe I'm missing something :) |
youknowriad
force-pushed
the
fix/region-navigation
branch
from
699a175 to
ac2274e
Compare
There was a problem hiding this comment.
The landmark for the inspector on the site editor looks a little strange:
The landmark for the browse mode sidebar also appears cut:
Both these issues are probably something on my test instance.
If we are with the save/publish panel open on the site editor navigating the regions Ctrl + backtick closes the save panel. (already an issue in the trunk can be checked later).
On browse mode, I can only see a landmark for the browse mode sidebar. Should we also have one for the content preview?
|
@jorgefilipecosta the z-index file was broken, I think that's why you didn't have the right styles. |
That was it and now I see the two landmarks on browse mode. |
jorgefilipecosta
approved these changes
Dec 15, 2022
|
Thank you, Riad! The region stacker div removal looks great 👍 |
|
Thanks for working on this. Sorry I didn't have time to test it earlier. Unfortunately, both the functionality and the focus style seem to not work well. Most importantly:
As mentioned in #45369 we can't use a CSS pseudo element. because the pseudo element scrolls together wit the content. To reproduce:
Note that, potentially, this will happen on any navigable region that is scrollable. Noting that the CSS pseudo element approach was already proposed in #21717 and not recommended for the same reason. Also noting that long time ago the focus style was actually using a CSS pseudo element and we changed it because of the scrolling issue, see #8554 Therefore, we need to set the focus style on an actual HTML element and this element must meet two conditions. As noted in #45369
For these reasons, I'd still recommend the approach based on the 'stacker div'. It does solve the x-index issue in a reliable way. It doesn't use a CSS pseudo element. It makes things more reliable. I wouldn't recommend a CSS-only approach because any change to the CSS could break the focus style. Keeping the 'stacker div' makes the There are a few other things that don't work (most notably the navigate region doesn't work in the Site editor browse mode). I'll try to detail in a new issue. |
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.
closes #46509
Follow up to #44770
What?
When we introduced the browse mode layout, we inadvertently broke the aria regions navigation. The problem is that the
InterfaceSkeletoncomponent is not the top level component anymore.How?
I think now the site editor shouldn't be using that component at all because it departed from its intended original design, that said, and since that component still handles the positioning of several elements for us (inserters, sidebar...), I'm keeping it for now but I'm moving the navigateRegions hook to the higher-level layout which should bring us back to how it used to work before the browse PR.
Notes:
Testing Instructions
ctrl + option + nto navigate between the different aria regions.