From f30bd33fd8f1b92152502b01b5386ece347e18ba Mon Sep 17 00:00:00 2001
From: Tomi Virkki <tomivirkki@users.noreply.github.com>
Date: Tue, 28 May 2024 12:59:13 +0300
Subject: [PATCH] fix: do not cancel edit column cell click events (#7453)

---
 .../vaadin-grid-pro-inline-editing-mixin.js   | 23 ++++++-----
 packages/grid-pro/test/edit-column.common.js  | 16 ++++++++
 .../grid/src/vaadin-grid-active-item-mixin.js | 40 +++++++++++++------
 3 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js b/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js
index f5eb6b2dde..ccfea3a009 100644
--- a/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js
+++ b/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js
@@ -120,15 +120,6 @@ export const InlineEditingMixin = (superClass) =>
       this.$.table.addEventListener('click', (e) => {
         const column = this.getEventContext(e).column;
         if (column && this._isEditColumn(column)) {
-          if (e.target.matches(':not([type=checkbox])')) {
-            // Prevents the `click` event from a column's cell in order to prevent making the cell's parent row active.
-            // Note, it should not prevent the `click` event from checkboxes. Otherwise, they will not respond to user interactions.
-            // Read more: https://github.com/vaadin/web-components/pull/2539#discussion_r712076433.
-            // TODO: Using `preventDefault()` for any other purpose than preventing the default browser's behavior is bad practice
-            // and therefore it would be great to refactor this part someday.
-            e.preventDefault();
-          }
-
           if (this.editOnClick) {
             this._enterEditFromEvent(e);
           }
@@ -166,6 +157,20 @@ export const InlineEditingMixin = (superClass) =>
       }
     }
 
+    /**
+     * Prevents making an edit column cell's parent row active on click.
+     *
+     * @override
+     * @protected
+     */
+    _shouldPreventCellActivationOnClick(e) {
+      return (
+        super._shouldPreventCellActivationOnClick(e) ||
+        // The clicked cell is editable
+        this._isEditColumn(this.getEventContext(e).column)
+      );
+    }
+
     /**
      * Override an observer from `DisabledMixin` to stop
      * editing when grid element becomes disabled.
diff --git a/packages/grid-pro/test/edit-column.common.js b/packages/grid-pro/test/edit-column.common.js
index 3a3a4b5ec8..1d60d481ed 100644
--- a/packages/grid-pro/test/edit-column.common.js
+++ b/packages/grid-pro/test/edit-column.common.js
@@ -695,6 +695,22 @@ describe('edit column', () => {
         cell._content.click();
         expect(activeItemChangedSpy.called).to.be.true;
       });
+
+      it('should not fire the event on focusable content click in the not editable column', () => {
+        cell = getContainerCell(grid.$.items, 0, 3);
+        cell._content.append(document.createElement('button'));
+        cell._content.querySelector('button').click();
+        expect(activeItemChangedSpy.called).to.be.false;
+      });
+
+      it('should not cancel click events on editable column cells', () => {
+        const clickSpy = sinon.spy();
+        grid.addEventListener('click', clickSpy);
+        cell = getContainerCell(grid.$.items, 0, 1);
+        cell._content.click();
+        expect(clickSpy.called).to.be.true;
+        expect(clickSpy.getCall(0).args[0].defaultPrevented).to.be.false;
+      });
     });
   });
 
diff --git a/packages/grid/src/vaadin-grid-active-item-mixin.js b/packages/grid/src/vaadin-grid-active-item-mixin.js
index 9d603e9e1e..c31858f36c 100644
--- a/packages/grid/src/vaadin-grid-active-item-mixin.js
+++ b/packages/grid/src/vaadin-grid-active-item-mixin.js
@@ -69,27 +69,41 @@ export const ActiveItemMixin = (superClass) =>
     }
 
     /**
-     * We need to listen to click instead of tap because on mobile safari, the
-     * document.activeElement has not been updated (focus has not been shifted)
-     * yet at the point when tap event is being executed.
+     * Checks whether the click event should not activate the cell on which it occurred.
+     *
+     * @protected
+     */
+    _shouldPreventCellActivationOnClick(e) {
+      const { cell } = this._getGridEventLocation(e);
+      return (
+        // Something has handled this click already, e. g., <vaadin-grid-sorter>
+        e.defaultPrevented ||
+        // No clicked cell available
+        !cell ||
+        // Cell is a details cell
+        cell.getAttribute('part').includes('details-cell') ||
+        // Cell is the empty state cell
+        cell === this.$.emptystatecell ||
+        // Cell content is focused
+        cell._content.contains(this.getRootNode().activeElement) ||
+        // Clicked on a focusable element
+        this._isFocusable(e.target) ||
+        // Clicked on a label element
+        e.target instanceof HTMLLabelElement
+      );
+    }
+
+    /**
      * @param {!MouseEvent} e
      * @protected
      */
     _onClick(e) {
-      if (e.defaultPrevented) {
-        // Something has handled this click already, e. g., <vaadin-grid-sorter>
+      if (this._shouldPreventCellActivationOnClick(e)) {
         return;
       }
 
       const { cell } = this._getGridEventLocation(e);
-      if (!cell || cell.getAttribute('part').indexOf('details-cell') > -1 || cell === this.$.emptystatecell) {
-        return;
-      }
-      const cellContent = cell._content;
-
-      const activeElement = this.getRootNode().activeElement;
-      const cellContentHasFocus = cellContent.contains(activeElement);
-      if (!cellContentHasFocus && !this._isFocusable(e.target) && !(e.target instanceof HTMLLabelElement)) {
+      if (cell) {
         this.dispatchEvent(
           new CustomEvent('cell-activate', {
             detail: {