Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Navigation accessibility issues #36292

Merged
merged 7 commits into from
Nov 10, 2021
Merged

Fix Navigation accessibility issues #36292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
67dd98b
Fix display contents accessibility issue in Navigation
tellthemachines Nov 8, 2021
c7f450d
Remove aria-hidden from navigation wrapper.
tellthemachines Nov 8, 2021
868f635
Fix content justification in overlay nav.
tellthemachines Nov 9, 2021
cffca14
Fix for alignment inside overlay menu.
jasmussen Nov 8, 2021
a141ce8
Move text align rule.
jasmussen Nov 8, 2021
b8932da
Revert "Fix content justification in overlay nav."
tellthemachines Nov 9, 2021
cca9163
Fix php lint errors.
tellthemachines Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// --layout-justification-setting allows children to inherit the value regardless or row or column direction.
$style .= "--layout-justification-setting: {$justify_content_options[ $layout['justifyContent'] ]};";
$style .= '--layout-direction: row;';
$style .= "--layout-wrap: $flex_wrap;";
$style .= "--layout-justify: {$justify_content_options[ $layout['justifyContent'] ]};";
$style .= '--layout-align: center;';
}
}
} else {
$style .= 'flex-direction: column;';
if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) {
$style .= "align-items: {$justify_content_options[ $layout['justifyContent'] ]};";
$style .= "--justification-setting: {$justify_content_options[ $layout['justifyContent'] ]};";
if ( ! empty( $layout['setCascadingProperties'] ) && $layout['setCascadingProperties'] ) {
// --layout-justification-setting allows children to inherit the value regardless or row or column direction.
$style .= "--layout-justification-setting: {$justify_content_options[ $layout['justifyContent'] ]};";
$style .= '--layout-direction: column;';
$style .= '--layout-justify: initial;';
$style .= "--layout-align: {$justify_content_options[ $layout['justifyContent'] ]};";
}
}
}
$style .= '}';
Expand Down
32 changes: 25 additions & 7 deletions packages/block-editor/src/layouts/flex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,10 @@ export default {
);
},
save: function FlexLayoutStyle( { selector, layout } ) {
const { orientation = 'horizontal' } = layout;
const {
orientation = 'horizontal',
setCascadingProperties = false,
} = layout;
const blockGapSupport = useSetting( 'spacing.blockGap' );
const hasBlockGapStylesSupport = blockGapSupport !== null;
const justifyContent =
Expand All @@ -94,21 +97,36 @@ export default {
const flexWrap = flexWrapOptions.includes( layout.flexWrap )
? layout.flexWrap
: 'wrap';
// --justification-setting allows children to inherit the value
// regardless or row or column direction.
const rowOrientation = `
let rowOrientation = `
flex-direction: row;
align-items: center;
justify-content: ${ justifyContent };
--justification-setting: ${ justifyContent };
`;
if ( setCascadingProperties ) {
// --layout-justification-setting allows children to inherit the value
// regardless or row or column direction.
rowOrientation += `
--layout-justification-setting: ${ justifyContent };
--layout-direction: row;
--layout-wrap: ${ flexWrap };
--layout-justify: ${ justifyContent };
--layout-align: center;
`;
}
const alignItems =
alignItemsMap[ layout.justifyContent ] || alignItemsMap.left;
const columnOrientation = `
let columnOrientation = `
flex-direction: column;
align-items: ${ alignItems };
--justification-setting: ${ alignItems };
`;
if ( setCascadingProperties ) {
columnOrientation += `
--layout-justification-setting: ${ alignItems };
--layout-direction: column;
--layout-justify: initial;
--layout-align: ${ alignItems };
`;
}
return (
<style>{ `
${ appendSelectors( selector ) } {
Expand Down
4 changes: 4 additions & 0 deletions packages/block-library/src/navigation-submenu/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ button.wp-block-navigation-item__content {
color: currentColor;
font-size: inherit;
font-family: inherit;

// Buttons default to center alignment. This becomes visible
// when a menu item label is long enough to wrap.
text-align: left;
}

.wp-block-navigation-submenu__toggle {
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/navigation/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@
"allowSwitching": false,
"allowInheriting": false,
"default": {
"type": "flex"
"type": "flex",
"setCascadingProperties": true
}
}
},
Expand Down
6 changes: 1 addition & 5 deletions packages/block-library/src/navigation/edit/responsive-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,7 @@ export default function ResponsiveWrapper( {
</Button>
) }

<div
className={ responsiveContainerClasses }
id={ modalId }
aria-hidden={ ! isOpen }
>
<div className={ responsiveContainerClasses } id={ modalId }>
<div
className="wp-block-navigation__responsive-close"
tabIndex="-1"
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ function( $block ) {

$responsive_container_markup = sprintf(
'<button aria-expanded="false" aria-haspopup="true" aria-label="%3$s" class="%6$s" data-micromodal-trigger="modal-%1$s"><svg width="24" height="24" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" role="img" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5" /><rect x="4" y="15" width="16" height="1.5" /></svg></button>
<div class="%5$s" id="modal-%1$s" aria-hidden="true">
<div class="%5$s" id="modal-%1$s">
<div class="wp-block-navigation__responsive-close" tabindex="-1" data-micromodal-close>
<div class="wp-block-navigation__responsive-dialog" role="dialog" aria-modal="true" aria-labelledby="modal-%1$s-title" >
<button aria-label="%4$s" data-micromodal-close class="wp-block-navigation__responsive-container-close"><svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" role="img" aria-hidden="true" focusable="false"><path d="M13 11.8l6.1-6.3-1-1-6.1 6.2-6.1-6.2-1 1 6.1 6.3-6.5 6.7 1 1 6.5-6.6 6.5 6.6 1-1z"></path></svg></button>
Expand Down
33 changes: 17 additions & 16 deletions packages/block-library/src/navigation/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,11 @@

// Navigation block inner container.
.wp-block-navigation__container {
display: flex;
flex-wrap: var(--layout-wrap, wrap);
flex-direction: var(--layout-direction, initial);
justify-content: var(--layout-justify, initial);
align-items: var(--layout-align, initial);

// Reset the default list styles
list-style: none;
Expand All @@ -329,7 +334,11 @@
bottom: 0;

.wp-block-navigation__responsive-container-content {
display: contents;
display: flex;
flex-wrap: var(--layout-wrap, wrap);
flex-direction: var(--layout-direction, initial);
justify-content: var(--layout-justify, initial);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a quick test, the following seems to have fixed it for me:

Suggested change
justify-content: var(--layout-justify, initial);
justify-content: flex-start;

Can you think of any reason this shouldn't work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes left, center, right:
Screenshot 2021-11-08 at 08 58 16

Screenshot 2021-11-08 at 08 58 27

Screenshot 2021-11-08 at 08 59 56

Screenshot 2021-11-08 at 08 58 38

Screenshot 2021-11-08 at 09 00 11

Screenshot 2021-11-08 at 08 58 51

Space between gets weird, I'll look at what I can do there:
Screenshot 2021-11-08 at 09 00 38

align-items: var(--layout-align, initial);
}

// Overlay menu.
Expand All @@ -350,8 +359,6 @@
// Add extra top padding so items don't conflict with close button.
padding: 72px 24px 24px 24px;
background-color: inherit;
// Fallback to inheritance in case the --justification-setting variable fails.
align-items: inherit;

.wp-block-navigation__responsive-container-content {
// Override the container flex layout settings
Expand All @@ -360,7 +367,11 @@
display: flex;
flex-direction: column;
// Inherit alignment settings from container.
align-items: var(--justification-setting, inherit);
align-items: var(--layout-justification-setting, inherit);

// Always align the contents of the menu to the top.
justify-content: flex-start;

// Allow menu to scroll.
overflow: auto;
padding: 0;
Expand Down Expand Up @@ -412,7 +423,7 @@
display: flex;
flex-direction: column;
// Inherit alignment settings from container.
align-items: var(--justification-setting, inherit);
align-items: var(--layout-justification-setting, initial);
}
}

Expand All @@ -435,7 +446,7 @@
@include break-small() {
&:not(.hidden-by-default) {
&:not(.is-menu-open) {
display: contents;
display: block;
width: 100%;
position: relative;
z-index: 2;
Expand Down Expand Up @@ -518,13 +529,3 @@ html.has-modal-open {
overflow: hidden;
}

.wp-block-navigation__responsive-close,
.wp-block-navigation__responsive-dialog,
.wp-block-navigation__container {
display: contents;

.is-menu-open & {
// Fallback to inheritance in case the --justification-setting variable fails.
align-items: inherit;
}
}
7 changes: 5 additions & 2 deletions packages/block-library/src/page-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// The Page List block should inherit navigation styles when nested within it
.wp-block-navigation {
.wp-block-page-list {
display: contents;
flex-wrap: wrap;
display: flex;
flex-direction: var(--layout-direction, initial);
justify-content: var(--layout-justify, initial);
align-items: var(--layout-align, initial);
flex-wrap: var(--layout-wrap, wrap);
background-color: inherit;
}

Expand Down