refactor!: fire click on space for cells with text content (#10970)

Author: sissbruecker

packages/grid/src/vaadin-grid-active-item-mixin.js

@@ -70,6 +70,8 @@ export const ActiveItemMixin = (superClass) =>
       return (
         // Something has handled this click already, e. g., <vaadin-grid-sorter>
         e.defaultPrevented ||
+        // Skip if click event explicitly requests to skip cell activation
+        e.skipCellActivate ||
         // No clicked cell available
         !cell ||
         // Cell is a details cell

packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js

@@ -766,19 +766,31 @@ export const KeyboardNavigationMixin = (superClass) =>
 
       e.preventDefault();
 
+      // Fire click event on the first element in the cell, or the cell itself.
+      // This serves two purposes:
+      // - Activate any buttons or other clickable elements in the cell
+      // - Fire a general click event for the cell, which can be used on the
+      //   application level to detect clicks on cells / items in the grid.
+      //   Used by the Flow component to implement the `item-click` event.
+      //
+      // When clicking an element in the cell, allow activating the cell since
+      // that is excluded from the keydown handler above. Active item mixin
+      // takes care of only activating the item under correct conditions.
+      //
+      // When clicking a cell with only text nodes, skip activating the cell
+      // on click, since that is already handled on keydown.
       const cell = e.composedPath()[0];
-      if (cell._content && cell._content.firstElementChild) {
-        const wasNavigating = this.hasAttribute('navigating');
-        cell._content.firstElementChild.dispatchEvent(
-          new MouseEvent('click', {
-            shiftKey: e.shiftKey,
-            bubbles: true,
-            composed: true,
-            cancelable: true,
-          }),
-        );
-        this.toggleAttribute('navigating', wasNavigating);
-      }
+      const target = (cell._content && cell._content.firstElementChild) || cell;
+      const wasNavigating = this.hasAttribute('navigating');
+      const clickEvent = new MouseEvent('click', {
+        shiftKey: e.shiftKey,
+        bubbles: true,
+        composed: true,
+        cancelable: true,
+      });
+      clickEvent.skipCellActivate = target === cell;
+      target.dispatchEvent(clickEvent);
+      this.toggleAttribute('navigating', wasNavigating);
     }
 
     /**

packages/grid/test/keyboard-navigation.test.js

@@ -20,12 +20,14 @@ import '../all-imports.js';
 import {
   attributeRenderer,
   flushGrid,
+  getBodyCell,
   getCell,
   getCellContent,
   getContainerCell,
   getFirstVisibleItem,
   getFocusedCellIndex,
   getFocusedRowIndex,
+  getHeaderCell,
   getLastVisibleItem,
   getPhysicalItems,
   getRowCells,
@@ -101,6 +103,11 @@ function spaceUp(target) {
   keyUpOn(target || grid.shadowRoot.activeElement, 32, [], ' ');
 }
 
+function spacePress(target) {
+  spaceDown(target);
+  spaceUp(target);
+}
+
 function pageUp(target) {
   keyDownOn(target || grid.shadowRoot.activeElement, 33, [], 'PageUp');
 }
@@ -1399,7 +1406,7 @@ describe('keyboard navigation', () => {
     it('should activate on space keydown', () => {
       tabToBody();
 
-      spaceDown();
+      spacePress();
 
       expect(grid.activeItem).to.equal('foo');
     });
@@ -1409,7 +1416,7 @@ describe('keyboard navigation', () => {
       clickItem(0);
 
       down();
-      spaceDown();
+      spacePress();
 
       expect(grid.activeItem).to.equal('bar');
     });
@@ -1418,7 +1425,7 @@ describe('keyboard navigation', () => {
       focusItem(0);
       clickItem(0); // Activates first item on click
 
-      spaceDown();
+      spacePress();
 
       expect(grid.activeItem).to.be.null;
     });
@@ -1468,7 +1475,7 @@ describe('keyboard navigation', () => {
     it('should not activate on space keydown click on a native input', () => {
       const input = focusFirstBodyInput(0);
       escape(input);
-      spaceDown();
+      spacePress();
 
       expect(grid.activeItem).to.be.null;
     });
@@ -1477,7 +1484,7 @@ describe('keyboard navigation', () => {
       clickItem(0);
 
       tabToHeader();
-      spaceDown();
+      spacePress();
 
       expect(grid.activeItem).to.equal('foo');
     });
@@ -1500,32 +1507,41 @@ describe('keyboard navigation', () => {
     });
 
     describe('space click shortcut', () => {
-      beforeEach(() => {
-        header.children[0].children[1].focus();
-        right();
-      });
+      function verifyClickOnSpace(cell) {
+        let composedPath;
+        const clickSpy = sinon.spy((e) => {
+          composedPath = e.composedPath();
+        });
+        grid.addEventListener('click', clickSpy);
 
-      it('should dispatch click event on first cell child on space keyup', () => {
-        const firstChild = getCellContent(header.children[0].children[2]).children[0];
-        const dispatchEventStub = sinon.stub(firstChild, 'dispatchEvent');
+        const firstElementChild = getCellContent(cell).firstElementChild;
 
-        spaceDown();
-        expect(dispatchEventStub.called).to.be.false;
+        spaceDown(cell);
+        expect(clickSpy.called).to.be.false;
 
-        spaceUp();
-        expect(dispatchEventStub.called).to.be.true;
-        expect(dispatchEventStub.args[0][0] instanceof MouseEvent).to.be.true;
-        expect(dispatchEventStub.args[0][0].type).to.equal('click');
-      });
+        spaceUp(cell);
+        expect(clickSpy.calledOnce).to.be.true;
+
+        const event = clickSpy.args[0][0];
+        const target = composedPath[0];
 
-      it('should not dispatch click event on other cell children on space keyup', () => {
-        const secondChild = getCellContent(header.children[0].children[2]).children[1];
-        const dispatchEventStub = sinon.stub(secondChild, 'dispatchEvent');
+        expect(event instanceof MouseEvent).to.be.true;
+        expect(event.type).to.equal('click');
+        expect(target).to.equal(firstElementChild || cell);
+      }
 
-        spaceDown();
-        spaceUp();
+      it('should dispatch click event on space keyup in header cells', () => {
+        // Verify for various cells with either text nodes or element children
+        grid.querySelectorAll('vaadin-grid-column').forEach((_, index) => {
+          verifyClickOnSpace(getHeaderCell(grid, 0, index));
+        });
+      });
 
-        expect(dispatchEventStub.called).to.be.false;
+      it('should dispatch click event on space keyup in body cells', () => {
+        // Verify for various cells with either text nodes or element children
+        grid.querySelectorAll('vaadin-grid-column').forEach((_, index) => {
+          verifyClickOnSpace(getBodyCell(grid, 0, index));
+        });
       });
 
       it('should prevent default keydown action when clicking on space', () => {
@@ -1537,21 +1553,16 @@ describe('keyboard navigation', () => {
       });
 
       it('should not activate if synthetic click has default prevented', () => {
-        const firstBodyRowFirstChild = getCellContent(getRows(body)[0].children[2]).children[0];
+        const bodyCell = getBodyCell(grid, 0, 2);
+        const firstChild = getCellContent(bodyCell).children[0];
 
         const spy = sinon.spy();
-        listenOnce(firstBodyRowFirstChild, 'click', (e) => {
+        listenOnce(firstChild, 'click', (e) => {
           spy();
           e.preventDefault();
         });
 
-        // Navigate to body, row 1, column 3
-        tabToBody();
-        right();
-        right();
-
-        spaceDown();
-        spaceUp();
+        spacePress(bodyCell);
 
         expect(spy.called).to.be.true;
         expect(grid.activeItem).to.be.null;