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.

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

Refactor the hooked blocks plugins UI to not use flex-direction row-reverse #63094

Closed
afercia opened this issue Jul 3, 2024 · 4 comments · Fixed by #63133
Closed

Refactor the hooked blocks plugins UI to not use flex-direction row-reverse #63094

afercia opened this issue Jul 3, 2024 · 4 comments · Fixed by #63133
Assignees
Labels
[Feature] Block hooks [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 3, 2024

Description

See
#52969
#54029

In #63081 a new Stylelint custom rule is added to prevent the usage of the flex-direction CSS property values row-reverse and column-reverse values.

For more details about why this is a problem for accessibility and the very limited cases where these values can be used, please see #63048 and #61241

The UI for the hooked blocks toggles uses row-reverse. By doing so, it alters the meaning and interaction of the toggle labels and the toggles themselves. This is not OK for accessibility.

Also importantly: base component should not be altered with local implementations or CSS overrides.
The base component are designed, built, and tested with accessibility in mind. Or, at least, they aim to be as accessible as possible. Instead, it appeaers the implementation of this UI in #52969 and #54029 didn't take accessibility into consideration enough, neither the PRs had any accessibility label.

Local ad-hoc implementations and overrides also introduce UI inconsistencies that don't help providing an overall cohesive and consistent user experiences. In most of hte cases they just add cognitive load. Not to mention maintenances cost in the long term.

This UI needs to be refactored to not use row-reverse. To my understanding, it was used only to allow the addition of icons representing the plugins in the UI. I'd argue those icons add a little value. They're not necessary and they break the toggles in terms of expected design, consistency, usability, and accessibility.

From a semantics and accessibility perspective, the toggles are, under the hood, inputs of type checkbox. The expectation is to have the input on the left and the label on the right. Additionally, the unnecessary spacing between the label on the left and the toggle on the right doesn't help users who use screen magnifiers.

Cc @WordPress/gutenberg-components

References:
Anyone interested in understanding why, historically, labels for input fields are placed before the field and labels for checkboxes and radio buttons are placed after, may want to have a read at https://www.w3.org/WAI/WCAG22/Techniques/general/G162.html

Step-by-step reproduction instructions

function register_loginout_hooked_block( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( $anchor_block === 'core/post-content' && $position === 'after' ) {
		$hooked_blocks[] = 'core/loginout';
	}

	return $hooked_blocks;
}
add_filter( 'hooked_block_types', 'register_loginout_hooked_block', 10, 4 );

function register_pagelist_hooked_block( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( $anchor_block === 'core/post-content' && $position === 'before' ) {
		$hooked_blocks[] = 'core/page-list';
	}

	return $hooked_blocks;
}
add_filter( 'hooked_block_types', 'register_pagelist_hooked_block', 10, 4 );
  • Go to the Site Editor > Templates > Single Posts template.
  • Observe that:
    • The page list block is inserted before the post content.
    • The login/out block is inserted after the post content.
  • Select the post content block.
  • Observe the toggles to show/hide the hooked blocks in the Inserter > Block tab > Plugins section.
  • Observe the toggles are on the right and their labels on the left.
  • Expected: the toggles to be on the left and their labels to immediately follow to the right.
  • Inspect the sources and observe the toggle position is swapped with flex-direction: row-reverse; set on their container.

Screenshots, screen recording, code snippet

The highlighted toggles in the screenshot below are the ones with their order altered with flex-direction: row-reverse;.
Notice the design inconsistency with the default toggle above in the panel.

Screenshot 2024-07-03 at 11 45 15

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Package] Block editor /packages/block-editor labels Jul 3, 2024
@ciampo
Copy link
Contributor

ciampo commented Jul 3, 2024

The ToggleControl component correctly shows the toggle before its label.

From a quick look, here are the styles that are overriding the component's behavior:

/**
* Since we're displaying the block icon alongside the block name,
* we need to right-align the toggle.
*/
.components-toggle-control .components-h-stack {
flex-direction: row-reverse;
}
/**
* Un-reverse the flex direction for the toggle's label.
*/
.components-toggle-control .components-h-stack .components-h-stack {
flex-direction: row;
}

From the components' point of view, we can't really prevent consumers of the components to apply style overrides — which we always recommend against, especially if the style overrides use internal .components-* classnames (which are not part of the public APIs of those components).

This feels like something that should be rather discussed with @WordPress/gutenberg-design

@mirka
Copy link
Member

mirka commented Jul 3, 2024

I think this can be attributed in part to the lack of modularity in ToggleControl. I added it to #60799.

@afercia
Copy link
Contributor Author

afercia commented Jul 4, 2024

Imo, components should allow 'layout variants' in very very limited cases. Ideally, they should not.
For example, checkboxes / radio buttons on the left and their labels on the right is an industry standard. I explained why this should not be changed in a previous comment. Designers should not override the layout or be able to change it in any way because they may miss important usability and accessibility considerations.
At the very least, design guide lines and usage best practices should be added to the documentation.

@afercia
Copy link
Contributor Author

afercia commented Jul 4, 2024

One more consideration about icons usage in the current UI. I'm not sure using icons is ideal in the first place.
Hooked blocks may use icons that are very similar or identical to other icons used for other editor features. For example a 'search' icon, a 'social sharing' icon, a 'previous' icon and even the 'login/logout' icon in this UI may be potentially confusing for users. For example, as a user, I would see a lens icon followed by a text 'Search' and expect to click there and perform a search. Example screenshot with potentially confusing cases with core blocks below.

Screenshot 2024-07-04 at 14 55 44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block hooks [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants