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.

Sign up for GitHub

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

Jump to bottom

fix: [Obs Services > Create Service Group modal][KEYBOARD]: Focus should not be auto-set on first input when modal appears #194696

Merged
merged 8 commits into from
Oct 7, 2024
Merged

fix: [Obs Services > Create Service Group modal][KEYBOARD]: Focus should not be auto-set on first input when modal appears #194696

merged 8 commits into from
Oct 7, 2024

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Oct 2, 2024 •
edited by kibanamachine
Loading

Closes: #194965
Closes: #194966

Description

Changes Made

  1. Removed:
- inputRef.current?.focus(); // autofocus on initial render
  1. Added aria-labelledby={modalTitleId} for EuiModal. See https://eui.elastic.co/#/layout/modal.
  2. Slightly updated Name and Color validation.

Screen

Screen.Recording.2024-10-02.at.15.32.49.mov

@alexwizp alexwizp added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes v9.0.0 labels Oct 2, 2024
@alexwizp
Copy link
Contributor Author

alexwizp commented Oct 2, 2024

/ci

@alexwizp
Copy link
Contributor Author

alexwizp commented Oct 2, 2024

/ci

@alexwizp alexwizp marked this pull request as ready for review October 2, 2024 15:45
@alexwizp alexwizp requested a review from a team as a code owner October 2, 2024 15:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-accessibility (Project:Accessibility)

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Oct 2, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

cauemarcondes
cauemarcondes reviewed Oct 7, 2024
View reviewed changes
...ility_solution/apm/public/components/app/service_groups/service_group_save/group_details.tsx Show resolved Hide resolved
...ility_solution/apm/public/components/app/service_groups/service_group_save/group_details.tsx Show resolved Hide resolved
...ility_solution/apm/public/components/app/service_groups/service_group_save/group_details.tsx Show resolved Hide resolved
...ility_solution/apm/public/components/app/service_groups/service_group_save/group_details.tsx Outdated Show resolved Hide resolved
...ility_solution/apm/public/components/app/service_groups/service_group_save/group_details.tsx Show resolved Hide resolved
...ity_solution/apm/public/components/app/service_groups/service_group_save/select_services.tsx Show resolved Hide resolved
cauemarcondes
cauemarcondes approved these changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

kibana-ci commented Oct 7, 2024 •
edited
Loading

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 13b8b63
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-194696-13b8b635b417

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 3.4MB 3.4MB +251.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alexwizp alexwizp merged commit d576365 into elastic:main Oct 7, 2024
31 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11215089937

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 7, 2024
@alexwizp
fix: [Obs Services > Create Service Group modal][KEYBOARD]: Focus sho…
7728bf2 
…uld not be auto-set on first input when modal appears (elastic#194696)

Closes: elastic#194965
Closes: elastic#194966

# Description

- [x] elastic#194965 <br /> Focus is
currently being set on the first input in the "Create group" modal.
Screen reader users will hear the input name, but not get the title of
the modal read aloud this way, and it could be confusing. We should be
letting the EuiModal set focus naturally on the modal or close button so
screen reader users hear the title as expected.

- [x] elastic#194966 <br /> Focus must
be returned properly when I cancel the "Create group" workflow in
Services > Create service group modal.

# Changes Made

1. Removed:

```diff
- inputRef.current?.focus(); // autofocus on initial render
```

2. Added `aria-labelledby={modalTitleId}` for `EuiModal`. See
https://eui.elastic.co/#/layout/modal.
3. Slightly updated `Name` and `Color` validation.

# Screen

https://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7
(cherry picked from commit d576365)
@kibanamachine kibanamachine mentioned this pull request Oct 7, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine mentioned this pull request Oct 7, 2024
Merged
kibanamachine added a commit that referenced this pull request Oct 7, 2024
@kibanamachine @alexwizp
[8.x] fix: [Obs Services &gt; Create Service Group modal][KEYBOARD]: …
5ea090b 
…Focus should not be auto-set on first input when modal appears (#194696) (#195235)

# Backport

This will backport the following commits from `main` to `8.x`:
- [fix: [Obs Services &gt; Create Service Group modal][KEYBOARD]: Focus
should not be auto-set on first input when modal appears
(#194696)](#194696)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alexey
Antonov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-07T12:04:39Z","message":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal appears (#194696)\n\nCloses:
#194965 \r\nCloses:
https://github.com/elastic/kibana/issues/194966\r\n\r\n# Description
\r\n\r\n- [x] #194965 <br />
Focus is\r\ncurrently being set on the first input in the \"Create
group\" modal.\r\nScreen reader users will hear the input name, but not
get the title of\r\nthe modal read aloud this way, and it could be
confusing. We should be\r\nletting the EuiModal set focus naturally on
the modal or close button so\r\nscreen reader users hear the title as
expected.\r\n\r\n\r\n- [x]
#194966 <br /> Focus must\r\nbe
returned properly when I cancel the \"Create group\" workflow
in\r\nServices > Create service group modal.\r\n\r\n# Changes
Made\r\n\r\n1. Removed:\r\n\r\n```diff \r\n- inputRef.current?.focus();
// autofocus on initial render\r\n```\r\n\r\n2. Added
`aria-labelledby={modalTitleId}` for `EuiModal`.
See\r\nhttps://eui.elastic.co/#/layout/modal.\r\n3. Slightly updated
`Name` and `Color` validation.\r\n\r\n\r\n#
Screen\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7","sha":"d5763658c39856aefb5e15fa9e3e771f8bb0d613","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Project:Accessibility","release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services","apm:review"],"title":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal
appears","number":194696,"url":"https://github.com/elastic/kibana/pull/194696","mergeCommit":{"message":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal appears (#194696)\n\nCloses:
#194965 \r\nCloses:
https://github.com/elastic/kibana/issues/194966\r\n\r\n# Description
\r\n\r\n- [x] #194965 <br />
Focus is\r\ncurrently being set on the first input in the \"Create
group\" modal.\r\nScreen reader users will hear the input name, but not
get the title of\r\nthe modal read aloud this way, and it could be
confusing. We should be\r\nletting the EuiModal set focus naturally on
the modal or close button so\r\nscreen reader users hear the title as
expected.\r\n\r\n\r\n- [x]
#194966 <br /> Focus must\r\nbe
returned properly when I cancel the \"Create group\" workflow
in\r\nServices > Create service group modal.\r\n\r\n# Changes
Made\r\n\r\n1. Removed:\r\n\r\n```diff \r\n- inputRef.current?.focus();
// autofocus on initial render\r\n```\r\n\r\n2. Added
`aria-labelledby={modalTitleId}` for `EuiModal`.
See\r\nhttps://eui.elastic.co/#/layout/modal.\r\n3. Slightly updated
`Name` and `Color` validation.\r\n\r\n\r\n#
Screen\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7","sha":"d5763658c39856aefb5e15fa9e3e771f8bb0d613"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194696","number":194696,"mergeCommit":{"message":"fix:
[Obs Services > Create Service Group modal][KEYBOARD]: Focus should not
be auto-set on first input when modal appears (#194696)\n\nCloses:
#194965 \r\nCloses:
https://github.com/elastic/kibana/issues/194966\r\n\r\n# Description
\r\n\r\n- [x] #194965 <br />
Focus is\r\ncurrently being set on the first input in the \"Create
group\" modal.\r\nScreen reader users will hear the input name, but not
get the title of\r\nthe modal read aloud this way, and it could be
confusing. We should be\r\nletting the EuiModal set focus naturally on
the modal or close button so\r\nscreen reader users hear the title as
expected.\r\n\r\n\r\n- [x]
#194966 <br /> Focus must\r\nbe
returned properly when I cancel the \"Create group\" workflow
in\r\nServices > Create service group modal.\r\n\r\n# Changes
Made\r\n\r\n1. Removed:\r\n\r\n```diff \r\n- inputRef.current?.focus();
// autofocus on initial render\r\n```\r\n\r\n2. Added
`aria-labelledby={modalTitleId}` for `EuiModal`.
See\r\nhttps://eui.elastic.co/#/layout/modal.\r\n3. Slightly updated
`Name` and `Color` validation.\r\n\r\n\r\n#
Screen\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6636f2dc-b9b7-4d4d-8144-90249f8327e7","sha":"d5763658c39856aefb5e15fa9e3e771f8bb0d613"}}]}]
BACKPORT-->

Co-authored-by: Alexey Antonov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees
No one assigned
Labels
apm:review backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
6 participants
@alexwizp @elasticmachine @kibana-ci @kibanamachine @cauemarcondes @smith