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.

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

Merged

Conversation

sammyl720
Copy link
Contributor

Add ability to automatically hide tab-group scroll control when there are no longer any tabs to show

Copy link

vercel bot commented Jul 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Aug 6, 2024 6:21pm

yringler

This comment was marked as resolved.

Copy link
Member

@claviska claviska left a 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!

src/components/tab-group/tab-group.styles.ts Outdated Show resolved Hide resolved
src/components/tab-group/tab-group.component.ts Outdated Show resolved Hide resolved
Comment on lines 79 to 80
/** Hide scroll buttons when inactive. */
@property({ attribute: 'auto-hide-scroll-buttons', type: Boolean }) autoHideScrollButtons = false;
Copy link
Member

@claviska claviska Aug 1, 2024

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?

Copy link
Collaborator

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.

Copy link
Contributor

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 is auto, 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?

Copy link
Collaborator

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.

Copy link
Member

@claviska claviska Aug 5, 2024

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

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

src/components/tab-group/tab-group.component.ts Outdated Show resolved Hide resolved
@sammyl720
Copy link
Contributor Author

Thank for the quick response

I'll hold off on making it the default behavior until we have further feedback.
For now, I applied the rest code review suggestions / improvements ,

…ns__prevent-button-focus

Prevent tab-group scroll buttons from being focusable
@sammyl720
Copy link
Contributor Author

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

@sammyl720 sammyl720 requested a review from claviska August 7, 2024 15:32
@claviska claviska merged commit 65126e8 into shoelace-style:next Sep 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants