From 9a725f2c563e772dfbea9bba18ac13096521774a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 21 Mar 2022 14:57:41 +0100 Subject: [PATCH 1/5] fix: do not dispatch cell-focus on mouse up outside of cell --- packages/grid/src/vaadin-grid.js | 13 ++++++---- .../grid/test/keyboard-navigation.test.js | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/packages/grid/src/vaadin-grid.js b/packages/grid/src/vaadin-grid.js index a0bb78d5fe..fdaad14d75 100644 --- a/packages/grid/src/vaadin-grid.js +++ b/packages/grid/src/vaadin-grid.js @@ -7,7 +7,7 @@ import './vaadin-grid-column.js'; import './vaadin-grid-styles.js'; import { beforeNextRender } from '@polymer/polymer/lib/utils/render-status.js'; import { html, PolymerElement } from '@polymer/polymer/polymer-element.js'; -import { isAndroid, isFirefox, isIOS, isSafari, isTouch } from '@vaadin/component-base/src/browser-utils.js'; +import { isAndroid, isChrome, isFirefox, isIOS, isSafari, isTouch } from '@vaadin/component-base/src/browser-utils.js'; import { ElementMixin } from '@vaadin/component-base/src/element-mixin.js'; import { TabindexMixin } from '@vaadin/component-base/src/tabindex-mixin.js'; import { processTemplates } from '@vaadin/component-base/src/templates.js'; @@ -685,13 +685,16 @@ class Grid extends ElementMixin( // focusable slot wrapper, that is why cells are not focused with // mousedown. Workaround: listen for mousedown and focus manually. cellContent.addEventListener('mousedown', () => { - if (window.chrome) { + if (isChrome) { // Chrome bug: focusing before mouseup prevents text selection, see http://crbug.com/771903 - const mouseUpListener = () => { - if (!cellContent.contains(this.getRootNode().activeElement)) { + const mouseUpListener = (event) => { + // If focus is on element within the cell content — respect it, do not change + const contentContainsFocusedElement = cellContent.contains(this.getRootNode().activeElement); + // Only focus if mouse is released on cell content itself + const mouseUpWithinCell = event.target === cellContent || cellContent.contains(event.target); + if (!contentContainsFocusedElement && mouseUpWithinCell) { cell.focus(); } - // If focus is in the cell content — respect it, do not change. document.removeEventListener('mouseup', mouseUpListener, true); }; document.addEventListener('mouseup', mouseUpListener, true); diff --git a/packages/grid/test/keyboard-navigation.test.js b/packages/grid/test/keyboard-navigation.test.js index a1fe3bff13..ab25908517 100644 --- a/packages/grid/test/keyboard-navigation.test.js +++ b/packages/grid/test/keyboard-navigation.test.js @@ -18,6 +18,7 @@ import '../vaadin-grid.js'; import '../vaadin-grid-tree-column.js'; import '../vaadin-grid-column-group.js'; import '../vaadin-grid-selection-column.js'; +import { isChrome } from '@vaadin/component-base/src/browser-utils.js'; import { flushGrid, getCell, @@ -1889,6 +1890,31 @@ describe('keyboard navigation', () => { grid.removeEventListener('cell-focus', spy); }); + + // Chrome uses a workaround to dispatch cell-focus in mouse up, + // should dispatch event on mouse up events on cell itself + (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up inside of cell', () => { + const spy = sinon.spy(); + grid.addEventListener('cell-focus', spy); + + // Mouse down and release on cell content + const cell = getRowFirstCell(0); + mouseDown(cell._content); + mouseUp(cell._content); + expect(spy.calledOnce).to.be.true; + }); + + // Chrome uses a workaround to dispatch cell-focus in mouse up, + // should not dispatch event on mouse up events outside of cell + // Regression test for https://github.com/vaadin/flow-components/issues/2863 + (isChrome ? it : it.skip)('should not dispatch cell-focus on mouse up outside of cell', () => { + const spy = sinon.spy(); + grid.addEventListener('cell-focus', spy); + + mouseDown(getRowFirstCell(0)._content); + mouseUp(document.body); + expect(spy.calledOnce).to.be.false; + }); }); }); From c534ac5b2a23d3bb4c07a3515931c357be146826 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Mon, 21 Mar 2022 15:51:15 +0100 Subject: [PATCH 2/5] import chrome check from test helpers --- packages/grid/test/keyboard-navigation.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grid/test/keyboard-navigation.test.js b/packages/grid/test/keyboard-navigation.test.js index ab25908517..f556e25388 100644 --- a/packages/grid/test/keyboard-navigation.test.js +++ b/packages/grid/test/keyboard-navigation.test.js @@ -4,6 +4,7 @@ import { down as mouseDown, fixtureSync, focusin, + isChrome, keyboardEventFor, keyDownOn, keyUpOn, @@ -18,7 +19,6 @@ import '../vaadin-grid.js'; import '../vaadin-grid-tree-column.js'; import '../vaadin-grid-column-group.js'; import '../vaadin-grid-selection-column.js'; -import { isChrome } from '@vaadin/component-base/src/browser-utils.js'; import { flushGrid, getCell, From daf1f0e9931d501313069d42b936b2d4e3b0babc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Tue, 22 Mar 2022 15:38:20 +0100 Subject: [PATCH 3/5] address review comments --- packages/grid/src/vaadin-grid.js | 2 +- packages/grid/test/keyboard-navigation.test.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/grid/src/vaadin-grid.js b/packages/grid/src/vaadin-grid.js index fdaad14d75..264b2e6a59 100644 --- a/packages/grid/src/vaadin-grid.js +++ b/packages/grid/src/vaadin-grid.js @@ -691,7 +691,7 @@ class Grid extends ElementMixin( // If focus is on element within the cell content — respect it, do not change const contentContainsFocusedElement = cellContent.contains(this.getRootNode().activeElement); // Only focus if mouse is released on cell content itself - const mouseUpWithinCell = event.target === cellContent || cellContent.contains(event.target); + const mouseUpWithinCell = cellContent.contains(event.target); if (!contentContainsFocusedElement && mouseUpWithinCell) { cell.focus(); } diff --git a/packages/grid/test/keyboard-navigation.test.js b/packages/grid/test/keyboard-navigation.test.js index f556e25388..6ab6c3e840 100644 --- a/packages/grid/test/keyboard-navigation.test.js +++ b/packages/grid/test/keyboard-navigation.test.js @@ -1892,8 +1892,8 @@ describe('keyboard navigation', () => { }); // Chrome uses a workaround to dispatch cell-focus in mouse up, - // should dispatch event on mouse up events on cell itself - (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up inside of cell', () => { + // should dispatch event when releasing mouse on cell content itself + (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up on cell content', () => { const spy = sinon.spy(); grid.addEventListener('cell-focus', spy); @@ -1905,7 +1905,7 @@ describe('keyboard navigation', () => { }); // Chrome uses a workaround to dispatch cell-focus in mouse up, - // should not dispatch event on mouse up events outside of cell + // should not dispatch event when releasing mouse outside of cell // Regression test for https://github.com/vaadin/flow-components/issues/2863 (isChrome ? it : it.skip)('should not dispatch cell-focus on mouse up outside of cell', () => { const spy = sinon.spy(); From 0d5c9b6136203f24085e4c2e4d44a2559cd15cdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Wed, 23 Mar 2022 08:57:03 +0100 Subject: [PATCH 4/5] add test for clicking on cell content child --- .../grid/test/keyboard-navigation.test.js | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/packages/grid/test/keyboard-navigation.test.js b/packages/grid/test/keyboard-navigation.test.js index 6ab6c3e840..b8dbdf6e25 100644 --- a/packages/grid/test/keyboard-navigation.test.js +++ b/packages/grid/test/keyboard-navigation.test.js @@ -1891,29 +1891,43 @@ describe('keyboard navigation', () => { grid.removeEventListener('cell-focus', spy); }); - // Chrome uses a workaround to dispatch cell-focus in mouse up, - // should dispatch event when releasing mouse on cell content itself - (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up on cell content', () => { - const spy = sinon.spy(); - grid.addEventListener('cell-focus', spy); + // Separate test suite for Chrome, where we use a workaround to dispatch + // cell-focus on mouse up + describe('chrome', () => { + (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up on cell content', () => { + const spy = sinon.spy(); + grid.addEventListener('cell-focus', spy); - // Mouse down and release on cell content - const cell = getRowFirstCell(0); - mouseDown(cell._content); - mouseUp(cell._content); - expect(spy.calledOnce).to.be.true; - }); + // Mouse down and release on cell content element + const cell = getRowFirstCell(0); + mouseDown(cell._content); + mouseUp(cell._content); + expect(spy.calledOnce).to.be.true; + }); - // Chrome uses a workaround to dispatch cell-focus in mouse up, - // should not dispatch event when releasing mouse outside of cell - // Regression test for https://github.com/vaadin/flow-components/issues/2863 - (isChrome ? it : it.skip)('should not dispatch cell-focus on mouse up outside of cell', () => { - const spy = sinon.spy(); - grid.addEventListener('cell-focus', spy); + (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up within cell content', () => { + const spy = sinon.spy(); + grid.addEventListener('cell-focus', spy); + + // Mouse down and release on cell content child + const cell = getRowFirstCell(0); + const contentSpan = document.createElement('span'); + cell._content.appendChild(contentSpan); + + mouseDown(contentSpan); + mouseUp(contentSpan); + expect(spy.calledOnce).to.be.true; + }); + + // Regression test for https://github.com/vaadin/flow-components/issues/2863 + (isChrome ? it : it.skip)('should not dispatch cell-focus on mouse up outside of cell', () => { + const spy = sinon.spy(); + grid.addEventListener('cell-focus', spy); - mouseDown(getRowFirstCell(0)._content); - mouseUp(document.body); - expect(spy.calledOnce).to.be.false; + mouseDown(getRowFirstCell(0)._content); + mouseUp(document.body); + expect(spy.calledOnce).to.be.false; + }); }); }); }); From de4edb4063f4a259b5fb087b4468bf4ee1eb8c6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Wed, 23 Mar 2022 09:18:40 +0100 Subject: [PATCH 5/5] refactor to skip whole suite --- packages/grid/test/keyboard-navigation.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/grid/test/keyboard-navigation.test.js b/packages/grid/test/keyboard-navigation.test.js index b8dbdf6e25..b498e3b56e 100644 --- a/packages/grid/test/keyboard-navigation.test.js +++ b/packages/grid/test/keyboard-navigation.test.js @@ -1893,8 +1893,8 @@ describe('keyboard navigation', () => { // Separate test suite for Chrome, where we use a workaround to dispatch // cell-focus on mouse up - describe('chrome', () => { - (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up on cell content', () => { + (isChrome ? describe : describe.skip)('chrome', () => { + it('should dispatch cell-focus on mouse up on cell content', () => { const spy = sinon.spy(); grid.addEventListener('cell-focus', spy); @@ -1905,7 +1905,7 @@ describe('keyboard navigation', () => { expect(spy.calledOnce).to.be.true; }); - (isChrome ? it : it.skip)('should dispatch cell-focus on mouse up within cell content', () => { + it('should dispatch cell-focus on mouse up within cell content', () => { const spy = sinon.spy(); grid.addEventListener('cell-focus', spy); @@ -1920,7 +1920,7 @@ describe('keyboard navigation', () => { }); // Regression test for https://github.com/vaadin/flow-components/issues/2863 - (isChrome ? it : it.skip)('should not dispatch cell-focus on mouse up outside of cell', () => { + it('should not dispatch cell-focus on mouse up outside of cell', () => { const spy = sinon.spy(); grid.addEventListener('cell-focus', spy);