Fix Navigation accessibility issues #36292
Conversation
tellthemachines
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
tellthemachines
changed the title
tellthemachines
added
[Block] Navigation
|
Size Change: +302 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
|
Nice, very fast work. I see this generally working as intended. One small issue for this PR. Beause the modal overlay menu is always vertical (at least until a future where its contents can be edited in isolation), it should always be top-aligned. And so when the flex-direction changes from the horizontal navigation menu to the vertical navigation menu, so does the workings of align-items and justify-content. At the moment when the navigation is justified right (or presumably anything other than left), the overlay menu is bottom aligned:
By the way, the CSS variables here we can probably leverage to fix the issue with menu opening direction — i.e. opening the menu down and left when the navigation is justified right. Does this make sense? |
No, it did not make sense. My idea was to use use one of the new justification CSS variables to set the Outside of restoring the |
| display: flex; | ||
| flex-wrap: var(--layout-wrap, wrap); | ||
| flex-direction: var(--layout-direction, initial); | ||
| justify-content: var(--layout-justify, initial); |
There was a problem hiding this comment.
In a quick test, the following seems to have fixed it for me:
| justify-content: var(--layout-justify, initial); | |
| justify-content: flex-start; |
Can you think of any reason this shouldn't work?
There was a problem hiding this comment.
This fixes left, center, right:
Space between gets weird, I'll look at what I can do there:
|
Alright I took the liberty of committing a better fix than the one I suggested in #36292 (comment). It works for me, for both vertical and horizontal menus: |
|
I'm looking at some issues in a separate PR, and realized that when a text label is long enough to wrap, the
So I moved the text-alignment rule to the submenu block directly. |
|
Thanks for the review and improvements @jasmussen !
We could do something like this, but we'd need to set a new custom property conditionally if layout is (or isn't) right aligned. Something like: I'm not sure about this approach because it adds logic to the layout calculations that is super specific to the nav use case. The other custom properties can potentially be useful for any block with complex markup, but submenu alignment is unlikely to occur elsewhere. How likely is it that we'd need to hook arbitrary styles to a specific alignment in other blocks? The JS approach you hint at in #36241 would be the best if we can get it right (which won't be easy, based on all the issues we've had with the Popover component 😅 ) but perhaps in the meantime the most harmless patch would be to add a bit of custom logic to the Navigation edit that checks the layout value and adds a
With flex we'd still need a classname or something to tell us the direction though, wouldn't we? I'm assuming you were thinking of something like |
Yep, the idea of a invalid custom property with a number fallback was out there. I just wanted to share since I mentioned having an idea, and every once in a while it inspires better ideas. I do agree that we probably need the justification classes in the interim. One thought, though: if we are starting to think about #36241 as a longer term solution, it seems like that approach would also need classes, right? Something like a programmatically added class to the submenu container a la
Yes, I wasn't able to remove the use of position-absolute, and unless we specifically set the In any case, this is a good first step, thank you again. |
Not necessarily. If it works like the Popover component, it'll read the position of the parent element in the viewport and then position itself accordingly. All the positioning is done with JS, so the classes aren't needed. |
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
* Fix display contents accessibility issue in Navigation * Remove aria-hidden from navigation wrapper. * Fix content justification in overlay nav. * Fix for alignment inside overlay menu. * Move text align rule. * Revert "Fix content justification in overlay nav." This reverts commit 5170bb5bb3b9ead314a1ecec68768f2fe2511804. * Fix php lint errors. Co-authored-by: jasmussen <[email protected]>
* Fix display contents accessibility issue in Navigation * Remove aria-hidden from navigation wrapper. * Fix content justification in overlay nav. * Fix for alignment inside overlay menu. * Move text align rule. * Revert "Fix content justification in overlay nav." This reverts commit 5170bb5bb3b9ead314a1ecec68768f2fe2511804. * Fix php lint errors. Co-authored-by: jasmussen <[email protected]>
|
Cherry picked into the Gutenberg 11.9 release in: 0ee7f3b |
Sounds good. I was thinking classes because the properties to set could, maybe beneficially, be inherited. Imagine this deliberately insane menu:
6 gray boxes, 6 submenu containers, pseudo code:
And so on. The idea being that we can avoid some potentially gnarly inline px values that need to be updated onresize with some relatively simple inheritance values. It might be overthinking it, it might be better as you suggest, but I thought I'd mention it as a potentially basic way to only assign classes based on whether the screen edge is about to be hit. Edit: Oh and thanks for landing this one! 👏 |
| @@ -103,14 +103,26 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support | |||
| */ | |||
| if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) { | |||
| $style .= "justify-content: {$justify_content_options[ $layout['justifyContent'] ]};"; | |||
| // --justification-setting allows children to inherit the value regardless or row or column direction. | |||
| $style .= "--justification-setting: {$justify_content_options[ $layout['justifyContent'] ]};"; | |||
| if ( ! empty( $layout['setCascadingProperties'] ) && $layout['setCascadingProperties'] ) { | |||
There was a problem hiding this comment.
My first reaction here is that I don't like this to be honest, it breaks the "layout" abstraction to solve a specific use-case.
There was a problem hiding this comment.
It feels to me like we're tweaking the layout abstraction here to suit the navigation block while we should be thinking about abstraction in a more generic way. Maybe this indicates that the navigation block shouldn't be using the default "flex layout" or implementing its own custom layout?
Or maybe it indicates that we need iterations on the layout like this PR tries to do but I'm not understanding what this is trying to do at the abstraction level.
I guess what I'm trying to say is that when applying a layout to a block, that block shouldn't need to define styles that override the structural styles, these styles are provided by the layout, meaning there's no need for awareness of these CSS variables inside the block or for its children ideally.
|
Can we do something about this ? I'm really concerned about leaking this into 5.9. Also, there's a related notice maybe on trunk (just opening an simple post on the frontend)
|
|
Actually, the notice is related to another layout change in #36681 |
|
#37113 is a potential fix for the issue mentioned by @youknowriad. |
Migrating Navigation block to flex layout was flagged as a high priority item in the Navigation tracking issue: #35521 Unless we come up with a better way of making layout work on blocks with complex HTML, the only alternative I see is to roll back layout for the Navigation block altogether.
Layout as it was initially didn't work for the Navigation block because it has multiple containers inside it that each need to have flex styles applied to them, and those styles are required to change when justification or orientation is changed. This was the best solution I could find to the problem of using layout in blocks with complex markup structures. We may not have any other core blocks with this problem (I can't think of any offhand) but there are likely to be 3rd party blocks that will benefit from this. It's generic enough that any container block that is more than a single wrapper around its inner blocks can leverage it, and having (If layout worked with classnames, as the previous justification controls did, this solution wouldn't be necessary. It's essentially a workaround for the shortcomings of auto-generated styles, allowing us to leverage the cascade through custom properties instead of classnames.) Having said that, layout may not be the best solution for Navigation. We've had to add back a couple of the legacy classnames due to issues with submenu layout and child blocks inheriting orientation: #36340. We've also had feedback from extenders that the classnames were useful for other purposes: #36525 The major upside of using layout is being able to set both orientation and justification as layout properties, where before orientation was a block variation. Layout makes it easier to try out different settings, with the controls all in the same place. I can't see any other way of achieving that short of introducing yet another custom control, which should be avoided.
This isn't an override, more an extension. The styles defined by layout aren't being replaced, but in order for the layout to be applied correctly to the block markup, they are being provided as custom properties so the inner wrappers of the block can use them. I see the intention of not wanting the block's internals or its children to be aware of the layout styles, but that will only work for blocks that are composed of a single HTML element wrapping their inner blocks. That may be the case for most core blocks, because we keep core blocks pared down to the essentials, but for any slightly more elaborate markup layout will fail. (Say for whatever reason the block has a header, and the layout styles are only meant to apply to its inner content area. Or that it has two content areas, and layout needs to apply to both their inner contents. In any of these scenarios, we need the flex styles to cascade.) When (if) we have better accessibility support for It's not clear to me what the best solution is. Any feedback appreciated! |
|
Thanks for the explantation, It's a hard problem indeed and I don't see a perfect solution.
The layout properties used here are The other concern for me is that this is an adhoc fix. Why limit the generation of these variables to only the "flex" layout and only these specific variables, why not everything. From a block and API point of view, the logic is not clear. Basically, we just exposed what we needed to fix the navigation issue. The last concern for me is that exposing APIs should be done very carefully.
I'm not sure I agree with this, I think it's something that works for most blocks, I think elaborate blocks should probably rely on inner blocks.. That said, I agree that it's just guesses at the moment and that's why we need to be careful with exposing APIs at the moment. For me, if we're not able to provide an good abstraction that the block author shouldn't have to know about internals (implementation details), we shouldn't build this abstraction entirely as it's just makes creating blocks harder.
Do we have any idea about the current support? Can we consider this as an incentive or progressive enhancement? Should we rollback the layout support for now in the navigation block? |
We have to generate both
Apart from We could hypothetically remove
This isn't needed for the "flow" layout, because there are no justification/orientation options, so it only makes sense to generate them for "flex". If we add a "grid" layout at some point, it might be useful to have these too, at least until subgrid gains wider support. The specific variables are all the ones needed to inherit the flex layout properties at a child element level; unless I'm missing something there are no other values provided by that layout type.
I'd love to have more info on this too. From looking at block plugins out in the wild, there are all sorts of custom layout alignment/orientation options, so it seems there's a need for a solid core API. But I'm worried that layout without the custom properties, or some other solution that allows for styles to cascade, is too inflexible to be of use except in the most basic cases.
I think block authors will always have to know something about the implementation. For instance, if layout styles are only applied to the block wrapper and don't cascade, the block author needs to know this. Layout will never just magically work on a block regardless of its markup; it assumes a very specific markup structure. Likewise, authors need to know that the styles are generated, and no classnames are provided that they can hook custom styles to. Providing a bunch of ready-made controls that output some CSS or some classnames is already a huge help to block authors, who would otherwise have to build their own controls. But I think some flexibility that allows space for custom styles and markup will be needed if we want this API to be adopted by extenders.
Safari and iOS Safari are our blockers here. Sadly there's no sign of the bug being fixed just yet, and because Safari is the browser most used with VoiceOver we can't use
I'm super reluctant to do this, because it would mean reverting to the horizontal/vertical block variations, which is a much worse experience in my opinion. If we really don't want the custom properties in layout, we could try generating classnames inside the Nav block as I suggested above. The downside is it's another API that we're putting out there, and #36525 suggests theme authors will leverage these classnames, so it'll be hard to remove them down the line. But at least the classnames would be specific to the Nav block, unlike the custom properties API. |
I think that's the wrong way to look at the layout abstraction, "flex" is an implementation detail of the "Flex layout" even if they share the name. We're not abstracting the DOM positioning tools but more UI layouts people want to achieve. http://every-layout.dev is a good inspiration and mental modal for this.
I don't like when we make API decisions for specific use-cases. Basically you're saying that no block will ever need the "widths" for the flow layout elsewhere. While I think that's the ideal situation, I can say the same for flex. Ideally, flex layouts blocks shouldn't need to know about the layout properties directly. If we say that it's needed for flex layout, I don't see why it should be limited to only a handful of specific properties and not be done consistently for all properties of all layouts.
If you really think that for your block, I'd say don't use the layout abstraction and if it proves to be the case for all blocks, we should remove the abstraction because it becomes just a burden and we just add specific block supports like other block supports. But what I'm getting here is that you seem to want the "UI" of the layout but without its style (or very few styles), I think maybe the best compromise we can make here (which would also align with other block supports) is to add the |
|
I've been thinking more here, if you still want the "style generation" from the layout attribute, we can still keep the added code from this PR but move it to a filter (similar to the layout filter itself) that is specific to the navigation block that way we don't impact the abstraction yet until we know more about the use-cases for other blocks... |
Yes, we still need the exact styles generated by layout, we just need to be able to apply them to a different wrapper than they are by default. I'm not sure I follow the solution though. Would this be another filter applied to |
yes we need a custom filter that is specific to the navigation block in both editor and frontend (layout filters are also duplicated in both editor and frontend) |
|
@youknowriad check out #37438; will that work? |
Description
Fixes issue mentioned in This comment. When
display: contentsis used on an element, all its semantics are lost, so screen readers won't read out the content correctly.I removed all instances of
display: contentsand instead added an optionalsetCascadingPropertiesflag to the layout config which, when enabled, outputs the flex config as custom properties, so they can be used by descendant elements.In the process of fixing this, I also noticed there is an
aria-hiddenonwp-block-navigation__responsive-containerthat's hiding the whole nav contents from screen readers. I removed it altogether, because nav content is already hidden from screen readers withdisplay: nonewhen responsive menu is enabled and in closed mode.How has this been tested?
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).