elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.09k stars 838 forks source link

[EuiDataGrid] Page Up / Page Down behavior #5450

Closed kertal closed 9 months ago

kertal commented 2 years ago

When using data grid page up and page down keys lead to switching to next next/previous page. This makes sense when all records are displayed. However, once the list is scrollable we might think of changing/ improving this behavior.

If the user e.h. has 100 records per page configured, but there are just 20 visible, so the user has to scroll, page down could be used to scroll to the next 20 records, until the bottom of the 100 records was reached. Then the subsequent page down could switch to the next page showing 100 - 120.

As an alternative we could implement this scrolling behavior only if no pagination was used, which might be the clearer approach.

Brought up here: https://github.com/elastic/kibana/issues/120180

miukimiu commented 2 years ago

Thanks, @kertal for opening this issue. I tested in Discover the data grid and it felt odd to press page down and nothing happens to the scrollbar. My expectation as a user was to jump down a number of rows and not just change the page.

I wouldn't say this is a bug because the way the paginations were first created was to jump to the previous/next page on page up or page down.

It feels to me that this is an enhancement. What do you think @chandlerprall @constancecchen?

chandlerprall commented 2 years ago

According to WAI-ARIA Authoring Practices 1.1's Keyboard Interaction For Data Grids says:

Page Down: Moves focus down an author-determined number of rows, typically scrolling so the bottom row in the currently visible set of rows becomes one of the first visible rows. If focus is in the last row of the grid, focus does not move. Page Up: Moves focus up an author-determined number of rows, typically scrolling so the top row in the currently visible set of rows becomes one of the last visible rows. If focus is in the first row of the grid, focus does not move.

Checking the historical context from a datagrid a11y spec Michail had put together for initial development & testing, we listed:

[page_down] scrolls the grid, page, or both so that the currently visible bottom most row because the top most; sets focus to the cell in the same column in the new top most row [page_up] opposite of [page_down]

Which seems to be in agreement with WAI ARIA plus the potential interaction with pagination. I've been unable to find anything that discusses these key usages in a paginated experience (grids or tables), so I'm thinking we should either remove the ability for page up/down to paginate, or only do so if focus has already moved to the (first|last) row. Meaning, with a grid that has pages of 20 items with only 10 visible in the container:

@1Copenut is there anything else we should be considering?

1Copenut commented 2 years ago

Thanks for linking the WAI-ARIA authoring practices for data grids @chandlerprall. I read through that spec and feel it's got the bases covered for pagination. I did go down the rabbit hole a bit and read more about navigating within cells, but that's another ticket. I'll add my findings there.

chandlerprall commented 2 years ago

I did go down the rabbit hole a bit and read more about navigating within cells, but that's another ticket. I'll add my findings there.

Those should be implemented, we've been doing manual testing against https://elastic.github.io/eui/#/tabular-content/data-grid-focus but now with Cypress we can add in regression tests

github-actions[bot] commented 1 year ago

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

github-actions[bot] commented 9 months ago

❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context.