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

Snackbar: Display Snackbar on top of Modal Screen Overlay #68728

Open
3 of 6 tasks
yogeshbhutkar opened this issue Jan 17, 2025 · 16 comments · May be fixed by #68975
Open
3 of 6 tasks

Snackbar: Display Snackbar on top of Modal Screen Overlay #68728

yogeshbhutkar opened this issue Jan 17, 2025 · 16 comments · May be fixed by #68975
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Regression Related to a regression in the latest release

Comments

@yogeshbhutkar
Copy link
Contributor

Description

The z-index of the snackbar component currently matches that of the modal screen overlay:

// Show snackbars above everything (similar to popovers)
".components-snackbar-list": 100000,
// Show modal under the wp-admin menus and the popover
".components-modal__screen-overlay": 100000,

As a result, the snackbar is rendered behind the modal overlay. Since snackbars provide critical feedback for user actions, they should ideally appear above the modal overlay to ensure their visibility.

For example, when inserting a Pattern through the Explore all patterns modal, selecting a pattern successfully adds it to the post content and triggers a snackbar notification. However, because the snackbar is hidden behind the modal overlay, users cannot see the feedback. This behavior does not provide proper feedback to the user and forces them to close the modal to confirm the action manually.

This behavior is demonstrated more clearly in the attached video.

Step-by-step reproduction instructions

  1. Navigate to the post edit screen.
  2. Open the Block Inserter.
  3. Navigate to the Patterns tab and open the Explore all patterns dialog.
  4. Try selecting a Pattern from the Dialog.
  5. Notice the snackbar got generated behind the Dialog.

Screenshots, screen recording, code snippet

issue.mov

Environment info

  • WordPress Version: 6.8-alpha-59361
  • Gutenberg Version: Trunk (5688333)
  • OS: macOS Sequoia

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

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@yogeshbhutkar yogeshbhutkar added the [Type] Bug An existing feature does not function as intended label Jan 17, 2025
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jan 17, 2025
@Mamaduka
Copy link
Member

The current behavior might be intentional. When a modal dialog is open, the content behind it should be inert. Additionally, clicking on a "Snackbar" to dismiss will cause the modal to close, which I think isn't ideal behavior.

cc @t-hamano, @afercia

@yogeshbhutkar
Copy link
Contributor Author

Displaying the snackbar behind the modal overlay may prevent users from receiving the intended feedback after inserting a pattern.

Additionally, clicking on a "Snackbar" to dismiss will cause the modal to close, which I think isn't ideal behavior.

If the snackbar is positioned behind the modal, wouldn’t attempting to interact with anything behind the modal naturally result in the modal closing?

@Mamaduka
Copy link
Member

If the snackbar is positioned behind the modal, wouldn’t attempting to interact with anything behind the modal naturally result in the modal closing?

In normal flow, you just dismiss "Snackbar" and continue doing what you did. But with the modal, you'll have to re-open it, which could be inconvenient.

I don't have an answer to what's the best UX here; this is why I requested additional feedback before we try to solve anything.

@t-hamano
Copy link
Contributor

When a modal dialog is open, the content behind it should be inert.

Given this, maybe the snackbar isn't the right choice.

Instead, should we show the notification inside a modal, like the font library modal?

Image

Or, if the modal was to be closed when the pattern was inserted, the current snackbar would make sense.

When the pattern modal was first implemented, it seemed to close the modal when the pattern was inserted (see the video attached to #35773). It might be a good idea to first look into when and why this behavior was changed.

@afercia
Copy link
Contributor

afercia commented Jan 17, 2025

When the pattern modal was first implemented, it seemed to close the modal when the pattern was inserted (see the video attached to #35773). It might be a good idea to first look into when and why this behavior was changed.

I'd agree this should be investigated to understand what the reasoning behind the change was.
A modal dialog is supposed to prevent interaction with the rest of the page. That's why it is qualified as modal (which is a behavior, not a name). Either the snackbar should not be used in this case or the modal dialog behavior should be reconsidered.

Edit: In WordPress 6.6 the Patterns explorer modal dialog closes when inserting a pattern so the change in behavior must be pretty recent.

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release and removed [Type] Bug An existing feature does not function as intended labels Jan 17, 2025
@afercia
Copy link
Contributor

afercia commented Jan 17, 2025

Raising this issue to Regression as the reported bug introduces a significant problem for keyboard interaction.

In fact, when using the keyboard and inserting a pattern from the modal dialog, the dialog stays open and focus moves to the inserted pattern in the editor canvas. As such, focus is outside the modal dialog, which is something that should never happen.

I do realize that when using the mouse this regression isn't immediately clear but this only proves that whatever the change that introduced this bug is, it hasn't been tested sufficiently with keyboard. This is far from ideal as largely untested changes keep being introduced in the UI often breaking basic interaction flows.
I'd greatly appreciate more caution when changing anything related to the UI and more importatnly I'd appreciate better testing. Cc @WordPress/gutenberg-core

To reproduce.

  • Open the pattern explorer modal dialog.
  • Use the keyboard to navigate to the list of patterns within the modal dialog.
  • Once on the first pattern, you can use the arrow keys to navigate the patterns.
  • Press Enter on a pattern of your choice.
  • The pattern gets inserted in the editor. Observe the focus style around the pattern you selected disappears.
  • Press the Tab key a couple times.
  • Observe focus is on the 'Close Settings' button in the inspector, outside of the modal dialog.

Screenshot:

Image

I'll update this issue title for better clarity.

Wondering whether we should close this issue and create a new one to report the root cause of the bug: the modal dialog should close.

@t-hamano
Copy link
Contributor

When the pattern modal was first implemented, it seemed to close the modal when the pattern was inserted (see the video attached to #35773). It might be a good idea to first look into when and why this behavior was changed.

I would like to investigate this point first.

@t-hamano
Copy link
Contributor

I identified this behavior as being changed in #63059.

The discussion in #61051 was about whether to keep the sidebar inserter open or not. It seems that they ultimately agreed on a policy of not closing the sidebar inserter when clicking on the canvas.

However, there was no mention of the Pattern Explorer modal, so the current behavior may not be what was intended.

Perhaps we need to decide which is the ideal behavior for the pattern explorer modal.

When we click on a pattern in the pattern explorer modal:

  • Close the modal. Show a snackbar. Focus the inserted pattern.
  • Don't close the modal. Notify in some way instead of a snackbar. Don't focus the inserted pattern.

@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Jan 20, 2025

Thanks a lot for this research.

As this might not be the intended effect, I favor implementing "Close the modal. Show a snackbar. Focus the inserted pattern." I will be updating the existing PR to reflect the same.

What are your thoughts?

@afercia
Copy link
Contributor

afercia commented Jan 20, 2025

I identified this behavior as being changed in #63059.

Thanks for tracking down the change that introduced this regression. So, to my understanding, the modal dialog used to close when inserting a pattern only because the Inserter panel used to close in the first place? Interesting.

Personally, I'm not sure the modal dialog should close automatically on insertion.

  • The UI doesn't really explain that clicking a pattern should insert it and close the modal dialog. Users have to discover it.
  • I would like the UI to be more self-explanatory and provide a flow where users can also select multiple patterns to be inserted.
  • I would like the UI to be more consistent with other established patterns in WordPress. For example, with the Media Library where users can 1) select and 2) click a button to insert and close.

@carolinan
Copy link
Contributor

carolinan commented Jan 22, 2025

Stupid question: can't we just make sure that the modal height leaves the space for the snackbar to show? The content is already scrollable.

@t-hamano
Copy link
Contributor

can't we just make sure that the modal height leaves the space for the snackbar to show?

It might solve the visual problem, but it doesn't solve the accessibility problem.

I think the outside of the modal needs to be inert.

@afercia
Copy link
Contributor

afercia commented Jan 22, 2025

I think the outside of the modal needs to be inert.

Yes, anything outside of the modal dialog is already not perceivable. The Modal component adds aria-hidden="true" to all the body children except script elements, live regions, and elements that are already hidden. See the Modal component aria-helper.

As such, the Snackbar is not perceivable by assistive technology. Instead, the speak message triggered by the Snackbar should still work as expected.

@t-hamano
Copy link
Contributor

The current behavior of the Pattern Explorer modal is an unintended change introduced in #63059. Therefore, I propose to at least revert to the previous behavior, where clicking on any pattern in the Patterns modal closes the modal and focuses the inserted pattern.

This is also important to fix the accessibility regression below:

In fact, when using the keyboard and inserting a pattern from the modal dialog, the dialog stays open and focus moves to the inserted pattern in the editor canvas. As such, focus is outside the modal dialog, which is something that should never happen.

After that, we can consider more advanced improvements like these:

  • The UI doesn't really explain that clicking a pattern should insert it and close the modal dialog. Users have to discover it.
  • I would like the UI to be more self-explanatory and provide a flow where users can also select multiple patterns to be inserter.
  • I would like the UI to be more consistent with other established patterns in WordPress. For example, with the Media Library where users can 1) select and 2) click a button to insert and close.

Incidentally, the original suggestion in this issue was about the stacking order of snackbars and modal screen overlays. Given the discussion so far, the current behavior may be correct. It may be best to avoid using the modal + snackbar pattern instead.

@afercia
Copy link
Contributor

afercia commented Jan 24, 2025

Therefore, I propose to at least revert to the previous behavior

I'd agree this seems to be the most reasonable thing to do for now because, as pointed out, it's an unintended change that introduced a regression.

@yogeshbhutkar
Copy link
Contributor Author

Thanks for the great discussions! 🚀 I've linked a PR that restores the previous behavior. 🔄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Regression Related to a regression in the latest release
Projects
None yet
5 participants