Skip to content

Commit

Permalink
fix(ld-tabs): replace scroll into view with scroll to
Browse files Browse the repository at this point in the history 
Background: We want the tablist to scroll in a way that the selected tab is presented in the horizontal middle of the scroll area. We initially chose to use the scrollIntoView() method with the inline: 'center' option for this. However, this results in scrolling of elements wrapping the ld-tabs component as well. This change addresses the issue by replacing the usage of scrollIntoView() with the usage of the scrollTo() method.

Fixes #792
  • Loading branch information
@borisdiakur
borisdiakur committed Jul 3, 2023
1 parent a22686c commit 2219a07
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 64 deletions.
58 changes: 41 additions & 17 deletions src/liquid/components/ld-tabs/ld-tablist/ld-tablist.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,20 @@ export class LdTablist {

private updateScrollable() {
if (this.isFloating()) return
const scrollButtonsWidth =
2 * this.btnScrollLeftRef.getBoundingClientRect().width
const scrollContainerWidth =

// We must round all width values to prevent circular painting!
const scrollButtonsWidth = this.scrollable
? Math.round(2 * this.btnScrollLeftRef.getBoundingClientRect().width)
: 0
const scrollContainerWidth = Math.round(
this.slotContainerRef.getBoundingClientRect().width
const contentWidth = Array.from(this.el.children)
.map((child) => child.getBoundingClientRect().width)
.reduce((a, b) => a + b)
this.scrollable =
scrollContainerWidth + (this.scrollable ? scrollButtonsWidth : 0) <
contentWidth
)
const contentWidth = Math.round(
Array.from(this.el.children)
.map((child) => child.getBoundingClientRect().width)
.reduce((a, b) => a + b)
)
this.scrollable = scrollContainerWidth + scrollButtonsWidth < contentWidth
}

private updateScrollButtons() {
Expand All @@ -98,18 +102,14 @@ export class LdTablist {
})
}

private focusTab(prevLdTab: HTMLElement, dir: 'left' | 'right') {
private focusTab(prevLdTab: HTMLLdTabElement, dir: 'left' | 'right') {
const currentTab =
dir === 'left'
? prevLdTab.previousElementSibling
: prevLdTab.nextElementSibling
if (isInnerFocusable(currentTab)) {
currentTab.focusInner()
currentTab.scrollIntoView({
behavior: 'smooth',
block: 'nearest',
inline: 'center',
})
this.scrollTabIntoView(currentTab as HTMLLdTabElement)
this.selectedIsFocused = currentTab === this.selectedTab
}
}
Expand Down Expand Up @@ -177,6 +177,25 @@ export class LdTablist {
)
}

private scrollTabIntoView(tab: HTMLLdTabElement) {
if (!tab || !this.slotContainerRef) {
return
}
const scrollContainerWidth =
this.slotContainerRef.getBoundingClientRect().width
const scrollButtonWidth = this.scrollable
? this.btnScrollLeftRef.getBoundingClientRect().width
: 0
this.slotContainerRef.scrollTo({
left:
tab.offsetLeft +
tab.getBoundingClientRect().width / 2 -
scrollContainerWidth / 2 -
scrollButtonWidth,
behavior: 'smooth',
})
}

@Listen('ldtabselect')
handleTabSelect(ev) {
this.selectedIsFocused = true
Expand All @@ -185,6 +204,9 @@ export class LdTablist {

@Watch('selectedTab')
private updateSelectedTabIndicator() {
// Scroll tab into view.
this.scrollTabIntoView(this.selectedTab)

if (!this.selectedTabIndicatorRef) return

const indicatorStyle = this.selectedTabIndicatorRef.style
Expand All @@ -195,8 +217,10 @@ export class LdTablist {
}

const selectedTabBcr = this.selectedTab.getBoundingClientRect()
const parentBcr = this.selectedTab.parentElement.getBoundingClientRect()
const offsetLeft = selectedTabBcr.left - parentBcr.left
const scrollContainerBcr = this.slotContainerRef.getBoundingClientRect()
const scrollContainerScrollLeft = this.slotContainerRef.scrollLeft
const offsetLeft =
selectedTabBcr.left - scrollContainerBcr.left + scrollContainerScrollLeft
indicatorStyle.transform = `translateX(${offsetLeft - 8}px)`
indicatorStyle.width = `${selectedTabBcr.width}px`
indicatorStyle.opacity = '1'
Expand Down
9 changes: 2 additions & 7 deletions src/liquid/components/ld-tabs/ld-tabs.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,12 @@ export class LdTabs {

private idDescriber = `ld-tabs-${tabsCount++}`

private updateTabs(currentLdTab) {
private updateTabs() {
// TODO: fix Stencils DOM implementation for unit testing and replace
// this.el.querySelector('[selected]')?.removeAttribute('selected')
Array.from(this.el.querySelectorAll('ld-tab'))
.find((tab) => tab.hasAttribute('selected'))
?.removeAttribute('selected')
currentLdTab.scrollIntoView({
behavior: 'smooth',
block: 'nearest',
inline: 'center',
})
}

private updateTabPanels(tabId: string) {
Expand All @@ -57,7 +52,7 @@ export class LdTabs {
private handleLdtabselect = (ev: CustomEvent<undefined>) => {
ev.stopImmediatePropagation()
const currentLdTab = ev.target as HTMLLdTabElement
this.updateTabs(currentLdTab)
this.updateTabs()
this.updateTabPanels(currentLdTab.id)
this.ldtabchange.emit(currentLdTab.id)
}
Expand Down
83 changes: 43 additions & 40 deletions src/liquid/components/ld-tabs/test/ld-tabs.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,10 @@ describe('ld-tabs', () => {
const ldTabpanel2 = ldTabpanels[2]
expect(ldTabpanel2).toHaveAttribute('hidden')

ldTabItems[2].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[2], 'scrollIntoView')
const spyScrollTo = jest.spyOn(
ldTablist.shadowRoot.querySelector('.ld-tablist__scroll-container'),
'scrollTo'
)

tabBtn2.dispatchEvent(new Event('click'))
await page.waitForChanges()
Expand All @@ -126,7 +128,7 @@ describe('ld-tabs', () => {

expect(ldTabpanel2).not.toHaveAttribute('hidden')

expect(spyScrollIntoView).toHaveBeenCalled()
expect(spyScrollTo).toHaveBeenCalled()
})

it('changes tab via click with no preselected tab', async () => {
Expand Down Expand Up @@ -164,8 +166,10 @@ describe('ld-tabs', () => {
const ldTabpanel0 = ldTabpanels[0]
const ldTabpanel2 = ldTabpanels[2]

ldTabItems[2].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[2], 'scrollIntoView')
const spyScrollTo = jest.spyOn(
ldTablist.shadowRoot.querySelector('.ld-tablist__scroll-container'),
'scrollTo'
)

tabBtn2.dispatchEvent(new Event('click'))
await page.waitForChanges()
Expand All @@ -180,7 +184,7 @@ describe('ld-tabs', () => {

expect(ldTabpanel2).not.toHaveAttribute('hidden')

expect(spyScrollIntoView).toHaveBeenCalled()
expect(spyScrollTo).toHaveBeenCalled()
})

it('changes tab with no target tabpanel', async () => {
Expand Down Expand Up @@ -212,8 +216,10 @@ describe('ld-tabs', () => {
expect(tabBtn2.getAttribute('aria-selected')).toEqual(null)
expect(tabBtn2.getAttribute('tabindex')).toEqual('-1')

ldTabItems[2].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[2], 'scrollIntoView')
const spyScrollTo = jest.spyOn(
ldTablist.shadowRoot.querySelector('.ld-tablist__scroll-container'),
'scrollTo'
)

tabBtn2.dispatchEvent(new Event('click'))
await page.waitForChanges()
Expand All @@ -224,7 +230,7 @@ describe('ld-tabs', () => {
expect(tabBtn2.getAttribute('aria-selected')).toEqual('true')
expect(tabBtn2.getAttribute('tabindex')).toEqual(null)

expect(spyScrollIntoView).toHaveBeenCalled()
expect(spyScrollTo).toHaveBeenCalled()
})

it('does not change tab via click on disabled tab', async () => {
Expand Down Expand Up @@ -272,8 +278,7 @@ describe('ld-tabs', () => {
const ldTabpanel3 = ldTabpanels[3]
expect(ldTabpanel3).toHaveAttribute('hidden')

ldTabItems[3].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[3], 'scrollIntoView')
const spyScrollTo = jest.spyOn(ldTabItems[3], 'scrollIntoView')

tabBtn3.dispatchEvent(new Event('click'))
await page.waitForChanges()
Expand All @@ -288,7 +293,7 @@ describe('ld-tabs', () => {

expect(ldTabpanel3).toHaveAttribute('hidden')

expect(spyScrollIntoView).not.toHaveBeenCalled()
expect(spyScrollTo).not.toHaveBeenCalled()
})

it('does not change tab via click on already selected tab', async () => {
Expand Down Expand Up @@ -316,16 +321,15 @@ describe('ld-tabs', () => {
expect(tabBtn0.getAttribute('aria-selected')).toEqual('true')
expect(tabBtn0.getAttribute('tabindex')).toEqual(null)

ldTabItems[0].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[0], 'scrollIntoView')
const spyScrollTo = jest.spyOn(ldTabItems[0], 'scrollIntoView')

tabBtn0.dispatchEvent(new Event('click'))
await page.waitForChanges()

expect(tabBtn0.getAttribute('aria-selected')).toEqual('true')
expect(tabBtn0.getAttribute('tabindex')).toEqual(null)

expect(spyScrollIntoView).not.toHaveBeenCalled()
expect(spyScrollTo).not.toHaveBeenCalled()
})
})

Expand Down Expand Up @@ -390,14 +394,10 @@ describe('ld-tabs', () => {
const ldTabpanel3 = ldTabpanels[3]
expect(ldTabpanel3).toHaveAttribute('hidden')

ldTab0.scrollIntoView = jest.fn()
ldTab1.scrollIntoView = jest.fn()
ldTab2.scrollIntoView = jest.fn()
ldTab3.scrollIntoView = jest.fn()
const spyScrollIntoView0 = jest.spyOn(ldTab0, 'scrollIntoView')
const spyScrollIntoView1 = jest.spyOn(ldTab1, 'scrollIntoView')
const spyScrollIntoView2 = jest.spyOn(ldTab2, 'scrollIntoView')
const spyScrollIntoView3 = jest.spyOn(ldTab3, 'scrollIntoView')
const spyScrollTo = jest.spyOn(
ldTablist.shadowRoot.querySelector('.ld-tablist__scroll-container'),
'scrollTo'
)

tabBtn0.focus = jest.fn(focusManager.focus)
tabBtn1.focus = jest.fn(focusManager.focus)
Expand All @@ -409,7 +409,7 @@ describe('ld-tabs', () => {
)
await page.waitForChanges()

expect(spyScrollIntoView1).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(1)

expect(tabBtn0.getAttribute('aria-selected')).toEqual('true')
expect(ldTabpanel0).not.toHaveAttribute('hidden')
Expand All @@ -421,12 +421,12 @@ describe('ld-tabs', () => {
new KeyboardEvent('keydown', { key: 'ArrowRight', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView2).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(2)

tabBtn2.dispatchEvent(new Event('click'))
await page.waitForChanges()

expect(spyScrollIntoView2).toHaveBeenCalledTimes(2)
expect(spyScrollTo).toHaveBeenCalledTimes(3)

expect(tabBtn0.getAttribute('aria-selected')).toEqual(null)
expect(tabBtn0.getAttribute('tabindex')).toEqual('-1')
Expand All @@ -444,7 +444,7 @@ describe('ld-tabs', () => {
new KeyboardEvent('keydown', { key: 'ArrowRight', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView3).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(4)

expect(tabBtn2.getAttribute('aria-selected')).toEqual('true')
expect(tabBtn2.getAttribute('tabindex')).toEqual(null)
Expand All @@ -458,37 +458,37 @@ describe('ld-tabs', () => {
new KeyboardEvent('keydown', { key: 'ArrowRight', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView3).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(4)

ldTab3.dispatchEvent(
new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView2).toHaveBeenCalledTimes(3)
expect(spyScrollTo).toHaveBeenCalledTimes(5)

ldTab2.dispatchEvent(
new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView1).toHaveBeenCalledTimes(2)
expect(spyScrollTo).toHaveBeenCalledTimes(6)

ldTab1.dispatchEvent(
new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView0).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(7)

ldTab0.dispatchEvent(
new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView0).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(7)

ldTab0.dispatchEvent(
new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true })
)
await page.waitForChanges()
expect(spyScrollIntoView0).toHaveBeenCalledTimes(1)
expect(spyScrollTo).toHaveBeenCalledTimes(7)

ldTab0.dispatchEvent(
new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })
Expand Down Expand Up @@ -520,7 +520,6 @@ describe('ld-tabs', () => {
expect(ldTabItems.length).toEqual(2)

const ldTabBtn1 = ldTabItems[1]
ldTabBtn1.scrollIntoView = jest.fn()
const tabBtn1 = ldTabBtn1.shadowRoot.querySelector('button')
const handleLdtabchange = jest.fn()

Expand Down Expand Up @@ -561,8 +560,10 @@ describe('ld-tabs', () => {
expect(tabBtn2.getAttribute('aria-selected')).toEqual(null)
expect(tabBtn2.getAttribute('tabindex')).toEqual('-1')

ldTabItems[2].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[2], 'scrollIntoView')
const spyScrollTo = jest.spyOn(
ldTablist.shadowRoot.querySelector('.ld-tablist__scroll-container'),
'scrollTo'
)

await ldTabs.switchTab(2)
await page.waitForChanges()
Expand All @@ -573,7 +574,7 @@ describe('ld-tabs', () => {
expect(tabBtn2.getAttribute('aria-selected')).toEqual('true')
expect(tabBtn2.getAttribute('tabindex')).toEqual(null)

expect(spyScrollIntoView).toHaveBeenCalled()
expect(spyScrollTo).toHaveBeenCalled()
})

it('changes tab via method with preselected tab using id', async () => {
Expand Down Expand Up @@ -605,8 +606,10 @@ describe('ld-tabs', () => {
expect(tabBtn2.getAttribute('aria-selected')).toEqual(null)
expect(tabBtn2.getAttribute('tabindex')).toEqual('-1')

ldTabItems[2].scrollIntoView = jest.fn()
const spyScrollIntoView = jest.spyOn(ldTabItems[2], 'scrollIntoView')
const spyScrollTo = jest.spyOn(
ldTablist.shadowRoot.querySelector('.ld-tablist__scroll-container'),
'scrollTo'
)

await ldTabs.switchTab('ld-tabs-10-tab-2')
await page.waitForChanges()
Expand All @@ -617,7 +620,7 @@ describe('ld-tabs', () => {
expect(tabBtn2.getAttribute('aria-selected')).toEqual('true')
expect(tabBtn2.getAttribute('tabindex')).toEqual(null)

expect(spyScrollIntoView).toHaveBeenCalled()
expect(spyScrollTo).toHaveBeenCalled()
})

it('throws if no tab is found when switching using index', async () => {
Expand Down

1 comment on commit 2219a07

@vercel
Copy link

@vercel vercel bot commented on 2219a07 Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully deployed to the following URLs:

liquid – ./

liquid-git-main-uxsd.vercel.app
liquid-uxsd.vercel.app
liquid-oxygen.vercel.app

Please sign in to comment.