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

Add ability to automatically hide tab-group scroll control #2128

Merged
merged 20 commits into from
Sep 18, 2024
Merged

Add ability to automatically hide tab-group scroll control #2128

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
eadb077
Add ability to automatically hide tab-group scroll control when there…
Jul 31, 2024
d8941ce
code review updates
Jul 31, 2024
31a2032
update and document how scroll buttons are hidden
Jul 31, 2024
a29eabe
AUTO-HIDE: Simplify
yringler Jul 31, 2024
d603b37
AUTO-HIDE: extract to constant
yringler Jul 31, 2024
91db30e
update changelog
Jul 31, 2024
05ad296
include pr number in changelog update
Jul 31, 2024
e23553e
add line
Jul 31, 2024
007b022
apply suggested changes
Aug 1, 2024
e95ad56
Prevent tab-group scroll buttons from being focusable
Aug 5, 2024
d3e0db8
Merge branch 'next' into auto-hide-tab-group-scroll-buttons
claviska Aug 5, 2024
4072e6e
prettier fix
Aug 5, 2024
07521ce
Merge branch 'auto-hide-tab-group-scroll-buttons' of https://github.c…
Aug 5, 2024
0dbef22
Merge pull request #4 from sammyl720/auto-hide-tab-group-scroll-butto…
sammyl720 Aug 5, 2024
dd1d34d
Set default for 'auto-hide-scroll-buttons' to true
Aug 5, 2024
f58fc70
Merge branch 'auto-hide-tab-group-scroll-buttons' of https://github.c…
Aug 5, 2024
78c6148
Make auto hiding scroll buttons the default behavior
Aug 5, 2024
c2a525c
Update changelog
Aug 5, 2024
bc02e71
update changelog
Aug 5, 2024
1a7e224
Merge pull request #5 from sammyl720/auto-hide-tab-group-scroll-butto…
sammyl720 Aug 6, 2024
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
142 changes: 142 additions & 0 deletions docs/pages/components/tab-group.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,148 @@ const App = () => (
);
```

### Auto hide scroll controls

When tabs are scrolled all the way to one side, the scroll button on that side can't be clicked. Add the `auto-hide-scroll-buttons` attribute to the tab group to hide the effected button in that case.

```html:preview
<sl-tab-group auto-hide-scroll-buttons>
<sl-tab slot="nav" panel="tab-1">Tab 1</sl-tab>
<sl-tab slot="nav" panel="tab-2">Tab 2</sl-tab>
<sl-tab slot="nav" panel="tab-3">Tab 3</sl-tab>
<sl-tab slot="nav" panel="tab-4">Tab 4</sl-tab>
<sl-tab slot="nav" panel="tab-5">Tab 5</sl-tab>
<sl-tab slot="nav" panel="tab-6">Tab 6</sl-tab>
<sl-tab slot="nav" panel="tab-7">Tab 7</sl-tab>
<sl-tab slot="nav" panel="tab-8">Tab 8</sl-tab>
<sl-tab slot="nav" panel="tab-9">Tab 9</sl-tab>
<sl-tab slot="nav" panel="tab-10">Tab 10</sl-tab>
<sl-tab slot="nav" panel="tab-11">Tab 11</sl-tab>
<sl-tab slot="nav" panel="tab-12">Tab 12</sl-tab>
<sl-tab slot="nav" panel="tab-13">Tab 13</sl-tab>
<sl-tab slot="nav" panel="tab-14">Tab 14</sl-tab>
<sl-tab slot="nav" panel="tab-15">Tab 15</sl-tab>
<sl-tab slot="nav" panel="tab-16">Tab 16</sl-tab>
<sl-tab slot="nav" panel="tab-17">Tab 17</sl-tab>
<sl-tab slot="nav" panel="tab-18">Tab 18</sl-tab>
<sl-tab slot="nav" panel="tab-19">Tab 19</sl-tab>
<sl-tab slot="nav" panel="tab-20">Tab 20</sl-tab>

<sl-tab-panel name="tab-1">Tab panel 1</sl-tab-panel>
<sl-tab-panel name="tab-2">Tab panel 2</sl-tab-panel>
<sl-tab-panel name="tab-3">Tab panel 3</sl-tab-panel>
<sl-tab-panel name="tab-4">Tab panel 4</sl-tab-panel>
<sl-tab-panel name="tab-5">Tab panel 5</sl-tab-panel>
<sl-tab-panel name="tab-6">Tab panel 6</sl-tab-panel>
<sl-tab-panel name="tab-7">Tab panel 7</sl-tab-panel>
<sl-tab-panel name="tab-8">Tab panel 8</sl-tab-panel>
<sl-tab-panel name="tab-9">Tab panel 9</sl-tab-panel>
<sl-tab-panel name="tab-10">Tab panel 10</sl-tab-panel>
<sl-tab-panel name="tab-11">Tab panel 11</sl-tab-panel>
<sl-tab-panel name="tab-12">Tab panel 12</sl-tab-panel>
<sl-tab-panel name="tab-13">Tab panel 13</sl-tab-panel>
<sl-tab-panel name="tab-14">Tab panel 14</sl-tab-panel>
<sl-tab-panel name="tab-15">Tab panel 15</sl-tab-panel>
<sl-tab-panel name="tab-16">Tab panel 16</sl-tab-panel>
<sl-tab-panel name="tab-17">Tab panel 17</sl-tab-panel>
<sl-tab-panel name="tab-18">Tab panel 18</sl-tab-panel>
<sl-tab-panel name="tab-19">Tab panel 19</sl-tab-panel>
<sl-tab-panel name="tab-20">Tab panel 20</sl-tab-panel>
</sl-tab-group>
```

```jsx:react
import SlTab from '@shoelace-style/shoelace/dist/react/tab';
import SlTabGroup from '@shoelace-style/shoelace/dist/react/tab-group';
import SlTabPanel from '@shoelace-style/shoelace/dist/react/tab-panel';

const App = () => (
<SlTabGroup auto-hide-scroll-buttons>
<SlTab slot="nav" panel="tab-1">
Tab 1
</SlTab>
<SlTab slot="nav" panel="tab-2">
Tab 2
</SlTab>
<SlTab slot="nav" panel="tab-3">
Tab 3
</SlTab>
<SlTab slot="nav" panel="tab-4">
Tab 4
</SlTab>
<SlTab slot="nav" panel="tab-5">
Tab 5
</SlTab>
<SlTab slot="nav" panel="tab-6">
Tab 6
</SlTab>
<SlTab slot="nav" panel="tab-7">
Tab 7
</SlTab>
<SlTab slot="nav" panel="tab-8">
Tab 8
</SlTab>
<SlTab slot="nav" panel="tab-9">
Tab 9
</SlTab>
<SlTab slot="nav" panel="tab-10">
Tab 10
</SlTab>
<SlTab slot="nav" panel="tab-11">
Tab 11
</SlTab>
<SlTab slot="nav" panel="tab-12">
Tab 12
</SlTab>
<SlTab slot="nav" panel="tab-13">
Tab 13
</SlTab>
<SlTab slot="nav" panel="tab-14">
Tab 14
</SlTab>
<SlTab slot="nav" panel="tab-15">
Tab 15
</SlTab>
<SlTab slot="nav" panel="tab-16">
Tab 16
</SlTab>
<SlTab slot="nav" panel="tab-17">
Tab 17
</SlTab>
<SlTab slot="nav" panel="tab-18">
Tab 18
</SlTab>
<SlTab slot="nav" panel="tab-19">
Tab 19
</SlTab>
<SlTab slot="nav" panel="tab-20">
Tab 20
</SlTab>

<SlTabPanel name="tab-1">Tab panel 1</SlTabPanel>
<SlTabPanel name="tab-2">Tab panel 2</SlTabPanel>
<SlTabPanel name="tab-3">Tab panel 3</SlTabPanel>
<SlTabPanel name="tab-4">Tab panel 4</SlTabPanel>
<SlTabPanel name="tab-5">Tab panel 5</SlTabPanel>
<SlTabPanel name="tab-6">Tab panel 6</SlTabPanel>
<SlTabPanel name="tab-7">Tab panel 7</SlTabPanel>
<SlTabPanel name="tab-8">Tab panel 8</SlTabPanel>
<SlTabPanel name="tab-9">Tab panel 9</SlTabPanel>
<SlTabPanel name="tab-10">Tab panel 10</SlTabPanel>
<SlTabPanel name="tab-11">Tab panel 11</SlTabPanel>
<SlTabPanel name="tab-12">Tab panel 12</SlTabPanel>
<SlTabPanel name="tab-13">Tab panel 13</SlTabPanel>
<SlTabPanel name="tab-14">Tab panel 14</SlTabPanel>
<SlTabPanel name="tab-15">Tab panel 15</SlTabPanel>
<SlTabPanel name="tab-16">Tab panel 16</SlTabPanel>
<SlTabPanel name="tab-17">Tab panel 17</SlTabPanel>
<SlTabPanel name="tab-18">Tab panel 18</SlTabPanel>
<SlTabPanel name="tab-19">Tab panel 19</SlTabPanel>
<SlTabPanel name="tab-20">Tab panel 20</SlTabPanel>
</SlTabGroup>
);
```

### Manual Activation

When focused, keyboard users can press [[Left]] or [[Right]] to select the desired tab. By default, the corresponding tab panel will be shown immediately (automatic activation). You can change this behavior by setting `activation="manual"` which will require the user to press [[Space]] or [[Enter]] before showing the tab panel (manual activation).
Expand Down
4 changes: 4 additions & 0 deletions docs/pages/resources/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ Components with the <sl-badge variant="warning" pill>Experimental</sl-badge> bad

New versions of Shoelace are released as-needed and generally occur when a critical mass of changes have accumulated. At any time, you can see what's coming in the next release by visiting [next.shoelace.style](https://next.shoelace.style).

## Next

- Added ability to auto hide scroll buttons for `<sl-tab-group>` when scroll button is not clickable by adding the `auto-hide-scroll-buttons` attribute. [#2128]

## 2.16.0

- Added the Czech translation [#2084]
Expand Down
44 changes: 41 additions & 3 deletions src/components/tab-group/tab-group.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export default class SlTabGroup extends ShoelaceElement {

@state() private hasScrollControls = false;

@state() private shouldHideScrollStartButton = false;

@state() private shouldHideScrollEndButton = false;
claviska marked this conversation as resolved.
Show resolved Hide resolved

/** The placement of the tabs. */
@property() placement: 'top' | 'bottom' | 'start' | 'end' = 'top';

Expand All @@ -72,6 +76,9 @@ export default class SlTabGroup extends ShoelaceElement {
/** Disables the scroll arrows that appear when tabs overflow. */
@property({ attribute: 'no-scroll-controls', type: Boolean }) noScrollControls = false;

/** Hide scroll buttons when inactive. */
@property({ attribute: 'auto-hide-scroll-buttons', type: Boolean }) autoHideScrollButtons = false;
Copy link
Member

@claviska claviska Aug 1, 2024 •
edited
Loading

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.

Sign in to view

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 •
edited
Loading

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.


connectedCallback() {
const whenAllDefined = Promise.all([
customElements.whenDefined('sl-tab'),
Expand Down Expand Up @@ -365,6 +372,27 @@ export default class SlTabGroup extends ShoelaceElement {
return nextTab;
}

/**
* The reality of the browser means that we can't expect the scroll position to be exactly what we want it to be, so
* we add one pixel of wiggle room to our calculations.
*/
private scrollOffset = 1;

private updateScrollButtons() {
claviska marked this conversation as resolved.
Show resolved Hide resolved
if (this.hasScrollControls && this.autoHideScrollButtons) {
this.shouldHideScrollStartButton = this.scrollFromStart() <= this.scrollOffset;
this.shouldHideScrollEndButton = this.isScrolledToEnd();
}
}

private isScrolledToEnd() {
return (this.scrollFromStart() + this.nav.clientWidth) >= this.nav.scrollWidth - this.scrollOffset;
}

private scrollFromStart() {
return this.localize.dir() === 'rtl' ? -this.nav.scrollLeft : this.nav.scrollLeft;
}

@watch('noScrollControls', { waitUntilFirstUpdate: true })
updateScrollControls() {
if (this.noScrollControls) {
Expand All @@ -378,6 +406,8 @@ export default class SlTabGroup extends ShoelaceElement {
this.hasScrollControls =
['top', 'bottom'].includes(this.placement) && this.nav.scrollWidth > this.nav.clientWidth + 1;
}

this.updateScrollButtons();
}

@watch('placement', { waitUntilFirstUpdate: true })
Expand Down Expand Up @@ -425,7 +455,11 @@ export default class SlTabGroup extends ShoelaceElement {
<sl-icon-button
part="scroll-button scroll-button--start"
exportparts="base:scroll-button__base"
class="tab-group__scroll-button tab-group__scroll-button--start"
class=${classMap({
"tab-group__scroll-button": true,
"tab-group__scroll-button--start": true,
"tab-group__scroll-button--start--hidden": this.shouldHideScrollStartButton
})}
name=${isRtl ? 'chevron-right' : 'chevron-left'}
library="system"
label=${this.localize.term('scrollToStart')}
Expand All @@ -434,7 +468,7 @@ export default class SlTabGroup extends ShoelaceElement {
`
: ''}

<div class="tab-group__nav">
<div class="tab-group__nav" @scrollend=${this.updateScrollButtons}>
claviska marked this conversation as resolved.
Show resolved Hide resolved
<div part="tabs" class="tab-group__tabs" role="tablist">
<div part="active-tab-indicator" class="tab-group__indicator"></div>
<slot name="nav" @slotchange=${this.syncTabsAndPanels}></slot>
Expand All @@ -446,7 +480,11 @@ export default class SlTabGroup extends ShoelaceElement {
<sl-icon-button
part="scroll-button scroll-button--end"
exportparts="base:scroll-button__base"
class="tab-group__scroll-button tab-group__scroll-button--end"
class=${classMap({
"tab-group__scroll-button": true,
"tab-group__scroll-button--end": true,
"tab-group__scroll-button--end--hidden": this.shouldHideScrollEndButton
})}
name=${isRtl ? 'chevron-left' : 'chevron-right'}
library="system"
label=${this.localize.term('scrollToEnd')}
Expand Down
6 changes: 6 additions & 0 deletions src/components/tab-group/tab-group.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ export default css`
padding: 0 var(--sl-spacing-x-large);
}

.tab-group--has-scroll-controls .tab-group__scroll-button--start--hidden,
.tab-group--has-scroll-controls .tab-group__scroll-button--end--hidden {
opacity: 0;
pointer-events: none;
claviska marked this conversation as resolved.
Show resolved Hide resolved
}

.tab-group__body {
display: block;
overflow: auto;
Expand Down