[EuiDataGrid] Fix column header move left focus bug #7698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- index being passed to `setFocusCell` wasn't accounting for leading control columns
- fix doesn't work in Kibana without pulling `setFocusedCell()` out to async, either a timeout or rAF work
cee-chen
commented
Apr 18, 2024
Comment on lines
+162
to
+165
| // Wait a beat to move focus, otherwise the EuiPopover's EuiFocusTrap's | ||
| // returnFocus logic sometimes steals it (depending on rerenders) | ||
| setTimeout(() => { | ||
| setFocusedCell([columnFocusIndex + newIndex, -1]); // -1 is the static y-index of the header |
There was a problem hiding this comment.
🥲 I honestly don't know why the fix doesn't work in Kibana without the setTimeout. But I also don't know why we can't repro this in EUI local, so this probably isn't the worst??
There was a problem hiding this comment.
Maybe something on Kibana is executed with a setTimeout or some delay too which interferes? 🤔
I think it's ok a use setTimeout here for this case. We might just need to keep an eye out if there are any additional ripple effects or if there are similar issues popping up due to some general usage issue on Kibana?
cee-chen
changed the title
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
|
FWIW, you can also reproduce elastic/kibana#177527 without Kibana by running the |
mgadewoll
approved these changes
Apr 18, 2024
Comment on lines
+162
to
+165
| // Wait a beat to move focus, otherwise the EuiPopover's EuiFocusTrap's | ||
| // returnFocus logic sometimes steals it (depending on rerenders) | ||
| setTimeout(() => { | ||
| setFocusedCell([columnFocusIndex + newIndex, -1]); // -1 is the static y-index of the header |
There was a problem hiding this comment.
Maybe something on Kibana is executed with a setTimeout or some delay too which interferes? 🤔
I think it's ok a use setTimeout here for this case. We might just need to keep an eye out if there are any additional ripple effects or if there are similar issues popping up due to some general usage issue on Kibana?
That's totally bizarre 😖 I wonder if it's a dev mode vs prod mode thing?? I tried adding |
|
Merging - talked through with Jason on Slack about what's causing the prod vs dev shenanigans (likely the focus trap's return focus behavior) |
This was referenced Apr 18, 2024
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
May 3, 2024
`v94.1.0-backport.0` ⏩ `v94.2.1-backport.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)_ --- ## [`v94.2.1-backport.0`](https://github.com/elastic/eui/releases/v94.2.1-backport.0) **This is a backport release only intended for use by Kibana.** - Reverted the `EuiFlexGroup`/`EuiFlexItem` `component` prop feature due to Kibana typing issues ## [`v94.2.1`](https://github.com/elastic/eui/releases/v94.2.1) **Bug fixes** - Fixed an `EuiTabbedContent` edge case bug that occurred when updated with a completely different set of `tabs` ([#7713](elastic/eui#7713)) - Fixed the `@storybook/test` dependency to be listed in `devDependencies` and not `dependencies` ([#7719](elastic/eui#7719)) ## [`v94.2.0`](https://github.com/elastic/eui/releases/v94.2.0) - Updated `getDefaultEuiMarkdownPlugins()` to allow excluding the following plugins in addition to `tooltip`: ([#7676](elastic/eui#7676)) - `checkbox` - `linkValidator` - `lineBreaks` - `emoji` - Updated `EuiSelectable`'s `isPreFiltered` prop to allow passing a configuration object, which allows disabling search highlighting in addition to search filtering ([#7683](elastic/eui#7683)) - Updated `EuiFlexGroup` and `EuiFlexItem` prop types to support passing any valid React component type to the `component` prop and ensure proper type checking of the extra props forwarded to the `component`. ([#7688](elastic/eui#7688)) - Updated `EuiSearchBar` to allow the `@` special character in query string searches ([#7702](elastic/eui#7702)) - Added a new, optional `optionMatcher` prop to `EuiSelectable` and `EuiComboBox` allowing passing a custom option matcher function to these components and controlling option filtering for given search string ([#7709](elastic/eui#7709)) **Bug fixes** - Fixed an `EuiPageTemplate` bug where prop updates would not cascade down to child sections ([#7648](elastic/eui#7648)) - To cascade props down to the sidebar, `EuiPageTemplate` now explicitly requires using the `EuiPageTemplate.Sidebar` rather than `EuiPageSidebar` - Fixed `EuiFieldNumber`'s typing to accept an icon configuration shape ([#7666](elastic/eui#7666)) - Fixed `EuiFieldText` and `EuiFieldNumber` to render the correct paddings for icon shapes set to `side: 'right'` ([#7666](elastic/eui#7666)) - Fixed `EuiFieldText` and `EuiFieldNumber` to fully ignore `icon`/`prepend`/`append` when `controlOnly` is set to true ([#7666](elastic/eui#7666)) - Fixed `EuiColorPicker`'s input not setting the correct right padding for the number of icons displayed ([#7666](elastic/eui#7666)) - Visual fixes for `EuiRange`s with `showInput`: ([#7678](elastic/eui#7678)) - Longer `append`/`prepend` labels no longer cause a background bug - Inputs can no longer overwhelm the actual range in width - Fixed a visual text alignment regression in `EuiTableRowCell`s with the `row` header scope ([#7681](elastic/eui#7681)) - Fixed `toolTipProps` type on `EuiSuperUpdateButton` to use `Partial<EuiToolTipProps>` ([#7692](elastic/eui#7692)) - Fixes missing prop type for `popperProps` on `EuiDatePicker` ([#7694](elastic/eui#7694)) - Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns` when moving columns to the left/right ([#7701](elastic/eui#7701)) ([#7698](elastic/eui#7698)) - Fixed `EuiSuperDatePicker` to validate date string with respect of locale on `EuiAbsoluteTab`. ([#7705](elastic/eui#7705)) - Fixed a visual bug with `EuiSuperDatePicker`'s absolute tab on small mobile screens ([#7708](elastic/eui#7708)) - Fixed i18n of empty and loading state messages for the `FieldValueSelectionFilter` component ([#7718](elastic/eui#7718)) **Dependency updates** - Updated `@hello-pangea/dnd` to v16.6.0 ([#7599](elastic/eui#7599)) - Updated `remark-rehype` to v8.1.0 ([#7601](elastic/eui#7601)) **Accessibility** - Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes to have unique aria-labels per row ([#7672](elastic/eui#7672)) - Added `aria-valuetext` attributes to `EuiRange`s with tick labels for improved screen reader UX ([#7675](elastic/eui#7675)) - Updated `EuiAccordion` to keep focus on accordion trigger instead of moving to content on click/keypress ([#7696](elastic/eui#7696)) - Added `aria-disabled` attribute to `EuiHorizontalSteps` when status is "disabled" ([#7699](elastic/eui#7699)) --------- Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
cee-chen
added a commit
that referenced
this pull request
May 8, 2024
cee-chen
added a commit
that referenced
this pull request
May 8, 2024
cee-chen
added a commit
to elastic/kibana
that referenced
this pull request
May 8, 2024
`v93.6.0` ⏩ `v93.6.0-backport.0` --- ## [`v93.6.0-backport.0`](https://github.com/elastic/eui/releases/v93.6.0-backport.0) **This is a backport release only intended for use by Kibana.** - Updated `EuiSearchBar` to allow the `@` special character in query string searches ([#7702](elastic/eui#7702)) **Bug fixes** - Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns` when moving columns to the left/right ([#7701](elastic/eui#7701)) ([#7698](elastic/eui#7698)) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Should resolve both elastic/kibana#177527 and elastic/kibana#180064
I apparently attempted to resolve this in #5530 but past-me didn't account for two things:
returnFocuslogic interfering withsetFocusedCellQA
Since I can't seem to repro this at all in EUI, it has to be QA'd in Kibana 🫠
yarn build-pack, wait for.tgzoutputRegression testing
General checklist
- [ ] Checked in both light and dark modes- [ ] Checked in mobileand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)