From e1ef097bc4c4a1e888de028f40b6f2bb5ea2d7b8 Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Fri, 19 Jul 2024 13:55:26 +0530 Subject: [PATCH] fix(tabs): prevent vertical auto scroll (#4613) * chore(tabs): updated tabs overflow to fix scroll into view for horizontal scroll * chore(tabs): updated golden image cache --------- Co-authored-by: Rajdeep Chandra Co-authored-by: Rajdeep Chandra --- .circleci/config.yml | 2 +- packages/tabs/src/Tabs.ts | 74 ++++- .../tabs/stories/tabs-overflow.stories.ts | 23 +- packages/tabs/test/tabs-overflow.test.ts | 254 +++++++++++------- 4 files changed, 254 insertions(+), 99 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index ca59b5ff4c..a23ad2e520 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,7 +10,7 @@ executors: parameters: current_golden_images_hash: type: string - default: 019c0496b4ab51e1da329cae280244c60500cb86 + default: 1dfeeaaaeb1eacac0bc3ef4a0aa30a0414d86025 wireit_cache_name: type: string default: wireit diff --git a/packages/tabs/src/Tabs.ts b/packages/tabs/src/Tabs.ts index fb51a8cb61..24b75ec21a 100644 --- a/packages/tabs/src/Tabs.ts +++ b/packages/tabs/src/Tabs.ts @@ -66,6 +66,38 @@ export const ScaledIndicator = { }, }; +/** + * Given that the scroll needs to be on the right side of the viewport. + * Returns the coordonate x it needs to scroll so that the tab with given index is visible. + */ +export function calculateScrollTargetForRightSide( + index: number, + direction: 'rtl' | 'ltr', + tabs: Tab[], + container: HTMLDivElement +): number { + const nextIndex = index + (direction === 'rtl' ? -1 : 1); + const nextTab = tabs[nextIndex]; + const viewportEnd = container.scrollLeft + container.offsetWidth; + return nextTab ? nextTab.offsetLeft - container.offsetWidth : viewportEnd; +} + +/** + * Given that the scroll needs to be on the left side of the viewport. + * Returns the coordonate x it needs to scroll so that the tab with given index is visible. + */ +export function calculateScrollTargetForLeftSide( + index: number, + direction: 'rtl' | 'ltr', + tabs: Tab[], + container: HTMLDivElement +): number { + const prevIndex = index + (direction === 'rtl' ? 1 : -1); + const prevTab = tabs[prevIndex]; + const leftmostElement = direction === 'rtl' ? -container.offsetWidth : 0; + return prevTab ? prevTab.offsetLeft + prevTab.offsetWidth : leftmostElement; +} + /** * @element sp-tabs * @@ -245,16 +277,54 @@ export class Tabs extends SizedMixin(Focusable, { noDefaultSize: true }) { return complete; } + private getNecessaryAutoScroll(index: number): number { + const selectedTab = this.tabs[index]; + const selectionEnd = selectedTab.offsetLeft + selectedTab.offsetWidth; + const viewportEnd = this.tabList.scrollLeft + this.tabList.offsetWidth; + const selectionStart = selectedTab.offsetLeft; + const viewportStart = this.tabList.scrollLeft; + + if (selectionEnd > viewportEnd) { + // Selection is on the right side, not visible. + return calculateScrollTargetForRightSide( + index, + this.dir, + this.tabs, + this.tabList + ); + } else if (selectionStart < viewportStart) { + // Selection is on the left side, not visible. + return calculateScrollTargetForLeftSide( + index, + this.dir, + this.tabs, + this.tabList + ); + } + + return -1; + } + public async scrollToSelection(): Promise { if (!this.enableTabsScroll || !this.selected) { return; } await this.updateComplete; - const selectedTab = this.tabs.find( + + const selectedIndex = this.tabs.findIndex( (tab) => tab.value === this.selected ); - selectedTab?.scrollIntoView(); + + if (selectedIndex !== -1 && this.tabList) { + // We have a selection, calculate the scroll needed to bring it into view + const scrollTarget = this.getNecessaryAutoScroll(selectedIndex); + + // scrollTarget = -1 means it is already into view. + if (scrollTarget !== -1) { + this.tabList.scrollTo({ left: scrollTarget }); + } + } } protected override updated( diff --git a/packages/tabs/stories/tabs-overflow.stories.ts b/packages/tabs/stories/tabs-overflow.stories.ts index 37391bd875..bdc64a7659 100644 --- a/packages/tabs/stories/tabs-overflow.stories.ts +++ b/packages/tabs/stories/tabs-overflow.stories.ts @@ -9,7 +9,7 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -import { TemplateResult } from '@spectrum-web-components/base'; +import { html, TemplateResult } from '@spectrum-web-components/base'; import { OverflowProperties, renderTabsOverflowExample } from './index.js'; export default { @@ -30,3 +30,24 @@ export const autoscroll = (args: OverflowProperties): TemplateResult => { autoscroll.args = { selected: 15, }; + +// https://github.com/adobe/spectrum-web-components/issues/4590 +export const autoscrollOnlyHorizontally = ( + args: OverflowProperties +): TemplateResult => { + return html` + +
+
There are some tabs down here!
+ ${renderTabsOverflowExample(args)} +
+ `; +}; +autoscrollOnlyHorizontally.args = { + selected: 15, +}; diff --git a/packages/tabs/test/tabs-overflow.test.ts b/packages/tabs/test/tabs-overflow.test.ts index 80c2ac67b6..6547f2e2ea 100644 --- a/packages/tabs/test/tabs-overflow.test.ts +++ b/packages/tabs/test/tabs-overflow.test.ts @@ -16,7 +16,13 @@ import '@spectrum-web-components/tabs/sp-tab.js'; import '@spectrum-web-components/tabs/sp-tabs.js'; import '@spectrum-web-components/tabs/sp-tab-panel.js'; import '@spectrum-web-components/tabs/sp-tabs-overflow.js'; -import { Tab, Tabs, TabsOverflow } from '@spectrum-web-components/tabs'; +import { + calculateScrollTargetForLeftSide, + calculateScrollTargetForRightSide, + Tab, + Tabs, + TabsOverflow, +} from '@spectrum-web-components/tabs'; import { ActionButton } from '@spectrum-web-components/action-button'; import { elementUpdated, expect, fixture } from '@open-wc/testing'; @@ -33,7 +39,6 @@ type OverflowProperties = { size: ElementSize; includeTabPanel: boolean; selected?: number; - autoscroll?: boolean; labelPrev?: string; labelNext?: string; }; @@ -43,62 +48,54 @@ const renderTabsOverflow = async ({ size, includeTabPanel, selected = 1, - autoscroll = false, }: OverflowProperties): Promise => { - const tabsContainer = await fixture( - html` -
- - - ${repeat( - new Array(count), - (item) => item, - (_item, index) => - html` - - ` - )} - ${includeTabPanel - ? html` - ${repeat( - new Array(count), - (item) => item, - (_item, index) => - html` - - Content for Tab Item - ${index + 1} - - ` - )} - ` - : nothing} - - -
- ` - ); + const tabsContainer = await fixture(html` +
+ + + ${repeat( + new Array(count), + (item) => item, + (_item, index) => html` + + ` + )} + ${includeTabPanel + ? html` + ${repeat( + new Array(count), + (item) => item, + (_item, index) => html` + + Content for Tab Item ${index + 1} + + ` + )} + ` + : nothing} + + +
+ `); await elementUpdated(tabsContainer); return tabsContainer; }; describe('TabsOverflow', () => { it('loads default tabs-overflow accessibly', async () => { - const el = await fixture( - html` - - - - - Tab Content 1 - Tab Content 2 - - - ` - ); + const el = await fixture(html` + + + + + Tab Content 1 + Tab Content 2 + + + `); await elementUpdated(el); @@ -173,13 +170,11 @@ describe('TabsOverflow', () => { }); it('should fail properly if slot is not sp-tabs', async () => { - const el = await fixture( - html` - -
Some div
- - ` - ); + const el = await fixture(html` + +
Some div
+ + `); await elementUpdated(el); const slot = el.shadowRoot.querySelector('slot'); @@ -200,20 +195,42 @@ describe('TabsOverflow', () => { const tabsEl = el.querySelector('sp-tabs') as Tabs; // Grab the coordonates of the selected tab. - const selectedTab = tabsEl.querySelector( + let selectedTab = tabsEl.querySelector( `[role="tab"][value="10"]` ) as Tab; expect(selectedTab).to.exist; - const selectedTabPosition = selectedTab.getBoundingClientRect(); + let selectedTabPosition = selectedTab.getBoundingClientRect(); // Selected tab is in the viewport, offset left is greater than 0 and less than the width of the tabs. expect(selectedTabPosition.left).to.be.greaterThan(0); - expect(selectedTabPosition.left).to.be.lessThan(tabsEl.clientWidth); + expect(selectedTabPosition.left).to.be.lessThan(tabsEl.offsetWidth); // First tab is not in the viewport anymore, its offset left is less than 0. const firstTab = tabsEl.querySelector(`[role="tab"][value="1"]`) as Tab; const firstTabPosition = firstTab.getBoundingClientRect(); expect(firstTabPosition.left).to.be.lessThan(0); + + // Make the component automatically scroll left by selecting the first tab. + tabsEl.selected = '1'; + await elementUpdated(tabsEl); + + selectedTab = tabsEl.querySelector(`[role="tab"][value="1"]`) as Tab; + expect(selectedTab).to.exist; + selectedTabPosition = selectedTab.getBoundingClientRect(); + + // First tab is in the viewport, offset left is greater than 0 and less than the width of the tabs. + expect(selectedTabPosition.left).to.be.greaterThan(0); + expect(selectedTabPosition.left).to.be.lessThan(tabsEl.offsetWidth); + + // Tab nr. 10 is not in the viewport anymore. + const previousSelection = tabsEl.querySelector( + `[role="tab"][value="10"]` + ) as Tab; + const previousSelectionPosition = + previousSelection.getBoundingClientRect(); + expect(previousSelectionPosition.left).to.be.greaterThan( + tabsEl.offsetWidth + ); }); it('prev and next buttons have default labels', async () => { @@ -243,40 +260,36 @@ describe('TabsOverflow', () => { }); it('prev and next buttons labels overwritten via attributes', async () => { - const tabsContainer = await fixture( - html` -
- - - ${repeat( - new Array(20), - (item) => item, - (_item, index) => - html` - - ` - )} - ${repeat( - new Array(20), - (item) => item, - (_item, index) => - html` - - Content for Tab Item ${index + 1} - - ` - )} - - -
- ` - ); + const tabsContainer = await fixture(html` +
+ + + ${repeat( + new Array(20), + (item) => item, + (_item, index) => html` + + ` + )} + ${repeat( + new Array(20), + (item) => item, + (_item, index) => html` + + Content for Tab Item ${index + 1} + + ` + )} + + +
+ `); await elementUpdated(tabsContainer); const el = tabsContainer; @@ -298,3 +311,54 @@ describe('TabsOverflow', () => { ); }); }); + +describe('calculateScrollTargetForRightSide', () => { + const container = { offsetWidth: 100, scrollLeft: 0 } as HTMLDivElement; + const tabs = [ + { offsetLeft: 0, offsetWidth: 100 }, // currently selected tab + { offsetLeft: 100, offsetWidth: 100 }, + { offsetLeft: 200, offsetWidth: 100 }, + ] as Tab[]; + + it('correctly aligns tab on the right side of the viewport', () => { + // Where do I need to scroll on the x axis to get the tab at index 2 to be visible? + expect( + calculateScrollTargetForRightSide(2, 'ltr', tabs, container) + ).to.equal(100); // You need to scroll 100px more + + // Repeat for RTL + expect( + calculateScrollTargetForRightSide(2, 'rtl', tabs, container) + ).to.equal(0); // You need to scroll at the begining of the scrollable area + }); +}); + +describe('calculateScrollTargetForLeftSide', () => { + const container = { offsetWidth: 100, scrollLeft: 200 } as HTMLDivElement; + const tabs = [ + { offsetLeft: -200, offsetWidth: 100 }, + { offsetLeft: -100, offsetWidth: 100 }, + { offsetLeft: 0, offsetWidth: 100 }, // currently selected tab + ] as Tab[]; + + it('correctly aligns tab on the left side of the viewport', () => { + // Where do I need to scroll on the x axis to get the tab at index 1 to be visible? + expect( + calculateScrollTargetForLeftSide(1, 'ltr', tabs, container) + ).to.equal(-100); // you need to scroll back -100px + + // Where do I need to scroll on the x axis to get the first tab to be visible? + expect( + calculateScrollTargetForLeftSide(0, 'ltr', tabs, container) + ).to.equal(0); // you need to scroll to the begining of the scrollable area + + // Repeat for RTL + expect( + calculateScrollTargetForLeftSide(1, 'rtl', tabs, container) + ).to.equal(100); + + expect( + calculateScrollTargetForLeftSide(0, 'rtl', tabs, container) + ).to.equal(0); + }); +});