-
-
Notifications
You must be signed in to change notification settings - Fork 817
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
Add ability to automatically hide tab-group scroll control #2128
Add ability to automatically hide tab-group scroll control #2128
Conversation
… are no longer any tabs to show
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
80fdbd1
to
d603b37
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.
Thanks for the PR! I'm of the opinion we should probably make this the default behavior instead of an additional option. I've pinged @lindsaym-fa for her thoughts on that.
I've added some comments and a few suggestions herein, but this is a really solid PR. Thanks for putting in the effort!
The only other thing I'd suggest — mostly to save yourself time in case it's something we can't accept or somebody is already work on — is to open an issue first next time so we can track and plan things without conflict :)
Thanks again!
/** Hide scroll buttons when inactive. */ | ||
@property({ attribute: 'auto-hide-scroll-buttons', type: Boolean }) autoHideScrollButtons = false; |
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.
I'm actually thinking this should be the default behavior. What do you think, @lindsaym-fa?
TL;DR – when tabs scroll, we show arrows to the left and right by default (unless no-scroll-controls
is present). The arrows are always visible, but this PR hides them when scrolling that direction isn't possible.
Open questions:
- Should this be the default?
- Should the arrows be hidden entirely or styled to look disabled?
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.
Agreed that it should be the default behavior!
I'm open to hiding the arrows entirely, though there's one UX issue that's a dealbreaker for me on this point: when using keyboard controls to operate the scroll arrows, the focus position is lost when you reach either the beginning or the end of the tab group. We ought to move focus to the last tab when scrolled to the end and to the first tab when scrolled to the start.
Alternatively, we could disable the start and end arrows and allow them to retain focus when the beginning or end of the tab group is reached, respectively.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Focus
If user is operating scroll arrows with keyboard, reaches end, and we hide the arrow
- If we move to last tab when scrolled to end etc, when
activation
isauto
, that will change which tab panel is shown. - If we move focus to active tab: If active tab isn't visible and we give it focus and scroll to it, the arrow will become enabled/visible again, which is confusing - the user will never see the hidden/disabled state, he'll just reach the end and be bounced to the beginning.
The user was in the middle of scrolling, so we know he wants to scroll. I think if the arrow that was focused on disappears, a reasonable focus point would be on the opposite arrow button.
If the arrow becomes disabled, I still think the reasonable behaviour would be to go to the opposite arrow, for the same reason.
Disabled vs hidden
Disabling and hiding are both reasonable. It happens to be that I need to hide it (and I'm fine with just having that functionality), but supporting both is also reasonable.
If we want to support both - that's a discussion of how that should be done, there are many options, IDK.
If we disable the tab, could that work with :disabled
? So a shoelace user could do something like
sl-tab-group::part(scroll-button):disabled {
visibility:hidden;
}
But IDK how that works with accessibility. We'd need to provide a way to move focus to somewhere reasonable. Or, shoelace could have explicit support for both options.
Thoughts?
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.
Great points, @yringler. Hiding the arrow and moving focus to the opposite scroll arrow seems like the best approach.
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.
Since the tabs are already accessible via keyboard (using arrow keys + home/end), I don't think the left/right scroll controls should be tabbable. My reasoning is very similar to the clear button in <input type="search">
, which Scott O'Hara explains:
The clear button is purposefully not in the tabbing order of the web page. Keyboard users can use the Delete or Backspace keys (or first select all the text field's text and then use these keys) to delete the contents of the text field.
Important: if the clear button was part of the web page's tabbing order, every text field a user had entered data into would then require that they tab past the clear button that they have no use for. This clear button being useless to them if they had filled out the field with the data they intended. Adding clear buttons to the web page's tabbing order is not recommended for this reason, as it has the potential to require two keyboard focus stops for every text field on the page that has a clear button.
So this functionality helps mouse and touch users, but making the scroll buttons tabbable hinders the experience for keyboard users who can simply press Left or Right to change tabs. Now they'll have to press tab two more times to get through the Tab Group.
Additionally, I'm not sure how screen reader users will interpret these buttons since they won't even be aware that tabs are visually hidden and require scrolling, so it seems to hurt their experience a bit too. (Note that setting tabindex="-1"
on the scroll buttons will prevent tabbing, but it won't stop screen reader users from being able to discover them using a virtual cursor.)
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.
Very interesting. In accessibility scenarios, the arrow buttons are at best redundant and at worst a problem. So can we just mark them as aria-hidden="true"
? In O'Hara's example, the clear button is technically useful - it's an easier way to clear. But here, the only purpose of the buttons is to scroll, and the keyboard user already has a direct way to do that.
That would also mean that, in terms of accessibility, shoelace would be agnostic to hiding the buttons vs disabling them; in either case, they are ignored by screen readers etc.
I had written about auto vs manual activation - looking at the w3 docs (in context of tab group here), there's a particular context in which each is appropriate. We should consider adding that guideline to the shoelace tab group docs.
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.
But here, the only purpose of the buttons is to scroll, and the keyboard user already has a direct way to do that.
That would also mean that, in terms of accessibility, shoelace would be agnostic to hiding the buttons vs disabling them; in either case, they are ignored by screen readers etc.
I'd be fine with this. What do you think, @lindsaym-fa?
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.
Yes! Removing the scroll arrows from the tab index completely resolves my initial concern. Better yet because curious keyboard users don't currently get any reward from using the scroll arrows — tabbing back to the tab group resets the scroll position, so any interactions with the scroll arrows are wasted.
We could offer the choice to either hide or disable the arrows, but in this case now where the arrows only benefit users with pointing devices, I'd venture that hiding the arrows offers better UX. Discerning disabled states can be tricky.
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.
Great! I updated it to prevent the scroll button arrows from receiving focus when using the keyboard to scroll.
Also, now the default behavior is to hide the scroll button when they're not clickable. I also added a fixed-scroll-controls
attribute option in case a user wants to keep the buttons visible.
Thank for the quick response I'll hold off on making it the default behavior until we have further feedback. |
…om/sammyl720/shoelace into auto-hide-tab-group-scroll-buttons
…ns__prevent-button-focus Prevent tab-group scroll buttons from being focusable
…om/sammyl720/shoelace into auto-hide-tab-group-scroll-buttons
…ns__default-behavior Make auto hiding scroll buttons the default behavior
Hi @claviska I've made the changes you suggested. Could you check it out again and let me know if there's anything else to improve? Thanks |
Add ability to automatically hide tab-group scroll control when there are no longer any tabs to show