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.

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

Jump to bottom

[Breadcrumbs] Focus goes to top of page after expanding in IE11 #20280

Closed
2 tasks done
malouro opened this issue Mar 25, 2020 · 5 comments · Fixed by #20489
Closed
2 tasks done

[Breadcrumbs] Focus goes to top of page after expanding in IE11 #20280

malouro opened this issue Mar 25, 2020 · 5 comments · Fixed by #20489
Labels
accessibility a11y component: breadcrumbs This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@malouro
Copy link

malouro commented Mar 25, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

In Internet Explorer 11, the focus is shifting to the top of the page after expanding the Breadcrumb list via keyboard.

(Note: if it helps to debug - after hitting the expand button, document.activeElement returns the <body> element of the page. This is also the case in Chrome and Firefox, but IE11 seems to handle the situation much differently.)

Expected Behavior 🤔

Focus should remain contained within the Breadcrumb list, possibly going to the next newly-revealed Breadcrumb.

Optionally, if there was some sort of available onExpand handler, this could probably be achieved via user implementation. (Unsure if this would constitute a separate issue all together, or even be a possible breaking change, but figured I'd put out the suggestion anyway.)

Steps to Reproduce 🕹

Using the current live documentation example of Collapsed Breadcrumbs, since I think codesandbox examples don't render in IE11?

Steps:

  1. Navigate to the above example in IE11
  2. Tab to the expand ... button in the "Collapsed breadcrumbs" example
  3. Hit enter
  4. Hit tab again and observe that the focus is now on the "Skip to content" button at the top of the page

Context 🔦

Testing that the Breadcrumb component is adequately accessible for our users (which a decent majority still actively use IE11 😢)

Your Environment 🌎

Tech Version
Material-UI v4.9.6
React
Browser IE 11
TypeScript
etc.
@oliviertassinari oliviertassinari added the component: breadcrumbs This is the name of the generic UI component, not the React module! label Mar 25, 2020
@oliviertassinari
Copy link
Member

The problem doesn't seem limited to IE 11. What do you think of this diff?

remote: Resolving deltas: 100% (143/143), completed with 29 local objects.
diff --git a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js
index d958e5362..c54eeddd2 100644
--- a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js
+++ b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js
@@ -63,8 +63,9 @@ const Breadcrumbs = React.forwardRef(function Breadcrumbs(props, ref) {
   const [expanded, setExpanded] = React.useState(false);

   const renderItemsBeforeAndAfter = (allItems) => {
-    const handleClickExpand = () => {
+    const handleClickExpand = (event) => {
       setExpanded(true);
+      event.currentTarget.parentNode.querySelector('a').focus();
     };

     // This defends against someone passing weird input, to ensure that if all

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Mar 26, 2020
@malouro
Copy link
Author

malouro commented Mar 26, 2020

Yeah something like that should work. 😄 I wouldn't mind strapping together a PR for this once I get the time to look at this a bit more

Thanks!

@oliviertassinari
Copy link
Member

@malouro Outside the need for a safeguard against potentially none a element, I believe it could land as above. Feel free to spend time on it.

@sheerryy
Copy link
Contributor

sheerryy commented Apr 3, 2020

@malouro @oliviertassinari Can I take this issue?

@malouro
Copy link
Author

malouro commented Apr 4, 2020 •
edited
Loading

@shehryarshoukat96 feel free to take this, haven't gotten the chance to get to it

@sheerryy sheerryy mentioned this issue Apr 10, 2020
Merged
1 task
@oliviertassinari oliviertassinari changed the title [Breadcrumbs] Focus goes to top of page after expanding in IE11 [Breadcrumbs] Keep focus in the component after expanding Apr 10, 2020
@oliviertassinari oliviertassinari changed the title [Breadcrumbs] Keep focus in the component after expanding [Breadcrumbs] Focus goes to top of page after expanding in IE11 Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accessibility a11y component: breadcrumbs This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Breadcrumbs] Keep focus in the component after expanding sheerryy/material-ui
3 participants
@oliviertassinari @malouro @sheerryy