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: do not cancel edit column cell click events (#7453) #7458

Merged
merged 1 commit into from
May 29, 2024
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
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
41 changes: 26 additions & 15 deletions packages/grid/src/vaadin-grid-active-item-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,28 +69,39 @@ 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.
* @param {!MouseEvent} e
* Checks whether the click event should not activate the cell on which it occurred.
*
* @protected
*/
_onClick(e) {
if (e.defaultPrevented) {
_shouldPreventCellActivationOnClick(e) {
const { cell } = this._getGridEventLocation(e);
return (
// Something has handled this click already, e. g., <vaadin-grid-sorter>
return;
}
e.defaultPrevented ||
// No clicked cell available
!cell ||
// Cell is a details cell
cell.getAttribute('part').includes('details-cell') ||
// 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
);
}

const path = e.composedPath();
const cell = path[path.indexOf(this.$.table) - 3];
if (!cell || cell.getAttribute('part').indexOf('details-cell') > -1) {
/**
* @param {!MouseEvent} e
* @protected
*/
_onClick(e) {
if (this._shouldPreventCellActivationOnClick(e)) {
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)) {
const { cell } = this._getGridEventLocation(e);
if (cell) {
this.dispatchEvent(
new CustomEvent('cell-activate', {
detail: {
Expand Down
Loading