Skip to content

Commit

Permalink
fix: do not cancel edit column cell click events (#7453)
Browse files Browse the repository at this point in the history
  • Loading branch information
tomivirkki committed May 28, 2024
1 parent bdabeea commit f30bd33
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 22 deletions.
23 changes: 14 additions & 9 deletions packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
Expand Down
16 changes: 16 additions & 0 deletions packages/grid-pro/test/edit-column.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
});

Expand Down
40 changes: 27 additions & 13 deletions packages/grid/src/vaadin-grid-active-item-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down

0 comments on commit f30bd33

Please sign in to comment.