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

fix: hide tooltip while scrolling #4819

Merged
merged 2 commits into from
Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions integration/tests/grid-tooltip.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
aTimeout,
enter,
escKeyDown,
fire,
fixtureSync,
focusin,
focusout,
Expand All @@ -12,6 +13,7 @@ import {
nextRender,
tabKeyDown,
} from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import sinon from 'sinon';
import '@vaadin/grid/vaadin-grid.js';
import '@vaadin/tooltip/vaadin-tooltip.js';
Expand Down Expand Up @@ -209,6 +211,39 @@ describe('tooltip', () => {
expect(tooltip.opened).to.be.false;
});

it('should hide the tooltip when starting scrolling', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also solves an issue in Safari which does not trigger mouseenter while scrolling (with a trackpad at least):

Bildschirmaufnahme.2022-10-25.um.10.04.10.mov

const cell = getCell(grid, 0);
mouseenter(cell);

fire(grid.$.table, 'scroll');
await nextFrame();

expect(tooltip.opened).to.be.false;
});

it('should not hide the tooltip when scrolling using keyboard navigation', async () => {
const cell = getCell(grid, 0);
tabKeyDown(document.body);
focusin(cell);
expect(tooltip.opened).to.be.true;

await sendKeys({ press: 'ArrowDown' });
fire(grid.$.table, 'scroll');
await nextFrame();

expect(tooltip.opened).to.be.true;
});

it('should not show the tooltip on mouseenter while scrolling', async () => {
fire(grid.$.table, 'scroll');
await nextFrame();

const cell = getCell(grid, 0);
mouseenter(cell);

expect(tooltip.opened).to.be.false;
});

it('should not set tooltip target if there is no tooltip', async () => {
const spy = sinon.spy(grid._tooltipController, 'setTarget');

Expand Down
3 changes: 3 additions & 0 deletions packages/grid/src/vaadin-grid-scroll-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ export const ScrollMixin = (superClass) =>
if (!this.hasAttribute('reordering')) {
this._scheduleScrolling();
}
if (!this.hasAttribute('navigating')) {
this._hideTooltip(true);
}

this._updateOverflow();
}
Expand Down
4 changes: 3 additions & 1 deletion packages/grid/src/vaadin-grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,9 @@ class Grid extends ElementMixin(
// For now we only support tooltip on desktop
if (!isAndroid && !isIOS) {
cell.addEventListener('mouseenter', (event) => {
this._showTooltip(event);
if (!this.$.scroller.hasAttribute('scrolling')) {
this._showTooltip(event);
}
});

cell.addEventListener('mouseleave', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/test/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ import sinon from 'sinon';

export const flushGrid = (grid) => {
grid._observer.flush();
grid._afterScroll();
if (grid._debounceScrolling) {
grid._debounceScrolling.flush();
}
grid._afterScroll();
Comment on lines 3 to -8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes flushGrid to also clear the scrolling attribute.

if (grid._debounceOverflow) {
grid._debounceOverflow.flush();
}
Expand Down