-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix constrained tabbing failures with Safari and Firefox #47426
Fix constrained tabbing failures with Safari and Firefox #47426
Conversation
Size Change: +677 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Flaky tests detected in 47a5430. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4136315129
|
Hi @afercia it looks like this PR contains two separate fixes. One for constrained tabbing and one for modals? Does it make sense to split these into two smaller PRs? While the change for the constrained tabbing behavior seems quiet straightforward, I'd like to understand more the changes for the modal component. |
Hi @youknowriad Sure I might split this fix into two smaller PRs but I'm not sure it would make much sense. While this PR touches two components, it actually fixes one feature: constraining tabbing. |
Tests keep failing for unrelated reasons, mostly timeouts or crashes e.g. |
5d57d28
to
3e35ac3
Compare
Sorry for the delay. If these are part of the same issue, it's fine to keep them together. I have some questions here:
|
@youknowriad thanks for your feedback. I'll try to do my best to answer your question. The 'scrollable' fix will need to be applied wherever there is a scrollable element. That's not limited to the Modal or popover components. So I'm not sure
Scrollable elements need to be keyboard accessible to solve cases like #41500. To do that, the scrollable element needs to be focusable in the first place. Firefox is the only browser that natively makes scrollable elements focusable. It does that only when there's an actual content overflow. You can test native browsers behavior on this codepen. |
__( 'Scrollable section' ) | ||
: undefined | ||
} | ||
tabIndex={ hasScrollableContent ? 0 : undefined } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can extract all this logic into a hook const { ref, props } = useScrollable();
and just basically apply the "ref" and "props" to the element that is supposed to be scrollable? I think it might bring some clarity to the code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, not sure why we need childrenContainerRef
can't we just check the height of the scrollable itself (or at least its children) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scrollable element has a fixed height,
The height of the content (the children) determines the actual content overflow, when it's greater than the scrollable height.
There may be multipole children though. Not sure how to calculate eh height of multiple children that could also be dynamically rendered. Seems to me the simplest option is to check for the height of an additional container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can extract all this logic into a hook
I'm all for better abstraction and improvements. I'd prefer to do that when enhancing the Scrollable
component though. See #45809
This is a bugfix and as such, it looks good to me. |
@@ -65,6 +67,7 @@ function UnforwardedModal( | |||
onKeyDown, | |||
isFullScreen = false, | |||
__experimentalHideHeader = false, | |||
scrollableContentLabel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop is the only API change in this PR. I'm fine with this PR if we don't add maintenance burden for us like that.
So in the sense, I suggested the refactor to improve the code quality a bit and clarify how this works but I'm fine shipping this as is if we only focus on the bug fix without new APIs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fair enough. Will remove it and keep the default aria-label={ __( 'Scrollable section' ) }
74b3b9a
to
04b9c37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code is good to ship 👍 Might be good to get another a11y check though. @alexstine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test looks good 👍 . Just a small question there.
Thanks everyone for the feedback and review. I commented on #45809 reporting the findings from this PR and the suggestion to craft a new hook. It will need to wait for #41001 anyways. |
I do not think I can really add much to this PR. I trust your judgement @afercia . Thanks. |
Thanks. Merging 🎉 |
As of [Gutenberg 15.2](WordPress/gutenberg#47426), the `Modal` component no longer renders the passed React nodes as direct children of `.components-modal__content`, effectively breaking the flex layout used by our Limited Global Styles gating modal. To fix that, we now use a wrapper with a custom class so we can build a flex layout without relying on how Gutenberg renders the `Modal` component.
Fixes #46041
Fixes #41500
What?
The refactoring of
useConstrainedTabbing
in #34836 introduced a few regressions and unexpected behaviors. Some of these regressions have been fixed already, see for example #42652, #45903, ,and #42653.This PR aims to fix one remaining regression with Safari and some unexpected behavior with Firefox.
TL;DR
The regression and unexpected behaviors surfaced only when attempting to run the existing editor/various/a11y.spec.js test with all browsers in #46038. The outcome was 1 failure with webkit (Safari) and 2 failures with Firefox.
That proved the native browsers behavior differs in some cases. Therefore, the current implementation of
useConstrainedTabbing
, which relies on native browser behavior, doesn't appear to be ideal in the first place.I'd like to note that when it comes to some accessibility features, it is very, very important to run E2E tests with all ajor browsers. In fact, it turned out browser behavior differs in at least two cases:
useConstrainedTabbing
current implementation allows to tab to scrollable elements but it doesn't take them into account as 'tabbable' elements, leading to unexpected behavior.I'd also like to kindly note that it appears the refactoring in #34836 didn[t introduce any new test for the specific use case it was meant to address, see https://github.com//issues/34681. I will create a separate issue for that.
Why?
The constrained tabbing feature is broken since more than one year now. It needs to work reliably and consistently across all major browsers. It also needs to be tested more effectively.
How?
ResizeObserver
to listen to the modal dialog content size changes and checks again whether it's scrollable. This is necessary because:<div>
element: this is necessary to determine size changes viaResizeObserver
.aria-label="Scrollable section" tabindex="0"
to the dialog content when it's scrollablescrollableContentLabel
prop that can be used to override the default labelScrollable section
.Note:
useResizeObserver
for now. I'm not sure I like the current implementation which injects an additional<div>
element and overlays it on the node to check for resizing. Also, that works only when the node to check for is a positioned element.useResizeObserver
so that it can be used directly on an element. /Cc @youknowriadTesting Instructions
npm run test:e2e:playwright -- --headed editor/various/a11y.spec.js
and check the tests pass.role=document
does not have anaria-label
attribute nor atabindex
attribute.aria-label="Scrollable section" tabindex="0"
attributes.qwerty
in the 'Search for a block' search field.aria-label
attribute nor atabindex
attribute.aria-label="Scrollable section" tabindex="0"
attributes.Testing Instructions for Keyboard
See above.
Screenshots or screencast