-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(tabs): refactor dismissable tab html #16033
fix(tabs): refactor dismissable tab html #16033
Conversation
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thanks for digging into this! I think it looks good from a code review standpoint. The landscape around this is kind of a mess. This stackoverflow post explains it pretty well. There's also a big thread about it that hasn't been resolved in w3c/aria#1440 I think this technically doesn't follow WAI-ARIA 1.2 because a @tombrunet when choosing between fixing this or following the spec, I think we should side with the user and fix it regardless of what the spec says, but what do you think? Also, this contains a change that could break tests or custom functionality in consuming applications because of the shift in the DOM. Despite that, I would support shipping this to all users in our next minor without a feature flag due to the severity of the bug. |
So, what we're effectively doing is saying that there is an item in the tab order between the tablist and the tab panel, which is "Dismiss current tab". It happens to be shown inside the tab item, but it is effectively outside the tablist. I wonder if one approach here is to treat this somewhat like the skiptomain function. Rather than put the focus to the X in the tab item, we make a "Delete tab item" that appears to the right of the tablist after a user tabs out of the tablist. Does that solve anything? It suggests to me it would let us solve the discoverability and actionability requirement AND would not violate ARIA. @tombrunet ? Note that I'm suggesting this as additional behaviour to the current use of a Delete key press. I think we'd retain that, but also provide this. |
@mbgower The largest problem with that approach is that if you change reading modes on the close button, you are out of context. If I go to the close button and then ask to read the previous word, I expect to hear the name of the tab I'm on, not the name of the last tab in the list. Looking at some other apps, it does seem that handling of these tab widgets is changing. VS Code has a behavior similar to what Carbon is proposing here (though the close button is only visible for the active tab and on mouse hover). Chrome is actually making every tab widget and close button tabbable. |
I also think putting the tablist into an edit function is another potential notion to deal with this.
There's even the possibility that rather than deal with these edits in the tablist, the tab panel for the Edit items tab would actually literally be a series of inputs (deletable) for each of the existing tab items. I really don't know how often the delete tab function is used in Carbon products, so have no idea how radical this rethink is I realize I'm just tossing out ideas here, which is not really to the purpose. I suggest we can continue this at the CAG call on Monday. |
@alisonjoseph, I've disrupted your fix with my comments, so I wanted to put in an epilogue on this. I have a few cautions on the fix which need to be addressed, as well as some suggestions on future changes. But once those issues are addressed (numbers 1 and 3 below) this looks good to go from an accessibility problem when using JAWS. I can do future testing with iOS VO as well. Details below.
|
@mbgower thanks for the detailed feedback, I believe I've addressed all of your concerns if you could take another look.
|
@alisonjoseph, all these things have been accomplished, nice!
One thing I noticed is that when the last active tab is deleted (Activity tab), things get a little odd, with either the focus going to the disabled 4th Settings delete button, or focus going up the page. The disabled item may be the curve ball here. It might be an idea to strip that out in some tests and get it to behave predictably with a set of three enabled tabs: what happens if one deletes the LAST item and there are still prior ones available? My expectation would be that the focus would go to the Monitoring tab (the prior one). If you folks concur, is that doable? Once that makes sense, the disabled component is a bit of an outlier, IMO. I think if there are no more enabled items left, I'd treat it the same as if there were no items left, and got to the prior item? |
I think this is ready for review now. @tay1orjones @andreancardona @mbgower I've updated it so the previous tab should now take focus after removing a tab. Just wanted to double check that we wouldn't want the active tab to take focus vs the previous one? |
I wanted to clarify that I wasn't saying that each time a tablist item is deleted, the focus should go to the prior tablist item; I think 'next tab item, where available' is a good approach, and supports the APG pattern previously quoted. I was specifically talking about the situation where the last item (or the last non-disabled item) in the tablist is deleted. In both cases, I think it probably should go to the prior tablist item. It's worth noting that whichever approach you take (focus to next or focus to prior) you're always going to have to have the use case addressed where the intended focus point no longer exists -- i.e., there is no next item, or there is no prior item in the tablist. For that matter, you also need a plan for what happens to focus when there is NO item in the tablist (which theoretically shouldn't be possible, since according to the documentation I think there are supposed to be a minimum of 2 tab items, but I think I've seen it) |
@mbgower ok, focus states are updated. |
That's working well with JAWS. I also confirmed it is usable and understandable with VO on iOS. Nice! |
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 tested with Jaws and looks good to me! 🎉
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.
LGTM!
0376df7
Hey there! v11.55.0 was just released that references this issue/PR. |
* fix(tabs): refactor dismissable tab html * fix: typescript * feat: add button reset, active state and fix number values * fix: disabled styles * fix: styling issues * fix: fullwidth tabs * fix: update close to remove & add tab name & hide on non dismissible * chore: code comments * fix: tests * fix: set focus after removing tab * fix: focus * fix: set focus to active tab * feat: update focus state after removing tab * chore: add code comments * fix: focus updates * fix: focus states * Update packages/react/src/components/Tabs/Tabs.tsx --------- Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>
Closes #14820
Ref ##15919
Previously you were only able to dismiss a dismissable tab with the delete key or mouse. This leaves voiceover users on an iphone unaware the dismiss icon even existed.
This is a complete refactor of the html for dismissable tabs, it changes the icon to a button, and moves it outside of the containing tab button and into the tab order. This allows both keyboard and voiceover users to tab to the icon and dismiss the tabs.
Changelog
Changed
Testing / Reviewing
Make sure there are no regressions with any of the tab variants. Be sure to check dismissable in both regular and contained.
Check that the tab order is correct and that the dismiss icon is read on voice over.