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][CRA] Fix header cell focus when moving columns left/right #7701

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Apr 18, 2024

(Note: #7698 fixed the issue for Kibana, but the underlying issue still persists for CRA and likely CodeSandbox - potentially due to various prod vs dev bundled race conditions)

Before After
before after

Problem

We noticed that focus was not updating visually after moving columns left or right in certain environments in this PR: #7698.

If you initiate a "Move left" from column 4 -- you're essentially swapping columns. Column 4 becomes moves to position 3 and column 3 moves to position 4.

When you click on the header cell to initiate that action, the column 4 header cell gets focus. After column 4 moves to position 3, we make a call to the focus service to let it know that the column 3 header cell should now have focus.

Subsequently, that service makes a call to the column 3 header to set its isFocused property to true, to reflect that it's now focused. If that cell was not already focused, updating isFocused will also trigger a call to the underlying element's .focus() to focus it.

In this case, this particular column's isFocused property is already set to true, because it just moved from position 4 to 3. So when its isFocused property is updated, it detects that it already has focus, so it doesn't make a call to .focus() to focus it.

However, something happens in between there that forces the focus away in the browser (likely the focus trap associated with the action's menu in the header cell). So the header cell's isFocused prop is essentially out of sync with the browser. The cell thinks that it's already focused, even though it's not, so it simply loses focus.

Solution

This PR resolves that issue by calling .focus() anytime setFocus is set, anticipating that there may be cases when the browser gets out of sync with this property.

QA

  • (In EUI) yarn build-pack
  • (in CRA) npx create-react-app my-app
  • Follow the getting started guide for installing EUI (but point to to the built tgz file instead of @elastic/eui
  • Copy the first basic datagrid example here (any examples where columns can be moved left/right also suffice) into the app
  • yarn start and confirm that clicking left/right in the CRA works as expected, instead of losing focus

General checklist

  • Browser QA
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
      - [ ] Checked in both light and dark modes
      - [ ] Checked in mobile
  • Docs site QA - N/A, bugfix
  • Code quality checklist - N/A, appears to only affect production builds
  • Release checklist - merging existing changelog
  • Designer checklist - N/A

@cee-chen cee-chen changed the title Quick fix [EuiDataGrid] Fix cells moving left/right on CRA Apr 24, 2024
@cee-chen cee-chen changed the title [EuiDataGrid] Fix cells moving left/right on CRA [EuiDataGrid][CRA] Fix header cell focus when moving columns left/right Apr 24, 2024
@cee-chen
Copy link
Contributor

Going to skip the changelog on this and merge it with #7698's changelog!

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen marked this pull request as ready for review April 25, 2024 19:15
@cee-chen cee-chen requested a review from a team as a code owner April 25, 2024 19:15
@cee-chen
Copy link
Contributor

Thanks again for catching this Jason!

@cee-chen cee-chen merged commit e821c9f into elastic:main Apr 25, 2024
12 checks passed
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>
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request May 3, 2024
…ht (elastic#7701)

Co-authored-by: Cee Chen <constance.chen@elastic.co>
cee-chen added a commit that referenced this pull request May 8, 2024
…ht (#7701)

Co-authored-by: Cee Chen <constance.chen@elastic.co>
cee-chen added a commit that referenced this pull request May 8, 2024
…ht (#7701)

Co-authored-by: Cee Chen <constance.chen@elastic.co>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants