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

[EuiDataGrid] Implement draggable column headers #8015

Merged

Conversation

mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented Sep 11, 2024

Summary

closes #7136

This PR implements reordering EuiDataGrid columns via draggable column headers.
This implementation is opt-in keeping parity with current usages.

Todo

  • rebase/update after completed EuiDataGrid Emotion conversion
  • update VRT images
  • add docs update to EUI+ docs (DataGrid content is not yet available, EUI+ is still in need to be fully migrated/released)

Changes

  • adds prop canDragAndDropColumns on columnVisibility prop of EuiDataGrid to toggle draggable column headers - default value is false
  • implements EuiDragDropContext and EuiDroppable in EuiGridHeaderRow to enable a drop zone for all non-control columns
  • implements EuiDraggable in EuiDataGridHeaderCell to enable draggable column headers
    • uses usePortal on EuiDraggable because it requires reparenting via portal for dragged items to ensure correct positioning of the position: fixed element inside a transform (stacking) context
  • updates the customDragHandle prop value on EuiDraggable to be boolean | 'custom'
  • adds cypress tests for the dragging behavior

Accessibility

The Draggable implementation comes with great accessibility out of the box (adding hints and announcements where needed) BUT for the use case of datagrid headers it was not completely fitting and needed a small adjustment.

The expected behavior is, that focussing a column header, we want the columnheader role to be read with all its expected labels and attached descriptions.
EuiDraggable currently always added a role on the drag container (button or group) which will be read instead of the content (effectively losing us semantic information). To solve this we optionally remove the added role on the drag container, which results in the content being read on focus as wanted.

This was tested and optimized on Win11 for JAWS and NVDA.

Screen reader output

// JAWS
// draggable, non-interactive

1. focus of a header cell titled "Name"

Name Press the Enter key to view this column’s actions column header
Press space bar to start a drag. When dragging you can use the arrow keys to move the item around and escape to cancel. Some screen readers may require you to be in focus mode or to use your pass through key

2. press Enter to open the actions menu popover

Tabular Content / EuiDataGrid - Playground ⋅ Storybook - Google Chrome dialog
EuiDataGrid; Page 1 of 1.

3. press Escape to close the actions menu popover

Name Press the Enter key to view this column’s actions column header
Press space bar to start a drag. When dragging you can use the arrow keys to move the item around and escape to cancel. Some screen readers may require you to be in focus mode or to use your pass through key

4. press Space to drag
5. press ArrowRight reorder

You have lifted an item in position 2
You have moved the item from position 2 to position 3

6. press Space to confirm the drag and reorder

You have dropped the item. You have moved the item from position 2 to position 3
// JAWS
// draggable, interactive

1. focus of a header cell titled "Name"

Name, column header
Press the Enter key to interact with this cell’s contents. Press space bar to start a drag. When dragging you can use the arrow keys to move the item around and escape to cancel. Some screen readers may require you to be in focus mode or to use your pass through key

2. press Enter to enter the cell (focussing the first interactive child element)

Additional information Button
To activate press Enter.
tooltip content

3. Press Tab to focus the next child element

Name. Click to view column header actions. Button
To activate press Enter.

4. press Escape to leave the cell

Name, column header
Exited cell content. Press the Enter key to interact with this cell’s contents.

4. press Space to drag
5. press ArrowRight reorder

You have lifted an item in position 1
You have moved the item from position 1 to position 2

6. press Space to confirm the drag and reorder

You have dropped the item. You have moved the item from position 1 to position 2
Press the Enter key to interact with this cell’s contents.
// NVDA
// draggable, non-interactive

1. focus of a header cell titled "Name"

Name Press the Enter key to view this column's actions  column header  Press space bar to start a drag. When dragging you can use the arrow keys to move the item around and escape to cancel. Some screen readers may require you to be in focus mode or to use your pass through key  row 1  column 2

2. press Enter to open the actions menu popover

You are in a dialog. 
Press Escape, or tap/click outside the dialog to close.To navigate through the   list of column actions, press the Tab or Up and Down arrow keys.

3. press Escape to close the actions menu popover

EuiDataGrid; Page 1 of 1.  table
Name Press the Enter key to view this column's actions  column header  Press space bar to start a drag. When dragging you can use the arrow keys to move the item around and escape to cancel. Some screen readers may require you to be in focus mode or to use your pass through key

4. press Space to drag
5. press ArrowRight reorder

space
You have lifted an item in position 2 
You have moved the item from position 2 to position 3 

6. press Space to confirm the drag and reorder

space
You have dropped the item. You have moved the item from position 2 to position 3
// NVDA
// draggable, interactive

1. focus of a header cell titled "Name"

Name,  column header  Press the Enter key to interact with this cell's contents. Press space bar to start a drag. When dragging you can use the arrow keys to move the item around and escape to cancel. Some screen readers may require you to be in focus mode or to use your pass through key  row 1  column 1

2. press Enter to enter the cell (focussing the first interactive child element)

Additional information  button  
tooltip content

3. Press Tab to focus the next child element

Name. Click to view column header actions.  button  

4. press Escape to leave the cell

Name,  column header  Exited cell content. Press the Enter key to interact with this cell's contents.

4. press Space to drag
5. press ArrowRight reorder

space
You have lifted an item in position 1 
You have moved the item from position 1 to position 2 

6. press Space to confirm the drag and reorder

space
You have dropped the item. You have moved the item from position 1 to position 2

Screenshots

mouse usage

Screen.Recording.2024-09-10.at.18.01.57.mov

keyboard usage

Screen.Recording.2024-09-10.at.18.05.00.mov

resizing

Screen.Recording.2024-09-10.at.18.06.38.mov

QA

Storybook: https://eui.elastic.co/pr_8015/storybook/?path=/story/tabular-content-euidatagrid--playground&args=columnDragDrop:!true

EUI docs: https://eui.elastic.co/pr_8015/index.html#/tabular-content/data-grid-schema-columns#draggable-columns

  • verify columnDragDrop prop works to en/disable draggable columns
  • verify drag behavior works with mouse
    • draggable columns indicate drag behavior on hover
    • clicking draggable columns sets a focus state
    • draggable columns can be dragged and reorder of columns works as expected
    • opening the actions menu popover works and that clicking on another draggable column closes it
  • verify that the drag behavior works with keyboard
    • ArrowLeft/Right keys focus draggable header cells
    • Space key starts dragging a cell
      • ArrowLeft/Right move a dragged cell to another position
      • Space drops the dragged cell in the current position
      • Escape key aborts the drag
    • Enter key on interactive header cells enters the cell content
      • Escape in an interactive cell exits the cell and focuses it
  • verify that resizing a draggable cell works as expected

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

@@ -110,6 +115,8 @@ export const EuiDraggable: FunctionComponent<EuiDraggableProps> = ({
role={
hasInteractiveChildren
? 'group'
: customDragHandle === 'custom'
Copy link
Contributor Author

@mgadewoll mgadewoll Sep 11, 2024

Choose a reason for hiding this comment

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

The idea here is to provide means to unset the role for specific use cases. Using customDragHandle=true for that would have impact on current usages, hence I'm suggesting to add a third custom option as this should not be a default.

The reasoning here is this:
EuiDraggable adds a drag wrapper around its content, which has role="button|group" - this works for most cases where the drag element should be the interactive/focused element. The impact of this is, that the content of this element is not read as standalone semantic elements. E.g. role="button" removes additional semantic content and just reads visible text content.
For datagrid column headers we rather want the columnheader to still be the element to be read via screen readers as it holds all semantic information. Instead we want the draggable accessible information added to the column header instead of the wrapper being read only.
To ensure the columnheader role element is read, the draggable wrapper needs to have no role that removes the content semantics. Hence the decision to provide means to unset it for this case.

@mgadewoll mgadewoll force-pushed the datagrid/7136-draggable-column-headers branch from 9e23fdd to 2ccfb4d Compare September 13, 2024 11:51
@mgadewoll mgadewoll changed the title [DRAFT] [EuiDataGrid] Implement draggable column headers [EuiDataGrid] Implement draggable column headers Sep 13, 2024
@mgadewoll mgadewoll marked this pull request as ready for review September 13, 2024 16:05
@mgadewoll mgadewoll requested a review from a team as a code owner September 13, 2024 16:05
@mgadewoll mgadewoll marked this pull request as draft September 13, 2024 16:33
@mgadewoll mgadewoll marked this pull request as ready for review September 16, 2024 08:47
@mgadewoll mgadewoll force-pushed the datagrid/7136-draggable-column-headers branch from 141c294 to deceb15 Compare September 18, 2024 09:02
@mgadewoll mgadewoll force-pushed the datagrid/7136-draggable-column-headers branch from b428995 to 45ec4e5 Compare September 24, 2024 13:20
- draggable cells prevent onOutsideClick to be triggered, we need to manually update focus to ensure expected behavior

- moves columnResizer element to ensure drag and resize actions stay separate
- add columnDragDrop control to custom ehader story for testing with interactive headers
- ensures the columnheader element is read instead of the wrapping draggable container; this requires the draggable wrapper to not have a role as the default roles button/group remove any semantics from their content when focused which results in the content not fully being read
@mgadewoll mgadewoll force-pushed the datagrid/7136-draggable-column-headers branch from da5c4f8 to b256ff6 Compare October 7, 2024 11:45
+ add a few more focus trap/popover tests that I missed when removing the onBlur logic earlier
- this should be handled on all dragEnd events, not just drag cancel - moving force logic to EuiDroppable for cleanness

- this fix should still work for the dragCancel spec just added, but also work for the new interactive child focus on drag test just added (will fail without)
@cee-chen cee-chen force-pushed the datagrid/7136-draggable-column-headers branch from f064c2e to 26c72d5 Compare October 7, 2024 22:52
-ensures that column widths a retained even after reordering them
@mgadewoll
Copy link
Contributor Author

⚠️ One thing I noticed is that the screen reader output in JAWS changed along the way from the original state.
When starting a drag, JAWS reads the full cell element again, which is quite verbose. The information that a drag started is hence also read last - after all cell information. I'm not fully sure when this changed, this might already have been introduced with the reparenting as we're focusing different DOM elements which could trigger a full read.

I'd be fine keeping it as is for now though as it's not broken but we might want to check further on it separately if/how we could improve the output by making it less verbose.

ℹ️ NVDA output is as expected.

- this works fine in actual production / a stateful data grid

- the reason why this is now failing is because `setVisibleColumns` isn't a real function, so the columns aren't actually swapping, but `setFocusCell` is now being called on swap, which re-instantiates the animation
…op wrappers

- use a prop instead to determine last column CSS

- this was causing infinite grid width rerenders (caught by React 16 Cypress tests)

- wasn't happening in our Storybook because we had trailing control columns :sadwoah:
- also being used by the hidden copy logic, so let's just calculate it in one place in the header row and pass a boolean down rather than `visibleColCount`
- remove cache/map - reuse the existing `columnWidths` state/object instead, which essentially already serves as a map

- rename `computeColumnWidths` to `getInitialWidths` (more clear, IMO), and update it to not override any already set state via a `== null` check

+ add tests
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

As a heads up my local Docker/Loki setup isn't working so I can't run a final VRT snapshot update to confirm that nothing changed UI-wise - would super appreciate it if you could do that final check here tomorrow.

Otherwise, if CI passes and you don't have any other code changes you'd like to make, I think this is good to go!! 🤞 🎉

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@mgadewoll
Copy link
Contributor Author

As a heads up my local Docker/Loki setup isn't working so I can't run a final VRT snapshot update to confirm that nothing changed UI-wise - would super appreciate it if you could do that final check here tomorrow.

Otherwise, if CI passes and you don't have any other code changes you'd like to make, I think this is good to go!! 🤞 🎉

@cee-chen I just had a last quick check of the changes and ran the datagrid a last time in screen readers. It works great! Thanks for the great review and collaboration on this! ❤️ I learned a lot working on this 🧠

@mgadewoll mgadewoll merged commit 330efe8 into elastic:main Oct 10, 2024
5 checks passed
@mgadewoll mgadewoll deleted the datagrid/7136-draggable-column-headers branch October 14, 2024 07:38
cee-chen pushed a commit to elastic/kibana that referenced this pull request Oct 18, 2024
`v97.0.0-backport.0`⏩`v97.2.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v97.2.0`](https://github.com/elastic/eui/releases/v97.2.0)

- Updated `EuiHeaderLinks` with a new `xxs` gutter size
([#8079](elastic/eui#8079))

**Bug fixes**

- Reverted an `EuiDataGrid` regression from
[#8015](elastic/eui#8015) which prevented
overriding column widths via columns's `initialWidth`s
([#8086](elastic/eui#8086))


## [`v97.1.0`](https://github.com/elastic/eui/releases/v97.1.0)

- Added `columnVisibility.canDragAndDropColumns` on `EuiDataGrid` which
enables reordering columns via draggable header cells
([#8015](elastic/eui#8015))
- Updated `EuiHeader`s in dark mode to have a visible border-bottom
color ([#8070](elastic/eui#8070))
- Added props `minDate` and `maxDate` on `EuiSuperDatePicker` to support
restricting date range selections
([#8071](elastic/eui#8071))
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 18, 2024
`v97.0.0-backport.0`⏩`v97.2.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v97.2.0`](https://github.com/elastic/eui/releases/v97.2.0)

- Updated `EuiHeaderLinks` with a new `xxs` gutter size
([elastic#8079](elastic/eui#8079))

**Bug fixes**

- Reverted an `EuiDataGrid` regression from
[elastic#8015](elastic/eui#8015) which prevented
overriding column widths via columns's `initialWidth`s
([elastic#8086](elastic/eui#8086))

## [`v97.1.0`](https://github.com/elastic/eui/releases/v97.1.0)

- Added `columnVisibility.canDragAndDropColumns` on `EuiDataGrid` which
enables reordering columns via draggable header cells
([elastic#8015](elastic/eui#8015))
- Updated `EuiHeader`s in dark mode to have a visible border-bottom
color ([elastic#8070](elastic/eui#8070))
- Added props `minDate` and `maxDate` on `EuiSuperDatePicker` to support
restricting date range selections
([elastic#8071](elastic/eui#8071))

(cherry picked from commit d7c5608)
kibanamachine added a commit to elastic/kibana that referenced this pull request Oct 18, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [Upgrade EUI to v97.2.0
(#196397)](#196397)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Lene
Gadewoll","email":"lene.gadewoll@elastic.co"},"sourceCommit":{"committedDate":"2024-10-18T22:06:35Z","message":"Upgrade
EUI to v97.2.0
(#196397)\n\n`v97.0.0-backport.0`⏩`v97.2.0`\r\n\r\n_[Questions? Please
see our Kibana
upgrade\r\nFAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_\r\n\r\n---\r\n\r\n##
[`v97.2.0`](https://github.com/elastic/eui/releases/v97.2.0)\r\n\r\n-
Updated `EuiHeaderLinks` with a new `xxs` gutter
size\r\n([#8079](https://github.com/elastic/eui/pull/8079))\r\n\r\n**Bug
fixes**\r\n\r\n- Reverted an `EuiDataGrid` regression
from\r\n[#8015](elastic/eui#8015) which
prevented\r\noverriding column widths via columns's
`initialWidth`s\r\n([#8086](https://github.com/elastic/eui/pull/8086))\r\n\r\n\r\n##
[`v97.1.0`](https://github.com/elastic/eui/releases/v97.1.0)\r\n\r\n-
Added `columnVisibility.canDragAndDropColumns` on `EuiDataGrid`
which\r\nenables reordering columns via draggable header
cells\r\n([#8015](https://github.com/elastic/eui/pull/8015))\r\n-
Updated `EuiHeader`s in dark mode to have a visible
border-bottom\r\ncolor
([#8070](https://github.com/elastic/eui/pull/8070))\r\n- Added props
`minDate` and `maxDate` on `EuiSuperDatePicker` to
support\r\nrestricting date range
selections\r\n([#8071](https://github.com/elastic/eui/pull/8071))","sha":"d7c5608420caae76d6f307cee26d63c1cd01d381","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","EUI","v9.0.0","backport:version","v8.17.0"],"title":"Upgrade
EUI to
v97.2.0","number":196397,"url":"https://github.com/elastic/kibana/pull/196397","mergeCommit":{"message":"Upgrade
EUI to v97.2.0
(#196397)\n\n`v97.0.0-backport.0`⏩`v97.2.0`\r\n\r\n_[Questions? Please
see our Kibana
upgrade\r\nFAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_\r\n\r\n---\r\n\r\n##
[`v97.2.0`](https://github.com/elastic/eui/releases/v97.2.0)\r\n\r\n-
Updated `EuiHeaderLinks` with a new `xxs` gutter
size\r\n([#8079](https://github.com/elastic/eui/pull/8079))\r\n\r\n**Bug
fixes**\r\n\r\n- Reverted an `EuiDataGrid` regression
from\r\n[#8015](elastic/eui#8015) which
prevented\r\noverriding column widths via columns's
`initialWidth`s\r\n([#8086](https://github.com/elastic/eui/pull/8086))\r\n\r\n\r\n##
[`v97.1.0`](https://github.com/elastic/eui/releases/v97.1.0)\r\n\r\n-
Added `columnVisibility.canDragAndDropColumns` on `EuiDataGrid`
which\r\nenables reordering columns via draggable header
cells\r\n([#8015](https://github.com/elastic/eui/pull/8015))\r\n-
Updated `EuiHeader`s in dark mode to have a visible
border-bottom\r\ncolor
([#8070](https://github.com/elastic/eui/pull/8070))\r\n- Added props
`minDate` and `maxDate` on `EuiSuperDatePicker` to
support\r\nrestricting date range
selections\r\n([#8071](https://github.com/elastic/eui/pull/8071))","sha":"d7c5608420caae76d6f307cee26d63c1cd01d381"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196397","number":196397,"mergeCommit":{"message":"Upgrade
EUI to v97.2.0
(#196397)\n\n`v97.0.0-backport.0`⏩`v97.2.0`\r\n\r\n_[Questions? Please
see our Kibana
upgrade\r\nFAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_\r\n\r\n---\r\n\r\n##
[`v97.2.0`](https://github.com/elastic/eui/releases/v97.2.0)\r\n\r\n-
Updated `EuiHeaderLinks` with a new `xxs` gutter
size\r\n([#8079](https://github.com/elastic/eui/pull/8079))\r\n\r\n**Bug
fixes**\r\n\r\n- Reverted an `EuiDataGrid` regression
from\r\n[#8015](elastic/eui#8015) which
prevented\r\noverriding column widths via columns's
`initialWidth`s\r\n([#8086](https://github.com/elastic/eui/pull/8086))\r\n\r\n\r\n##
[`v97.1.0`](https://github.com/elastic/eui/releases/v97.1.0)\r\n\r\n-
Added `columnVisibility.canDragAndDropColumns` on `EuiDataGrid`
which\r\nenables reordering columns via draggable header
cells\r\n([#8015](https://github.com/elastic/eui/pull/8015))\r\n-
Updated `EuiHeader`s in dark mode to have a visible
border-bottom\r\ncolor
([#8070](https://github.com/elastic/eui/pull/8070))\r\n- Added props
`minDate` and `maxDate` on `EuiSuperDatePicker` to
support\r\nrestricting date range
selections\r\n([#8071](https://github.com/elastic/eui/pull/8071))","sha":"d7c5608420caae76d6f307cee26d63c1cd01d381"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Lene Gadewoll <lene.gadewoll@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDataGrid] Implement draggable columns with table headers
5 participants