WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.3k stars 4.11k forks source link

`@wordpress/components`: modality of popover-based components #63674

Open ciampo opened 1 month ago

ciampo commented 1 month ago

Context

As we're re-writing some of the components in the @wordpress/components package to use ariakit, we have the chance to re-discuss our approach to architecting our APIs and the behavior of such components.

More specifically, as we're working on the new version of DropdownMenu and CustomSelectControl, one of the aspects that we should discuss and agree on at a package level is the modality of popover-based components.

Using ariakit allows us to easily switch the modality (and related behaviour) of popover-based components (popover, tooltips, dropdowns, dropdown menus, dialogs, selects, comboboxes...) via a few props.

[!NOTE] Making a popover "modal" means that the rest of the page should be considered inert (inaccessible to assistive technology and non-interactable by the user while the popover is open, as explained well in this article).

In ariakit there are some related aspects that come with modality (and can be customized to a degree):

What

Therefore, we need to decide:

cc @WordPress/gutenberg-components

ciampo commented 1 month ago

I'll share my initial thoughts:

  • What should the default modality of popover-based components be? Should it be different depending on the component?

I'd opt for most components to be modal by default, except Tooltip

  • Should we expose APIs in our components to change such modality? If yes, for what components and in what shape?

We should in most cases, apart from:

  • Should make amends to the default ariakit behavior re. inline vs portaled popover depending on the modality?

I personally like ariakit's default behaviour, and I'd like to preserve it if possible. If rendering inline by default for non-modal popovers proves tricky, my second-best option would be to always portal them to document.body by default, and expose dedicated APIs for consumers of the component to change such behavior as needed (render inline, or change portal element).

Having said that, that's a topic with more nuances, and I decided to discuss it separately in #63697

  • Should make amends to the default ariakit behavior re.preventing body scroll depending on the modality?

I think that this behavior is sensible and I wouldn't change it. I would start without exposing dedicated props in our components, and see if we can get away with it.

  • Are we ok with the addition of a hidden dismiss button for modal popovers?

I am.

  • Do we need to take any additional precautions into account, given that there will be a transional phase during which some components will use the ariakit popover, and other components will use the first-party Popover from this package?

We will need to keep an eye on a few aspects:

Interactions between popover-based components that use different underlying components

For example, a dropdown menu that opens a modal.

We should be able to get around such issues because our new components will expose enough APIs that we could mimic the legacy Popover behavior if needed, and reach behavior parity.

Ideally, when all popover-based components use ariakit, we should be able to stop using the majority of these tweaks (slots, z-indexes, ...)

Introducing more modality in the editor

As of today, most popover-based components are not modal. So, making the modal by default would represent a bit of a change in how users interact with the component. We should keep an eye on that, and talk to product/design folks to get some early feedback.

tyxla commented 1 month ago

In general, I'm fine with introducing more modality in most of the popover-based components. That's how native controls work, and when playing with Storybook, I can see quite a few of our components that I expect to be modal, but they aren't. With that in mind, defaulting to modal makes sense to me.

That being said, let's get some early feedback from @WordPress/gutenberg-design to ensure we take any early thoughts into consideration when making direction decisions like this. Especially considering that there might be a longer transition period where some components will be modal while others won't be.

mirka commented 1 month ago

I'm pretty much in line with @ciampo's takes.

Are we ok with the addition of a hidden dismiss button for modal popovers?

I played around with this, and couldn't figure out how it becomes focusable. It's in the DOM but left out of the tab sequence with tabindex="-1". It didn't work with Ariakit.Dialog either. I'm theoretically ok with this feature, but can't fully assess at the moment due to not being able to actually trigger it πŸ˜…

ciampo commented 1 month ago

I'm theoretically ok with this feature, but can't fully assess at the moment due to not being able to actually trigger it πŸ˜…

Maybe @diegohaz can help

diegohaz commented 1 month ago

I'm theoretically ok with this feature, but can't fully assess at the moment due to not being able to actually trigger it πŸ˜…

Maybe @diegohaz can help

@mirka @ciampo

The hidden dismiss button added by modal dialogs in Ariakit is designed for screen reader users when a mouse and keyboard are not available (e.g., on mobile/touch devices). They can't click outside or press Esc, but they can access this button.

Without this button, they wouldn't be able to close the modal dialog or popover.

It's rendered only if the developer does not deliberately render DialogDismiss, PopoverDismiss, or another derivative component.

mirka commented 1 month ago

I confirmed the StackBlitz demo works as expected on iOS Safari with VoiceOver. Very cool feature, and completely non-intrusive to a user who doesn't need it. Thumbs up from me as well. Thanks @diegohaz!

joedolson commented 1 month ago

Having a dismiss button is most helpful because it makes the action needed to dismiss visible and obvious to users who don't know other methods of exiting, such as esc or off-control cilcks.

I'm opposed to adding a visually hidden dismiss; I think it needs to be visible and always available. Current popover controls have a visible dismiss button, and I'd consider removing that to be a significant accessibility regression; and I don't see any point in adding a visually hidden dismiss button when we have a visible one already.

I think our usage of this component should default to including the appropriate DialogDismiss or PopoverDismiss component, but I'm glad that there is at least some fallback if a developer omits those.

I'd prefer if it was also available via normal keyboard & screen reader navigation; I'm not sure why it's restricted to mobile interfaces only. @diegohaz can you provide any explanation of that?

diegohaz commented 1 month ago

I'd prefer if it was also available via normal keyboard & screen reader navigation; I'm not sure why it's restricted to mobile interfaces only. @diegohaz can you provide any explanation of that?

It's not restricted to mobile devices. It's accessible to the virtual cursor on any device or screen reader. Making it accessible to non-screen reader keyboard navigation would require Ariakit to make this button visible, which involves design decisions beyond the library's scope.

I agree that always rendering a visible dismiss button is better, and @wordpress/components can do that. My comment was only to explain what Ariakit does within its limited scope.

ciampo commented 1 month ago

Current popover controls have a visible dismiss button, and I'd consider removing that to be a significant accessibility regression

Hey @joedolson, I believe that a visible dismiss button is only present in the Modal component.

Here we're discussing modality as a general attribute for all popover-based components. I believe that it would be quite difficult to show a visible dismiss button for all modal popover - based components, and instead we should evaluate the addition of a visual dismiss button on a case by case.

For example:

alexstine commented 1 month ago

Some thoughts.

  1. Modal dialogs should indeed be modal dialogs but I do not think this should apply to selects or menus. These have their own accessibility tree computed attributes and it shouldn't be up to us to interfere with that.
  2. One good example of this would be setting aria-haspopup to any other value besides dialog and then upon selecting it, the user hears a dialog has opened. For example:
    <button type="button" aria-expanded="false" aria-haspopup="menu">Options</button>

    Selecting this should open a menu, not a dialog.

Docs:

  1. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup
  2. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/menu_role

While I think this is a great fit for Popovers and modals in general, let's be careful to not throw everything in a modal dialog as we run a high risk of misusing ARIA and miscommunicating what type of interactive controls a widget may have.

Thanks.

ciampo commented 1 month ago

Hey @alexstine , good to hear from you!

Just to clarify, we are discussing whether we should support modality for popover-based component, what default modality should each component have, and any related aspects (like dismiss buttons).

We would not be changing the role of selects or menus to 'dialog' β€” but we would potentially make them modal by default. Hope this clarifies it!

alexstine commented 1 month ago

@ciampo How are we defining modal here?

I believe all dialogs should have visible dismiss buttons, inline dialogs as well as modal dialogs.

Thanks.

afercia commented 1 month ago

I'd agree with @alexstine about the semantics.

The Popover component is used for many things though.

Some dropdown menus do use a Popover, for example all the dropdown menus in the block toolbar. In this case, the semantics should be 'menu' and should not be changed.

Other controls in the editor use a Popover, for example the color picker or the Global styles Shadows editing UI and many others. Typically, these UIs look and behave more like small dialog windows that contain controls or small forms. These aren't menus. They are more non-modal dialogs. Some of them do have an aria-label for the popover container but don't have an ARIA role, which is incorrect and not useful for users. Sine of them do havee a visible title, most don't. See https://github.com/WordPress/gutenberg/issues/63899#issuecomment-2248249080 There's plenty of inconsistencies and room for improvements.

How these Popovers should be treated? I'd think they should be non-modal dialogs because that's the closest ARIA role and UI pattern users would easily understand. I do realize it's not ideal but I cant think of a better role and interaction model. Any thoughts welcome.

ciampo commented 1 month ago

Let's make sure that we're using the same terminology and general concepts.

I'm going to quote some content from this excellent article, from MDN, and from the WAI-ARIA APG:

popover

A piece of UI (and as of recently, an HTML attribute too) that is not always visible (ie. ephemeral), that is displayed on top of other page content. It can be modal, or non-modal. Popovers don't necessarily come with a built-in role, as they assume many (dialog, listbox, menu, tooltip...).

In terms of DOM tree, Popovers can be rendered inline next to their anchor, or they can be rendered somewhere else in the DOM (we'd be using React portals in Gutenberg).

modal

Modality is more of a characteristic than a component itself.

When a modal component is open, it is the only thing that is not inert. Only the modal content can be interacted with, the rest of the page or application is made inert. Inert content is content that users cannot interact with. It is only really there visually, but you cannot Tab to it, click it, scroll it, or access the content via assistive technologies.

dialog (role)

The dialog role marks a descendant window of the primary window of a web application. It helps assistive technology identify the dialog's content as being grouped and separated from the rest of the page content

<Dialog /> (component)

Although we don't have a component called Dialog in the @wordpress/components library (it's mistakenly called Modal), we usually refer to Dialog component as a piece of UI that follows the WAI-ARIA Dialog pattern and therefore renders a modal dialog.


I believe all dialogs should have visible dismiss buttons, inline dialogs as well as modal dialogs.

I don't think that non-modal dialogs must have a visible dismiss button. It's not a pattern that I see used across reputable UI libraries, and I believe it would be very difficult to implement across Gutenberg's UI.

On the other hand, I agree that modal dialogs should have a visible dismiss button.

The Popover component is used for many things though.

In the @worpress/components package, the <Popover /> component is indeed used across many components, as it represents the low-level primitive used to implement popover-based UI.

The hypothetical new version of <Popover /> would offer a lot of flexibility (modal/non modal, rendered inline or in a portal, etc etc), so that its consumers can tweak it as needed. So, we'd just really need to decide whether <Popover /> should be modal or non-modal by default.

Some dropdown menus do use a Popover, for example all the dropdown menus in the block toolbar. In this case, the semantics should be 'menu' and should not be changed.

That is already the case β€” as mentioned above, semantics won't be affected. But, given the above definitions, a popover with role="menu" could be modal: while the semantics won't be affected (it will remain a menu, it will be labelled as expected, etc etc), if a modal popover menu is open, the rest of the page becomes inert.

And therefore, the topic that we'd like to discuss in this issue is: is it ok to allow a dropdown menu's popover to be modal? If so, should the dropdown menu's popover be modal or non-modal by default?

The same reasoning should be applied to other components.

Other controls in the editor use a Popover, for example the color picker or the Global styles Shadows editing UI and many others. Typically, these UIs look and behave more like small dialog windows that contain controls or small forms. These aren't menus. They are more non-modal dialogs. Some of them do have an aria-label for the popover container but don't have an ARIA role, which is incorrect and not useful for users. Sine of them do havee a visible title, most don't. See https://github.com/WordPress/gutenberg/issues/63899#issuecomment-2248249080 There's plenty of inconsistencies and room for improvements.

While I agree that there's room for improvements, if such components are not used correctly in the editor, that's something that should be discussed separately.

In this issue, we should focus on deciding how we can best support modality in popover-based components. As maintainers of @wordpress/components, we want to be able to offer flexible and compliant components that can empower theirs consumers to build UIs that are semantically correct and accessible.

I'd think they should be non-modal dialogs because that's the closest ARIA role and UI pattern users would easily understand. I do realize it's not ideal but I cant think of a better role and interaction model. Any thoughts welcome.

Got it β€” I'll register your feedback about the fact that <Popover /> should be non modal by default


@WordPress/gutenberg-components , should we consider introducing less modality by default to our popover-based components (which is closer to how the things are currently working)? We could always implement some behaviors like preventing body scroll and closing the popover when there's an outside interaction without the modality.

alexstine commented 1 month ago

@ciampo Any chance some draft PRs can spin up so I can just test the specific ideas? Sometimes I just don't follow text explanations well without seeing it in practice.

tyxla commented 1 month ago

@ciampo Any chance some draft PRs can spin up so I can just test the specific ideas? Sometimes I just don't follow text explanations well without seeing it in practice.

@alexstine, thanks for your interest in taking a look!

@ciampo is on vacation for the next week, but I'm happy to help demonstrate the non-modal behavior in order to keep the discussion going. I've opened a PR here: https://github.com/WordPress/gutenberg/pull/64200 - please review that PR for testing instructions.

A few notes about this example:

Let me know if you have any questions!

alexstine commented 1 month ago

@tyxla Is there anyway you can add this to something used in the post editor possibly? I'd like to give it a quick test in Playground without having to build it locally. Wish Storybook had a web version to test PRs vs. just latest trunk.

Thanks.

tyxla commented 1 month ago

@tyxla Is there anyway you can add this to something used in the post editor possibly? I'd like to give it a quick test in Playground without having to build it locally. Wish Storybook had a web version to test PRs vs. just latest trunk.

Thanks.

@alexstine Of course! Luckily, DropdownMenuV2 is used in the site editor, so you can spin up a Playground instance with this PR and play with it. I've updated the testing instructions of the PR, and I'm posting them here for posterity:

Let me know if you have any other questions.

Thanks for testing @alexstine, and I appreciate any feedback you might have!

alexstine commented 1 month ago

@tyxla Thanks for the instructions. Here are my thoughts.

  1. There is no need for a dismiss button here, escape is an okay interaction pattern for closing a submenu like this one.
  2. I believe the content should be inert because this menu is not rendered in the DOM with the associated trigger. This is why using React portals is not a good idea. The trigger element should control the content just below it in the DOM. If a user were to move focus away from the menu, the context would make no sense as they are at the bottom of the DOM compared to the position of the triggering element which may be near the top of the DOM, middle, etc.

Thanks.

diegohaz commented 1 month ago

2. I believe the content should be inert because this menu is not rendered in the DOM with the associated trigger. This is why using React portals is not a good idea. The trigger element should control the content just below it in the DOM. If a user were to move focus away from the menu, the context would make no sense as they are at the bottom of the DOM compared to the position of the triggering element which may be near the top of the DOM, middle, etc.

@alexstine I believe the context is preserved here even when using React Portal, meaning you can tab away from the menu and it works as if it were rendered right after the menu button. The same applies to the virtual cursor (if you disable focus mode while in the menu).

There's additional logic to ensure this works. I made this video a while ago to explain the strategy adopted here (sorry, my speech isn't good, but I'm talking about things you're likely already familiar with, so it should be fine to follow): https://www.youtube.com/watch?v=ELauJdPoDaQ

If you encounter any issues with this interaction, please feel free to report them.

Hopefully, in the near future, we will be able to use the native HTML popover feature that already does this, but in the meantime, this should work similarly.

alexstine commented 1 month ago

@diegohaz It does not work the same way. Upon following the testing instructions, I can navigate out of the menu by disabling focus mode. The first element I find above the menu in the DOM is the aria-live notifications region. The portal is attached as the last child in the body, not associated with the trigger.

Thanks.

diegohaz commented 1 month ago

The first element I find above the menu in the DOM is the aria-live notifications region. The portal is attached as the last child in the body, not associated with the trigger.

@alexstine I don't fully understand. Are you able to move the virtual cursor to the notification region, or are you talking about navigating the DOM tree view (not the site UI)?

Also, could you please let me know which screen reader and browser you're using? Thanks!

I just tested with NVDA and Edge. Either pressing Shift+Tab or Arrow Up with focus mode disabled correctly moves the focus or the virtual cursor to the menu button even though the menu popup is placed at the end of the DOM. Also tested with VoiceOver and Safari. VO+Arrow Left correctly moves the focus to the menu button.

alexstine commented 2 weeks ago

@diegohaz I am using NVDA and Firefox. If I switch to the virtual cursor (browse mode) pressing up arrow lands on the aria-live region injected by wordpress/a11y. By default, most of the portals I've seen in Gutenberg render near the end of the DOM, not as siblings/descendants of their controls.

diegohaz commented 2 weeks ago

@diegohaz I am using NVDA and Firefox. If I switch to the virtual cursor (browse mode) pressing up arrow lands on the aria-live region injected by wordpress/a11y. By default, most of the portals I've seen in Gutenberg render near the end of the DOM, not as siblings/descendants of their controls.

@alexstine I can test it again later. The last time I tested it with NVDA and Firefox, it worked properly. The portal is rendered at the end of the document in the DOM, but it's referenced by aria-owns on a sibling element of the original control that triggered the popover. Therefore, the accessibility tree should be structured as if there were no portal.

diegohaz commented 2 weeks ago

Here's an audio/video I recorded using NVDA 2024.2 and Firefox 129.0.1 on Windows 11, following the instructions in #64200

In the first part of the video, I use Tab and Shift + Tab to navigate from the menu button to the menu, to the table (the element after the menu button), and back to the menu and the menu button. Then, I use arrow up and arrow down in browse mode to navigate from the menu button to the menu items and back to the menu button.

The order of elements in both scenarios seems correct even though the menu is using React Portal to be rendered at the end of the DOM.

https://github.com/user-attachments/assets/90c6b5a8-3be0-499a-9f7d-3fdb56896210

afercia commented 10 hours ago

Some feedback on a few points from this discussion.

Popovers don't necessarily come with a built-in role, as they assume many (dialog, listbox, menu, tooltip...).

@ciampo, sure and that also means that Popovers must have a role depending on how they are used. When they're used for dialog-like UIs, they should have a role dialog, when they're used for menu-like UIs they whould have a role menu and so on. As such, the Popover component should have a required prop to set an appropriate role and it should not allow implementations without a role.

On the other hand, I agree that modal dialogs should have a visible dismiss button.

I agree and that was discussed several times across the yers iin this project. Alas, some designs don't want a dismiss button ad right now there's larte inconsistency in the editor. One exampel of that is in the varous 'welcome' modal dialogs. To me, a dismiss button should be required.

So, we'd just really need to decide whether should be modal or non-modal by default.

Yes, but the modality also implies that the Popover must have an appropriate ARIA role. I'd like to remind everyone that the concept of 'modality' and thus the aria-modal attirbute can only be used with the dialog and alertdialog roles.

the topic that we'd like to discuss in this issue is: is it ok to allow a dropdown menu's popover to be modal?

What would be the use case for a dropdown menu that is modal? I'm not sure I've ever seen a menu that makes inert the rest of the page other than in Gutenberg and to me that's an incorrect pattern. Glad it will be revised as mentioned by @tyxla.

if such components are not used correctly in the editor, that's something that should be discussed separately.

Regarding required ARIA roles and attributes, I'd argue that if the components don't make them requried, it is very likely the components will be used incorrectly. This is no different from other long-standing issues like the non-required label prop for inputs and buttons, which made a lot of harms across the 7 years of life of this project. I do realize components should be 'flexible' but they also should be 'strict' in requiring the necessary props to guarantee correct semantics and highest level of usability and accessibility.

In this issue, we should focus on deciding how we can best support modality in popover-based components. As maintainers of @wordpress/components, we want to be able to offer flexible and compliant components that can empower theirs consumers to build UIs that are semantically correct and accessible.

Agreed and appreciated. We all want that, not only you as maintainers πŸ™‚

I believe the content should be inert because this menu is not rendered in the DOM with the associated trigger.

This is a good point from @alexstine. Ideally, any UI that renders in the DOM far from the toggle control that opened it should be treated like a modal dialog. As Alex explains, assistive technologies provide dozens of ways to exit an UI even if tabbing is constrained within that UI. When that happens, users would be totally lost because there's a lot of content in the DOM between the Popover and the toggle that opened it. Which leads us to discuss the 'portal' pattern. It should really be used only for UIs that are modal as in UIs that make all the rest of the page inert.

Also the above point has been discussed several times across the years in this project but always dismissed for a reason or another reason. I would like to reinstate once again that for accessibility:

tyxla commented 6 hours ago

the topic that we'd like to discuss in this issue is: is it ok to allow a dropdown menu's popover to be modal?

What would be the use case for a dropdown menu that is modal? I'm not sure I've ever seen a menu that makes inert the rest of the page other than in Gutenberg and to me that's an incorrect pattern. Glad it will be revised as mentioned by @tyxla.

@afercia: note that I never said we're necessarily going in that direction. In my comment, I was explaining how the PR works, but as specified in the PR, it was solely for demonstration purposes and getting feedback. The whole idea was to test different ways to set things up and see how we can achieve the best compromise.