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

[EuiPageHeader] rightSideItems Update DOM to Match Display Order #6753

Merged
merged 10 commits into from
May 5, 2023

Conversation

breehall
Copy link
Contributor

@breehall breehall commented May 3, 2023

Fixes #6693

Summary

EuiPageHeader intentionally sets the content of the header to have a flex-direction of row-reverse. This is causing the last element of the within the array of items to catch focus first (via screenreader and keyboard).

This PR takes the responsibility of display order away from flex. Instead, the array is cloned and reversed so that the DOM matches the display order. Now, when accessed by keyboard, the first element in the list catches focus first.

Before & After
Screen.Recording.2023-05-03.at.5.02.13.PM.mov
Screen.Recording.2023-05-03.at.5.02.54.PM.mov

QA

View the Page Header component in the PR preview. Use a keyboard to navigate the examples. For each, the Do Something button should catch focus before the Add Something

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress 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

…ghtSideItems. This causes the list to be displayed in one order and show as reversed with CSS, causing screenreaders and keyboards to focus on the last element in the list first. Added a prop to the wrapWithFlex function that will ensure the DOM order matches the display order of rightSideItems.
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

Comment on lines +44 to 58
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snapshot change is expected. Previously with flex, in the DOM the array would be documented as [1,2], but displayed as [2,1].

Now the DOM order matches the display order.

@breehall breehall requested a review from 1Copenut May 4, 2023 13:44
@breehall breehall marked this pull request as ready for review May 4, 2023 13:44
@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

@breehall I reviewed this in VoiceOver + Safari and on the keyboard in Chrome. Everything behaved correctly for keyboard and screenreader native navigation. No issues in axe browser plugin.

I offered one small code refactor suggestion. It's just that, an opinionated suggestion, so feel free to accept or keep your original implementation. It took me a beat longer to reason about the reverse logic, so I passed reverse through to your if / else block and added the reverse() Array method where appropriate. Unit tests and snapshots passed, so either way.

LMK what you feel is the best implementation and I'll approve. Thank you!

src/components/page/page_header/page_header_content.tsx Outdated Show resolved Hide resolved
Comment on lines 348 to 349
const reverseRightSideItems = reverse
? rightSideItems.reverse()
Copy link
Member

Choose a reason for hiding this comment

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

.reverse() mutates the array in place. Do not ever call it without shallow cloning the array first, especially on arrays coming from consumers (vs arrays we define/control).

Suggested change
const reverseRightSideItems = reverse
? rightSideItems.reverse()
const reverseRightSideItems = reverse
? [...rightSideItems].reverse()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do not ever call it without shallow cloning the array first

Thank you for this and duly noted!

Copy link
Member

@cee-chen cee-chen May 4, 2023

Choose a reason for hiding this comment

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

To clarify, I was being a little didactic with the initial "do not ever". If we're the ones defining an array and know we want to simply mutate/reverse the array in place, great, mutate away (although some schools of thought in JS would still say don't do this :).

In this case rightSideItems is not an array we define/own and we do not want ever want to mutate consumer data.

breehall and others added 3 commits May 4, 2023 13:51
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@breehall breehall requested a review from cee-chen May 4, 2023 20:26
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.

One last code cleanup change request and I think this is good to go!

src/components/page/page_header/page_header_content.tsx Outdated Show resolved Hide resolved
src/components/page/page_header/page_header_content.tsx Outdated Show resolved Hide resolved
breehall and others added 4 commits May 4, 2023 19:02
… testing on smaller screens"

This reverts commit 12aaaea.

Reverting example change that was only intended for testing
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
… into pageheader/right-side

Pulling changes that were made in Github UI
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.

LGTM once tests pass!

@kibanamachine
Copy link

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

@breehall breehall merged commit b7eb120 into elastic:main May 5, 2023
jbudz pushed a commit to elastic/kibana that referenced this pull request May 15, 2023
## Summary

`eui@77.2.2` ⏩ `eui@79.0.1`

🦴 The primary changes in this upgrade are around the deprecated
`EuiLoadingContent` being removed in favor of `EuiSkeletonText`.
- Most instances have been a [direct swap of
usage](327626a),
but [some replacements were a bit more
opinionated](e6ceb36)
as I saw them as potential to take advantage of `EuiSkeletonText`'s
syntactical sugar and screen reader announcements for when state
switches to loaded.

---

## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1)

**Bug fixes**

- Fixed broken push `EuiFlyout` behavior
([#6764](elastic/eui#6764))


## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0)

- Updated all `EuiSkeleton` components with new props that allow for
more control over screen reader live announcements:
`announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps`
([#6752](elastic/eui#6752))
- Improved keyboard accessibility in `EuiPageHeader` by ensuring the
right side menu items come into focus from left to right.
([#6753](elastic/eui#6753))

**Breaking changes**

- Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton`
components instead. ([#6754](elastic/eui#6754))


## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0)

- Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and
`EuiSwitch` in their unchecked states to meet WCAG AA guidelines.
([#6729](elastic/eui#6729))
- Added React Testing Library `*ByTestSubject` custom commands to
`within()`. RTL utilities can be imported from
`@elastic/eui/lib/test/rtl`.
([#6737](elastic/eui#6737))
- Updated `EuiAvatar` to support a new letter `casing` prop that allow
customizing text capitalization
([#6739](elastic/eui#6739))
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration
(now defaults to `padding`)
([#6744](elastic/eui#6744))

**Bug fixes**

- Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL
and query string generation
([#6717](elastic/eui#6717))
- Fixed `EuiFieldNumber`'s native browser validity detection causing
extra unnecessary rerenders
([#6741](elastic/eui#6741))
- Fixed the `scrollLock` property on `EuiFocusTrap` (and other
components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to
no longer block scrolling on nested portalled content, such as combobox
dropdowns ([#6744](elastic/eui#6744))

**Breaking changes**

- `EuiAvatar`s with the default `user` type will now default to
capitalizing all initials in uppercase
([#6739](elastic/eui#6739))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jasonrhodes pushed a commit to elastic/kibana that referenced this pull request May 17, 2023
## Summary

`eui@77.2.2` ⏩ `eui@79.0.1`

🦴 The primary changes in this upgrade are around the deprecated
`EuiLoadingContent` being removed in favor of `EuiSkeletonText`.
- Most instances have been a [direct swap of
usage](327626a),
but [some replacements were a bit more
opinionated](e6ceb36)
as I saw them as potential to take advantage of `EuiSkeletonText`'s
syntactical sugar and screen reader announcements for when state
switches to loaded.

---

## [`79.0.1`](https://github.com/elastic/eui/tree/v79.0.1)

**Bug fixes**

- Fixed broken push `EuiFlyout` behavior
([#6764](elastic/eui#6764))


## [`79.0.0`](https://github.com/elastic/eui/tree/v79.0.0)

- Updated all `EuiSkeleton` components with new props that allow for
more control over screen reader live announcements:
`announceLoadingStatus`, `announceLoadedStatus`, and `ariaLiveProps`
([#6752](elastic/eui#6752))
- Improved keyboard accessibility in `EuiPageHeader` by ensuring the
right side menu items come into focus from left to right.
([#6753](elastic/eui#6753))

**Breaking changes**

- Removed deprecated `EuiLoadingContent`. Use the `EuiSkeleton`
components instead. ([#6754](elastic/eui#6754))


## [`78.0.0`](https://github.com/elastic/eui/tree/v78.0.0)

- Improved the contrast ratio of `EuiCheckbox`, `EuiRadio`, and
`EuiSwitch` in their unchecked states to meet WCAG AA guidelines.
([#6729](elastic/eui#6729))
- Added React Testing Library `*ByTestSubject` custom commands to
`within()`. RTL utilities can be imported from
`@elastic/eui/lib/test/rtl`.
([#6737](elastic/eui#6737))
- Updated `EuiAvatar` to support a new letter `casing` prop that allow
customizing text capitalization
([#6739](elastic/eui#6739))
- Updated `EuiFocusTrap` to support the `gapMode` prop configuration
(now defaults to `padding`)
([#6744](elastic/eui#6744))

**Bug fixes**

- Fixed inconsistency in `EuiSearchBar`'s AND/OR semantics between DSL
and query string generation
([#6717](elastic/eui#6717))
- Fixed `EuiFieldNumber`'s native browser validity detection causing
extra unnecessary rerenders
([#6741](elastic/eui#6741))
- Fixed the `scrollLock` property on `EuiFocusTrap` (and other
components using `EuiFocusTrap`, such as `EuiFlyout` and `EuiModal`) to
no longer block scrolling on nested portalled content, such as combobox
dropdowns ([#6744](elastic/eui#6744))

**Breaking changes**

- `EuiAvatar`s with the default `user` type will now default to
capitalizing all initials in uppercase
([#6739](elastic/eui#6739))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@breehall breehall deleted the pageheader/right-side branch October 6, 2023 16:04
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.

[EuiPageHeader] The focus order of rightSideItems does not correspond to the logical processing sequence
5 participants