Skip to content

Commit 54a6857

Browse files
fix: keep draggable grid cells announced by screen readers (#12135) (#12143)
Co-authored-by: Diego Cardoso <diego@vaadin.com>
1 parent cc5d3e2 commit 54a6857

6 files changed

Lines changed: 80 additions & 6 deletions

File tree

packages/grid/src/styles/vaadin-grid-base-styles.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,13 @@ export const gridStyles = css`
601601
outline-offset: calc(var(--vaadin-grid-border-width, 1px) * -1);
602602
}
603603
604+
/* Styles applied to draggable cell content; see _filterDragAndDrop in vaadin-grid-drag-and-drop-mixin.js. */
605+
::slotted(vaadin-grid-cell-content[draggable-source]) {
606+
-webkit-user-drag: element;
607+
-webkit-user-select: none;
608+
user-select: none;
609+
}
610+
604611
.row[dragover] {
605612
z-index: 100 !important;
606613
}

packages/grid/src/vaadin-grid-drag-and-drop-mixin.js

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -481,11 +481,23 @@ export const DragAndDropMixin = (superClass) =>
481481
const dragDisabled = !this.rowsDraggable || loading || (this.dragFilter && !this.dragFilter(model));
482482
const dropDisabled = !this.dropMode || loading || (this.dropFilter && !this.dropFilter(model));
483483

484+
// Chromium computes an empty accessible name for cells whose content
485+
// element has the `draggable` attribute when the accessibility tree is
486+
// built (https://issues.chromium.org/issues/534049472,
487+
// https://github.com/vaadin/web-components/issues/11726). Blink maps
488+
// `draggable="true"` to the `-webkit-user-drag: element` and
489+
// `user-select: none` styles, and the accessible-name bug is keyed to the
490+
// attribute, not the styles. So on Chromium, mark the draggable cell
491+
// content with the `draggable-source` attribute, which applies the
492+
// equivalent styles from the grid styles, instead of the `draggable`
493+
// attribute. This can be removed once the Chromium issue is fixed.
494+
const draggableAttribute = isChrome ? 'draggable-source' : 'draggable';
495+
484496
iterateRowCells(row, (cell) => {
485497
if (dragDisabled) {
486-
cell._content.removeAttribute('draggable');
498+
cell._content.removeAttribute(draggableAttribute);
487499
} else {
488-
cell._content.setAttribute('draggable', true);
500+
cell._content.setAttribute(draggableAttribute, true);
489501
}
490502
});
491503

packages/grid/test/column-rendering.test.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,15 @@ import { aTimeout, fixtureSync, keyDownOn, nextFrame, nextResize, oneEvent } fro
33
import Sinon from 'sinon';
44
import './grid-test-styles.js';
55
import '../all-imports.js';
6-
import { dragAndDropOver, dragStart, fire, flushGrid, getCellContent, getHeaderCellContent } from './helpers.js';
6+
import {
7+
cellDraggableAttribute,
8+
dragAndDropOver,
9+
dragStart,
10+
fire,
11+
flushGrid,
12+
getCellContent,
13+
getHeaderCellContent,
14+
} from './helpers.js';
715

816
['ltr', 'rtl'].forEach((dir) => {
917
describe(`lazy column rendering - ${dir}`, () => {
@@ -411,12 +419,12 @@ import { dragAndDropOver, dragStart, fire, flushGrid, getCellContent, getHeaderC
411419
grid.rowsDraggable = true;
412420
await nextFrame();
413421

414-
expect(getBodyCellContent(getLastVisibleColumnIndex()).getAttribute('draggable')).to.equal('true');
422+
expect(getBodyCellContent(getLastVisibleColumnIndex()).getAttribute(cellDraggableAttribute)).to.equal('true');
415423

416424
// Scroll back to the beginning
417425
await scrollHorizontally(-grid.$.table.scrollWidth);
418426
// Expect the cell that was previously not visible to have the draggable attribute
419-
expect(getBodyCellContent(0).getAttribute('draggable')).to.equal('true');
427+
expect(getBodyCellContent(0).getAttribute(cellDraggableAttribute)).to.equal('true');
420428
});
421429

422430
it('should not create excess cells for a row', async () => {

packages/grid/test/drag-and-drop.test.js

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import { aTimeout, fixtureSync, listenOnce, nextFrame, nextResize } from '@vaadi
44
import sinon from 'sinon';
55
import './grid-test-styles.js';
66
import '../src/vaadin-grid.js';
7+
import { isChrome } from '@vaadin/component-base/src/browser-utils.js';
78
import {
9+
cellDraggableAttribute,
810
dragAndDropOver,
911
flushGrid,
1012
getBodyCellContent,
@@ -25,7 +27,7 @@ describe('drag and drop', () => {
2527
const getDraggable = (grid, rowIndex = 0) => {
2628
const row = Array.from(grid.$.items.children).find((row) => row.index === rowIndex);
2729
const cellContent = row.querySelector('slot').assignedNodes()[0];
28-
return [row, cellContent].find((node) => node.getAttribute('draggable') === 'true');
30+
return [row, cellContent].find((node) => node.getAttribute(cellDraggableAttribute) === 'true');
2931
};
3032

3133
const fireDragStart = (draggable = getDraggable(grid)) => {
@@ -154,6 +156,39 @@ describe('drag and drop', () => {
154156
expect(getDraggable(grid)).not.to.be.ok;
155157
});
156158

159+
// See https://github.com/vaadin/web-components/issues/11726
160+
describe('accessible name workaround', () => {
161+
const getContent = (row = 0) => getBodyCellContent(grid, row, 0);
162+
163+
(isChrome ? it : it.skip)('should not set the draggable attribute on Chromium', () => {
164+
expect(getContent().hasAttribute('draggable')).to.be.false;
165+
});
166+
167+
(isChrome ? it : it.skip)('should mark draggable content with draggable-source styles on Chromium', () => {
168+
expect(getContent().getAttribute('draggable-source')).to.equal('true');
169+
const style = getComputedStyle(getContent());
170+
expect(style.webkitUserDrag).to.equal('element');
171+
expect(style.userSelect).to.equal('none');
172+
});
173+
174+
(isChrome ? it.skip : it)('should set the draggable attribute on other browsers', () => {
175+
expect(getContent().getAttribute('draggable')).to.equal('true');
176+
});
177+
178+
it('should unset the drag marker when rowsDraggable is disabled', () => {
179+
grid.rowsDraggable = false;
180+
flushGrid(grid);
181+
expect(getContent().hasAttribute(cellDraggableAttribute)).to.be.false;
182+
});
183+
184+
it('should not set the drag marker on rows excluded by dragFilter', () => {
185+
grid.dragFilter = (model) => model.index !== 0;
186+
flushGrid(grid);
187+
expect(getContent(0).hasAttribute(cellDraggableAttribute)).to.be.false;
188+
expect(getContent(1).getAttribute(cellDraggableAttribute)).to.equal('true');
189+
});
190+
});
191+
157192
describe('dragstart', () => {
158193
let dragStartSpy;
159194
let dropSpy;

packages/grid/test/helpers.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
import sinon from 'sinon';
2+
import { isChrome } from '@vaadin/component-base/src/browser-utils.js';
3+
4+
// Draggable cell content uses a browser-specific attribute; see _filterDragAndDrop.
5+
export const cellDraggableAttribute = isChrome ? 'draggable-source' : 'draggable';
26

37
export const flushGrid = (grid) => {
48
grid.performUpdate?.();

packages/vaadin-lumo-styles/src/components/grid.css

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -420,6 +420,14 @@
420420
}
421421

422422
/* Drag and Drop styles */
423+
424+
/* Draggable cell content; see _filterDragAndDrop in vaadin-grid-drag-and-drop-mixin.js. */
425+
::slotted(vaadin-grid-cell-content[draggable-source]) {
426+
-webkit-user-drag: element;
427+
-webkit-user-select: none;
428+
user-select: none;
429+
}
430+
423431
:host([dragover])::after {
424432
content: '';
425433
position: absolute;

0 commit comments

Comments
 (0)