Skip to content

Fix constrained tabbing failures with Safari and Firefox #47426

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

Merged
afercia merged 11 commits into from
Feb 14, 2023
Merged

Fix constrained tabbing failures with Safari and Firefox #47426

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7c087b4
Make the modal content container focusable when scrollable.
afercia Jan 20, 2023
80ea82d
Add test, improve existing a11y tests and run them with all browsers.
afercia Jan 20, 2023
7a87f07
Fix useConstrainedTabbing edge cases.
afercia Jan 23, 2023
66a4693
Add basic focus style for the scrollable section.
afercia Jan 24, 2023
2ce7d2a
Update snapshots.
afercia Jan 25, 2023
4fcb5b7
Clean up a11y test.
afercia Jan 25, 2023
3900724
Refine test.
afercia Jan 25, 2023
abc31c2
Remove scrollableContentLabel prop.
afercia Feb 9, 2023
04b9c37
Restore conditional aria-label for the scrollable section.
afercia Feb 9, 2023
47a5430
Update test after change to closeButtonLabel default.
afercia Feb 9, 2023
50da893
Add comment to clarify test.
afercia Feb 10, 2023
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
Prev Previous commit
Next Next commit
Restore conditional aria-label for the scrollable section.
  • Loading branch information
@afercia
afercia committed Feb 9, 2023
commit 04b9c374ca4f2d09b1ffac5fa534fdc00c7d7a4c
6 changes: 5 additions & 1 deletion packages/components/src/modal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,11 @@ function UnforwardedModal(
role="document"
onScroll={ onContentContainerScroll }
ref={ contentRef }
aria-label={ __( 'Scrollable section' ) }
aria-label={
hasScrollableContent
? __( 'Scrollable section' )
: undefined
}
tabIndex={ hasScrollableContent ? 0 : undefined }
Copy link
Contributor

@youknowriad youknowriad Feb 1, 2023

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.

Copy link
Contributor

@youknowriad youknowriad Feb 1, 2023

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) ?

Copy link
Contributor Author

@afercia afercia Feb 9, 2023

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.

Copy link
Contributor Author

@afercia afercia Feb 9, 2023

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

>
{ ! __experimentalHideHeader && (
Expand Down