Skip to content

DataGrid/TreeList: The click area on the separator must be at least 24 x 24 px - #35413

Open
markallenramirez wants to merge 6 commits into
DevExpress:mainfrom
markallenramirez:a11y_grids_separator/main
Open

markallenramirez wants to merge 6 commits into
DevExpress:mainfrom
markallenramirez:a11y_grids_separator/main

Conversation

@markallenramirez

Copy link
Copy Markdown
Contributor

No description provided.

@markallenramirez markallenramirez self-assigned this Sep 30, 2026
@markallenramirez
markallenramirez requested a review from a team as a code owner September 30, 2026 09:09
Copilot AI balanced review requested due to automatic review settings September 30, 2026 09:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

An existing touch-resizing QUnit assertion is incompatible with the new final separator position.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Expands the DataGrid/TreeList column-resize target to 24px.

Changes:

  • Enlarges and centers the transparent separator hit area.
  • Manages pointer events and separator positioning during resizing.
File Description
m_columns_resizing_reordering.ts Updates resize hit detection and separator state.
_index.scss Sets the transparent separator width to 24px.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI balanced review requested due to automatic review settings October 2, 2026 11:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The new final separator repositioning leaves an existing touch-start QUnit assertion stale and guaranteed to fail.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:07
@markallenramirez
markallenramirez force-pushed the a11y_grids_separator/main branch from be54652 to a2c1123 Compare October 6, 2026 13:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Separator positioning breaks an existing assertion, and overlapping targets can make narrow-column boundaries unreachable.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment on lines 877 to 881
that._targetPoint = that._getTargetPoint(
that.pointsByColumns(),
eventData,
columnsSeparatorWidth,
deltaX,
);
Copilot AI balanced review requested due to automatic review settings October 6, 2026 14:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new method calls break existing QUnit tests whose shared separator mock lacks the method.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

that._isReadyResizing = false;

if (that._targetPoint) {
that._columnsSeparatorView.changePointerEvents('auto');
Copilot AI balanced review requested due to automatic review settings October 6, 2026 14:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Grid coverage must use Jest, and the changed touch boundary currently lacks regression tests.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity · 1 Low severity

Open (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add Jest boundary tests for the 12px touch hit range

packages/​devextreme/​js/​__internal/​grids/​grid_core/​columns_resizing_reordering/​m_columns_resizing_reordering.ts:970

The new touch hit range is not covered at its changed boundary. Existing touch tests start directly on the separator or 10 px away, while the added TestCafe test exercises mouse input, so reverting this calculation to the old 10 px constant would still pass. Add Jest touch-start cases at 12 px (accepted) and just beyond 12 px (rejected).

assert.equal(columnsSeparator.element().css('cursor'), 'col-resize', 'cursor');
});

QUnit.test('changePointerEvents', function(assert) {
Copilot AI balanced review requested due to automatic review settings October 7, 2026 14:52
@markallenramirez
markallenramirez force-pushed the a11y_grids_separator/main branch from 32b6b64 to b37441b Compare October 7, 2026 14:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The shared grid implementation, styling, mocks, and regression coverage consistently implement the expanded resize target.

3 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 7, 2026 14:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Overlapping targets select the wrong separator for narrow columns, and a debug directive is unmatched.

4 open findings

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants