Overhaul EuiDataGrid's cell focus states #3128

chandlerprall · 2020-03-19T16:40:41Z

Summary

This is ready to review functionality, not yet the code side.

Added a new docs page for data grid, Data grid focus, to track requirements and provide a datagrid for testing these states. This also touched EuiFocusTrap's outside-click-disabling, so we'll want to double check that (added ability to re-enable a trap which was previously-disabled-by-click).

I also want to diagram/record the focus logic and refactor it, but that has to wait until the logic is solidified and should be in another PR.

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~

Checked in mobile
Checked in IE11 and Firefox
- [ ] Props have proper autodocs
~~- [ ] Added documentation examples~~
Added or updated jest tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2020-03-19T17:42:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

myasonik · 2020-03-19T20:22:30Z

When tabbing into the grid for the first time, focus skips all the toolbar items.

Full steps

Start tabbing at the top of the page (tab through "demo", "demo js", and "demo html"

The bug

Tabbing from "demo html" goes directly to the first cell instead of the first toolbar item

Note: Only tested in Firefox

myasonik · 2020-03-19T20:41:11Z

OK, I've got a very odd bug...

Full steps

Load page
Use mouse to adjust column width
Click off the data grid
Start tabbing (tab through "demo", "demo js", keep tabbing through all of the toolbar controls)

THE BUG

After the last toolbar control but before the first cell, notice an extra tab stop where focus is seems to be set on the whole grid

Note: Only tested in Firefox

myasonik · 2020-03-19T20:41:55Z

A feature request for the test page (which I otherwise love!) — a toggle on the page to make the columns interactive

chandlerprall · 2020-03-23T14:38:30Z

When tabbing into the grid for the first time, focus skips all the toolbar items.

Fixed, was targeting the wrong div

After the last toolbar control but before the first cell, notice an extra tab stop where focus is seems to be set on the whole grid

FireFox allows tabbing to a scrollable element. I've added an explicit tabindex=-1 to disable that.

A feature request for the test page (which I otherwise love!) — a toggle on the page to make the columns interactive

Not sure I know exactly what you mean; I take this to mean having 3 columns with no interactive elements and the toggle show/hides interactive elements into those?

kibanamachine · 2020-03-23T14:54:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

myasonik · 2020-03-23T16:12:47Z

Confirmed that I can't recreate those bugs anymore! 🎉

Don't know if this is something you want to handle here or as part of another ticket (as it's not introduced as part of this PR):
If a cell is partially cut off by the scrolling container, there's no way to see the whole thing with just a keyboard. If the cell is completely cut off, the browser will scroll it into view for you.

We should scroll the container to completely show the currently focused cell.

chandlerprall · 2020-03-23T16:57:46Z

If a cell is partially cut off by the scrolling container, there's no way to see the whole thing with just a keyboard. If the cell is completely cut off, the browser will scroll it into view for you.

I noticed this when testing in FF yesterday (Chrome works as expected). Let's deal with that in another issue. Mind creating it?

…case

chandlerprall · 2020-03-23T17:29:49Z

Pushed a toggle to enable/disable interactive headers in the focus example, which caught another focus bug.

kibanamachine · 2020-03-23T17:48:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

…olumns

myasonik · 2020-03-23T18:53:46Z

Not 100% sure what the interaction here should be so just bringing this up for discussion

Steps

Arrow over to a cell with 1 interactive item that is not expandable.
tab out and back into the grid

Bug?

Focus is set on the cell, not on the content as it is when arrowing around in the grid.

(You can also recreate this effect by clicking in the cell but not on the interactive item itself.)

myasonik · 2020-03-23T19:03:10Z

Advanced keyboard control bugs

Using F2 should be the same as using Enter when the cell is not expanded (this works as expected) and the same as Esc when it is expanded (this does not work)
Page Up and Page Down seems to have broken (should set focus to the first/last row in the same column you're currently in

snide · 2020-03-24T20:36:41Z

I'm doing a cleanup pass on the docs for this as part of my review. Functionally it seems to make sense. Issues that I see in it such as how to tab through truncated items I think are solvable by judicious use of other bits of functionality in Data Grid such as set widths, expandable cells...etc.

kibanamachine · 2020-04-02T15:41:29Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

snide · 2020-04-02T15:48:44Z

Sorry, the commit reminded me I still have a todo here. I'll try and get it done this week. I didn't see any major issues functionally. If @myasonik is OK with the state of this PR, I can always PR after since my changes were just to docs.

kibanamachine · 2020-04-02T16:16:22Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

chandlerprall · 2020-04-02T16:32:05Z

@myasonik all 3 bugs are now fixed!

kibanamachine · 2020-04-02T16:50:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

chandlerprall · 2020-04-02T20:06:56Z

After iterating with @myasonik , page up & page down will now always shift focus to the bottom or top (respectively) of the page, not only when actually paging. Also added pagination to the focus example for easier testing.

kibanamachine · 2020-04-02T20:23:55Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

myasonik

After playing around with it, I think this is better than what we had for sure.

I wonder if I might still expect that if you're on the first/last page that page up/down would bring you to the top/bottom of the grid. (Technically becoming not-like the rest but my brain keeps thinking that's how it should work.)

Either way, it's all up to interpretation and feelings now so I'm good to ship. We can open up a followup ticket if we want to discuss it or wait and see if anyone else opens up issues.

🚀

thompsongl · 2020-04-03T15:54:26Z

This is ready to review functionality, not yet the code side.

Let me know when this is ready, @chandlerprall

chandlerprall · 2020-04-03T16:46:02Z

@thompsongl this now looks stable for review, thanks!

thompsongl

Code LGTM. I also (successfully) ran through the scenarios in the docs for good measure

src/components/datagrid/data_grid.tsx

thompsongl · 2020-04-03T20:33:25Z

src/components/focus_trap/focus_trap.tsx

@@ -57,6 +57,13 @@ export class EuiFocusTrap extends Component<Props, State> {
    this.setInitialFocus(this.props.initialFocus);
  }

+  componentDidUpdate(prevProps: Props) {
+    if (prevProps.disabled === true && this.props.disabled === false) {
+      // eslint-disable-next-line react/no-did-update-set-state


Wondering if we should open an issue to convert this to a function component. Avoiding this is probably worth it.

TBH I've never cared about this react lint rule. We should convert for the sake of converting, at some point. though. Helps avoid these types of bugs

snide

Pushed a docs cleanup and did some design stuff on the example. LGTM.

chandlerprall · 2020-04-03T22:39:02Z

Will merge on green CI. Thanks everyone!

kibanamachine · 2020-04-03T22:58:49Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3128/

chandlerprall added 3 commits March 18, 2020 22:19

Initial datagrid focus changes, added a focus example page

5a15aa8

improved example, down to last case

9a610e3

Overhauled datagrid cell focus rules

13bd302

chandlerprall added the data grid label Mar 19, 2020

chandlerprall requested review from snide, thompsongl and myasonik March 19, 2020 16:40

chandlerprall added bug feature request labels Mar 19, 2020

lint fixes

af55ec6

Fix focus bugs found in review

326319f

Refactored datagrid focus example; fixed EuiDataGridHeaderCell focus …

55f6923

…case

Limit header interaction in datagrid focus example to only a couple c…

d616644

…olumns

Fixes various datagrid focus bugs

6031723

Fix bug in datagrid expansion popover toggling with f2

5e158aa

Fix page up & page down interactions on datagrid

276939f

Updated datagrid page up/down to always shift focus, even on 'end' pages

8f09d5b

myasonik approved these changes Apr 2, 2020

View reviewed changes

thompsongl approved these changes Apr 3, 2020

View reviewed changes

snide and others added 3 commits April 3, 2020 15:32

clean up focus docs

0f6151f

responsive fix

5652475

PR feedback

61c5f3c

snide approved these changes Apr 3, 2020

View reviewed changes

chandlerprall merged commit 24bfdb7 into elastic:master Apr 3, 2020

chandlerprall deleted the bug/2626-datagrid-cell-focus branch April 3, 2020 23:08

Tocknicsu mentioned this pull request May 22, 2020

DataGrid in Modal performance #3509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul EuiDataGrid's cell focus states #3128

Overhaul EuiDataGrid's cell focus states #3128

chandlerprall commented Mar 19, 2020 •

edited

Loading

kibanamachine commented Mar 19, 2020

myasonik commented Mar 19, 2020 •

edited

Loading

myasonik commented Mar 19, 2020

myasonik commented Mar 19, 2020

chandlerprall commented Mar 23, 2020

kibanamachine commented Mar 23, 2020

myasonik commented Mar 23, 2020

chandlerprall commented Mar 23, 2020

chandlerprall commented Mar 23, 2020

kibanamachine commented Mar 23, 2020

myasonik commented Mar 23, 2020

myasonik commented Mar 23, 2020

snide commented Mar 24, 2020

kibanamachine commented Apr 2, 2020

snide commented Apr 2, 2020

kibanamachine commented Apr 2, 2020

chandlerprall commented Apr 2, 2020

kibanamachine commented Apr 2, 2020

chandlerprall commented Apr 2, 2020

kibanamachine commented Apr 2, 2020

myasonik left a comment

thompsongl commented Apr 3, 2020

chandlerprall commented Apr 3, 2020

thompsongl left a comment

thompsongl Apr 3, 2020

chandlerprall Apr 3, 2020

snide left a comment

chandlerprall commented Apr 3, 2020

kibanamachine commented Apr 3, 2020

Overhaul EuiDataGrid's cell focus states #3128

Overhaul EuiDataGrid's cell focus states #3128

Conversation

chandlerprall commented Mar 19, 2020 • edited Loading

Summary

Checklist

kibanamachine commented Mar 19, 2020

myasonik commented Mar 19, 2020 • edited Loading

Full steps

The bug

myasonik commented Mar 19, 2020

Full steps

THE BUG

myasonik commented Mar 19, 2020

chandlerprall commented Mar 23, 2020

kibanamachine commented Mar 23, 2020

myasonik commented Mar 23, 2020

chandlerprall commented Mar 23, 2020

chandlerprall commented Mar 23, 2020

kibanamachine commented Mar 23, 2020

myasonik commented Mar 23, 2020

Steps

Bug?

myasonik commented Mar 23, 2020

Advanced keyboard control bugs

snide commented Mar 24, 2020

kibanamachine commented Apr 2, 2020

snide commented Apr 2, 2020

kibanamachine commented Apr 2, 2020

chandlerprall commented Apr 2, 2020

kibanamachine commented Apr 2, 2020

chandlerprall commented Apr 2, 2020

kibanamachine commented Apr 2, 2020

myasonik left a comment

Choose a reason for hiding this comment

thompsongl commented Apr 3, 2020

chandlerprall commented Apr 3, 2020

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl Apr 3, 2020

Choose a reason for hiding this comment

chandlerprall Apr 3, 2020

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

chandlerprall commented Apr 3, 2020

kibanamachine commented Apr 3, 2020

chandlerprall commented Mar 19, 2020 •

edited

Loading

myasonik commented Mar 19, 2020 •

edited

Loading