Skip to content

Commit

Permalink
fix: scroll to the next item on button click (#5925) (#5989)
Browse files Browse the repository at this point in the history 
* fix: scroll to the next not fully visible item on button click

* fix: revert scrollToItem change and use scroll instead

* refactor: revert refactoring

* fix: remove full visibility check and handle rounded zero scroll

* fix: fix tests and add partial visibility check flag

* fix: update full visibility check boundary

* fix: expand workaround case and refactor

* test: add test for scroll buttons getting stuck

* test: refactor the tests to not check visibility for buttons and items

* fix: delete previously removed argument

* fix: increase test approximation deltas

* fix tests to handle sub-pixel scrolling values

---------

Co-authored-by: Ugur Saglam <[email protected]>
Co-authored-by: Sascha Ißbrücker <[email protected]>
  • Loading branch information
@vaadin-bot @ugur-vaadin @sissbruecker
3 people authored Jun 19, 2023
1 parent c3a3d90 commit 52a1201
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 4 deletions.
82 changes: 80 additions & 2 deletions packages/tabs/src/vaadin-tabs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,90 @@ class Tabs extends ResizeMixin(ElementMixin(ListMixin(ThemableMixin(PolymerEleme

/** @private */
_scrollForward() {
this._scroll(-this.__direction * this._scrollOffset);
// Calculations here are performed in order to optimize the loop that checks item visibility.
const forwardButtonVisibleWidth = this._getNavigationButtonVisibleWidth('forward-button');
const backButtonVisibleWidth = this._getNavigationButtonVisibleWidth('back-button');
const scrollerRect = this._scrollerElement.getBoundingClientRect();
const itemToScrollTo = [...this.items]
.reverse()
.find((item) => this._isItemVisible(item, forwardButtonVisibleWidth, backButtonVisibleWidth, scrollerRect));
const itemRect = itemToScrollTo.getBoundingClientRect();
// This hard-coded number accounts for the width of the mask that covers a part of the visible items.
// A CSS variable can be introduced to get rid of this value.
const overflowIndicatorCompensation = 20;
const totalCompensation =
overflowIndicatorCompensation + this.shadowRoot.querySelector('[part="back-button"]').clientWidth;
let scrollOffset;
if (this.__isRTL) {
const scrollerRightEdge = scrollerRect.right - totalCompensation;
scrollOffset = itemRect.right - scrollerRightEdge;
} else {
const scrollerLeftEdge = scrollerRect.left + totalCompensation;
scrollOffset = itemRect.left - scrollerLeftEdge;
}
// It is possible that a scroll offset is calculated to be between 0 and 1. In this case, this offset
// can be rounded down to zero, rendering the button useless. It is also possible that the offset is
// calculated such that it results in scrolling backwards for a wide tab or edge cases. This is a
// workaround for such cases.
if (-this.__direction * scrollOffset < 1) {
scrollOffset = -this.__direction * (this._scrollOffset - totalCompensation);
}
this._scroll(scrollOffset);
}

/** @private */
_scrollBack() {
this._scroll(this.__direction * this._scrollOffset);
// Calculations here are performed in order to optimize the loop that checks item visibility.
const forwardButtonVisibleWidth = this._getNavigationButtonVisibleWidth('forward-button');
const backButtonVisibleWidth = this._getNavigationButtonVisibleWidth('back-button');
const scrollerRect = this._scrollerElement.getBoundingClientRect();
const itemToScrollTo = this.items.find((item) =>
this._isItemVisible(item, forwardButtonVisibleWidth, backButtonVisibleWidth, scrollerRect),
);
const itemRect = itemToScrollTo.getBoundingClientRect();
// This hard-coded number accounts for the width of the mask that covers a part of the visible items.
// A CSS variable can be introduced to get rid of this value.
const overflowIndicatorCompensation = 20;
const totalCompensation =
overflowIndicatorCompensation + this.shadowRoot.querySelector('[part="forward-button"]').clientWidth;
let scrollOffset;
if (this.__isRTL) {
const scrollerLeftEdge = scrollerRect.left + totalCompensation;
scrollOffset = itemRect.left - scrollerLeftEdge;
} else {
const scrollerRightEdge = scrollerRect.right - totalCompensation;
scrollOffset = itemRect.right - scrollerRightEdge;
}
// It is possible that a scroll offset is calculated to be between 0 and 1. In this case, this offset
// can be rounded down to zero, rendering the button useless. It is also possible that the offset is
// calculated such that it results in scrolling forward for a wide tab or edge cases. This is a
// workaround for such cases.
if (this.__direction * scrollOffset < 1) {
scrollOffset = this.__direction * (this._scrollOffset - totalCompensation);
}
this._scroll(scrollOffset);
}

/** @private */
_isItemVisible(item, forwardButtonVisibleWidth, backButtonVisibleWidth, scrollerRect) {
if (this._vertical) {
throw new Error('Visibility check is only supported for horizontal tabs.');
}
const buttonOnTheRightWidth = this.__isRTL ? backButtonVisibleWidth : forwardButtonVisibleWidth;
const buttonOnTheLeftWidth = this.__isRTL ? forwardButtonVisibleWidth : backButtonVisibleWidth;
const scrollerRightEdge = scrollerRect.right - buttonOnTheRightWidth;
const scrollerLeftEdge = scrollerRect.left + buttonOnTheLeftWidth;
const itemRect = item.getBoundingClientRect();
return scrollerRightEdge > Math.floor(itemRect.left) && scrollerLeftEdge < Math.ceil(itemRect.right);
}

/** @private */
_getNavigationButtonVisibleWidth(buttonPartName) {
const navigationButton = this.shadowRoot.querySelector(`[part="${buttonPartName}"]`);
if (window.getComputedStyle(navigationButton).opacity === '0') {
return 0;
}
return navigationButton.clientWidth;
}

/** @private */
Expand Down
85 changes: 83 additions & 2 deletions packages/tabs/test/scroll.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from '@esm-bundle/chai';
import { arrowDown, arrowLeft, arrowRight, arrowUp, fixtureSync } from '@vaadin/testing-helpers';
import { arrowDown, arrowLeft, arrowRight, arrowUp, fixtureSync, nextFrame } from '@vaadin/testing-helpers';
import '../vaadin-tabs.js';

describe('scrollable tabs', () => {
Expand All @@ -17,6 +17,13 @@ describe('scrollable tabs', () => {
<vaadin-tab>Baz1</vaadin-tab>
<vaadin-tab>Foo2</vaadin-tab>
<vaadin-tab>Bar2</vaadin-tab>
<vaadin-tab>Baz2</vaadin-tab>
<vaadin-tab>Foo3</vaadin-tab>
<vaadin-tab>Bar3</vaadin-tab>
<vaadin-tab>Baz3</vaadin-tab>
<vaadin-tab>Foo4</vaadin-tab>
<vaadin-tab>Bar4</vaadin-tab>
<vaadin-tab>Baz4</vaadin-tab>
</vaadin-tabs>
`);
tabs._observer.flush();
Expand Down Expand Up @@ -53,9 +60,10 @@ describe('scrollable tabs', () => {
});

it('should scroll forward when arrow button is clicked', () => {
const initialScrollLeft = scroller.scrollLeft;
const btn = tabs.shadowRoot.querySelector('[part="forward-button"]');
btn.click();
expect(scroller.scrollLeft).to.be.closeTo(scroller.scrollWidth - scroller.offsetWidth, 1);
expect(scroller.scrollLeft).to.be.greaterThan(initialScrollLeft);
});

it('should scroll back when arrow button is clicked', () => {
Expand All @@ -64,6 +72,79 @@ describe('scrollable tabs', () => {
btn.click();
expect(scroller.scrollLeft).to.be.equal(0);
});

['ltr', 'rtl'].forEach((dir) => {
describe(dir, () => {
beforeEach(async () => {
tabs.setAttribute('dir', dir);
await nextFrame();
});

it('should have displayed all the items fully when scrolled forward to the end via button', async () => {
const forwardButton = tabs.shadowRoot.querySelector('[part="forward-button"]');

expect(-tabs.__direction * tabs._scrollerElement.scrollLeft).to.equal(0);

forwardButton.click();
expect(-tabs.__direction * tabs._scrollerElement.scrollLeft).to.be.approximately(310, 10);

forwardButton.click();
expect(-tabs.__direction * tabs._scrollerElement.scrollLeft).to.be.approximately(537, 10);
});

it('should have displayed all the items fully when scrolled back to the start via button', async () => {
// Initially scroll to the end
tabs._scrollToItem(items.length - 1);

const backButton = tabs.shadowRoot.querySelector('[part="back-button"]');

expect(-tabs.__direction * tabs._scrollerElement.scrollLeft).to.be.approximately(537, 10);

backButton.click();
expect(-tabs.__direction * tabs._scrollerElement.scrollLeft).to.be.approximately(228, 10);

backButton.click();
expect(tabs._scrollerElement.scrollLeft).to.equal(0);
});

it('should not get stuck with wide tabs when scrolled forward to the end via button', async () => {
tabs.style.width = '100px';

const forwardButton = tabs.shadowRoot.querySelector('[part="forward-button"]');
let previousScrollLeft;
let currentScrollLeft = tabs._scrollerElement.scrollLeft;
// Click the forward button until it does not have any effect
do {
previousScrollLeft = currentScrollLeft;
forwardButton.click();
currentScrollLeft = tabs._scrollerElement.scrollLeft;
} while (previousScrollLeft !== currentScrollLeft);

const scrollerEndPosition =
-tabs.__direction * tabs._scrollerElement.scrollLeft + tabs._scrollerElement.offsetWidth;
expect(scrollerEndPosition).to.be.approximately(tabs._scrollerElement.scrollWidth, 1);
});

it('should not get stuck with wide tabs when scrolled back to the start via button', async () => {
tabs.style.width = '100px';

// Initially scroll to the end
tabs._scrollToItem(items.length - 1);

const backButton = tabs.shadowRoot.querySelector('[part="back-button"]');
let previousScrollLeft;
let currentScrollLeft = tabs._scrollerElement.scrollLeft;
// Click the back button until it does not have any effect
do {
previousScrollLeft = currentScrollLeft;
backButton.click();
currentScrollLeft = tabs._scrollerElement.scrollLeft;
} while (previousScrollLeft !== currentScrollLeft);

expect(tabs._scrollerElement.scrollLeft).to.be.approximately(0, 1);
});
});
});
});

describe('vertical', () => {
Expand Down

0 comments on commit 52a1201

Please sign in to comment.