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

fix: [Obs Applications > Services | Traces | Dependencies][SCREEN READER]: H1 tag should not include secondary information #193880

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Sep 24, 2024

Closes: #194988
Closes: #194987
Closes: #194986

Description

Observability has a few pages that wrap related information like alert counts in the H1 tag. This presents a challenge to screen readers because all of that information now becomes the heading level one. It clogs up the Headings menu and makes it harder to reason about the page and what's primary information vs. secondary.

What was changed?:

  1. extra content has been removed from pageTitle and moved to rightSideItems.

Screen:

image

Note

On smaller screens (at certain resolutions) sometimes we have an issue described in elastic/eui#8039 . But It's not a blocker for that PR and will be fixed on EUI side

@alexwizp
Copy link
Contributor Author

/ci

@alexwizp alexwizp marked this pull request as ready for review September 25, 2024 07:34
@alexwizp alexwizp requested a review from a team as a code owner September 25, 2024 07:34
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 25, 2024
@elasticmachine
Copy link
Contributor

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

@alexwizp alexwizp added Project:Accessibility and removed ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 25, 2024
@elasticmachine
Copy link
Contributor

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

@alexwizp alexwizp added v9.0.0 release_note:skip Skip the PR/issue when compiling release notes backport:version Backport to applied version labels v8.16.0 labels Sep 25, 2024
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 25, 2024
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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 -305.0B

History

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

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.

Please, could you fix the env field location?


const rightSideItems = [
...(showServiceGroupSaveButton ? [<ServiceGroupSaveButton />] : []),
...(environmentFilter ? [<ApmEnvironmentFilter />] : []),
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at where the env filter is located now:
Screenshot 2024-09-25 at 14 26 08

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cauemarcondes ok, let's wait till EUI will fix elastic/eui#8039.

# Conflicts:
#	x-pack/plugins/observability_solution/apm/public/components/routing/templates/apm_main_template/index.tsx
@alexwizp
Copy link
Contributor Author

alexwizp commented Oct 8, 2024

@cauemarcondes can I ask you to have a look again? EUI with elastic/eui#8039 fix already in Kibana

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 8, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 835473e
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193880-835473e3708c

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 -305.0B

History

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, thanks it worked as expected now.
Uploading Screenshot 2024-10-08 at 15.53.26.png…

@alexwizp alexwizp merged commit a78a31d into elastic:main Oct 8, 2024
24 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 8, 2024
…DER]: H1 tag should not include secondary information (elastic#193880)

Closes: elastic#194988
Closes: elastic#194987
Closes: elastic#194986

## Description
Observability has a few pages that wrap related information like alert
counts in the H1 tag. This presents a challenge to screen readers
because all of that information now becomes the heading level one. It
clogs up the Headings menu and makes it harder to reason about the page
and what's primary information vs. secondary.

## What was changed?:

1. extra content has been removed from `pageTitle` and moved to
`rightSideItems`.

## Screen:

<img width="1226" alt="image"
src="https://github.com/user-attachments/assets/221a1d80-7686-47e3-b0d1-b8c8eada9374">

> [!NOTE]
> On smaller screens (at certain resolutions) sometimes we have an issue
described in elastic/eui#8039 . But It's not a
blocker for that PR and will be fixed on EUI side

(cherry picked from commit a78a31d)
@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 added a commit that referenced this pull request Oct 8, 2024
…CREEN READER]: H1 tag should not include secondary information (#193880) (#195478)

# Backport

This will backport the following commits from `main` to `8.x`:
- [fix: [Obs Applications &gt; Services | Traces | Dependencies][SCREEN
READER]: H1 tag should not include secondary information
(#193880)](#193880)

<!--- 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-08T17:04:40Z","message":"fix:
[Obs Applications > Services | Traces | Dependencies][SCREEN READER]: H1
tag should not include secondary information (#193880)\n\nCloses:
https://github.com/elastic/kibana/issues/194988\r\nCloses:
https://github.com/elastic/kibana/issues/194987\r\nCloses:
https://github.com/elastic/kibana/issues/194986\r\n\r\n##
Description\r\nObservability has a few pages that wrap related
information like alert\r\ncounts in the H1 tag. This presents a
challenge to screen readers\r\nbecause all of that information now
becomes the heading level one. It\r\nclogs up the Headings menu and
makes it harder to reason about the page\r\nand what's primary
information vs. secondary.\r\n\r\n## What was changed?:\r\n \r\n1. extra
content has been removed from `pageTitle` and moved
to\r\n`rightSideItems`.\r\n\r\n\r\n## Screen: \r\n\r\n<img
width=\"1226\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/221a1d80-7686-47e3-b0d1-b8c8eada9374\">\r\n\r\n\r\n\r\n>
[!NOTE] \r\n> On smaller screens (at certain resolutions) sometimes we
have an issue\r\ndescribed in elastic/eui#8039
. But It's not a\r\nblocker for that PR and will be fixed on EUI
side","sha":"a78a31d1873a7dca3d175870aee05801b056f5a4","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","ci:project-deploy-observability","Team:obs-ux-infra_services","apm:review","v8.16.0","backport:version"],"title":"fix:
[Obs Applications > Services | Traces | Dependencies][SCREEN READER]: H1
tag should not include secondary
information","number":193880,"url":"https://github.com/elastic/kibana/pull/193880","mergeCommit":{"message":"fix:
[Obs Applications > Services | Traces | Dependencies][SCREEN READER]: H1
tag should not include secondary information (#193880)\n\nCloses:
https://github.com/elastic/kibana/issues/194988\r\nCloses:
https://github.com/elastic/kibana/issues/194987\r\nCloses:
https://github.com/elastic/kibana/issues/194986\r\n\r\n##
Description\r\nObservability has a few pages that wrap related
information like alert\r\ncounts in the H1 tag. This presents a
challenge to screen readers\r\nbecause all of that information now
becomes the heading level one. It\r\nclogs up the Headings menu and
makes it harder to reason about the page\r\nand what's primary
information vs. secondary.\r\n\r\n## What was changed?:\r\n \r\n1. extra
content has been removed from `pageTitle` and moved
to\r\n`rightSideItems`.\r\n\r\n\r\n## Screen: \r\n\r\n<img
width=\"1226\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/221a1d80-7686-47e3-b0d1-b8c8eada9374\">\r\n\r\n\r\n\r\n>
[!NOTE] \r\n> On smaller screens (at certain resolutions) sometimes we
have an issue\r\ndescribed in elastic/eui#8039
. But It's not a\r\nblocker for that PR and will be fixed on EUI
side","sha":"a78a31d1873a7dca3d175870aee05801b056f5a4"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193880","number":193880,"mergeCommit":{"message":"fix:
[Obs Applications > Services | Traces | Dependencies][SCREEN READER]: H1
tag should not include secondary information (#193880)\n\nCloses:
https://github.com/elastic/kibana/issues/194988\r\nCloses:
https://github.com/elastic/kibana/issues/194987\r\nCloses:
https://github.com/elastic/kibana/issues/194986\r\n\r\n##
Description\r\nObservability has a few pages that wrap related
information like alert\r\ncounts in the H1 tag. This presents a
challenge to screen readers\r\nbecause all of that information now
becomes the heading level one. It\r\nclogs up the Headings menu and
makes it harder to reason about the page\r\nand what's primary
information vs. secondary.\r\n\r\n## What was changed?:\r\n \r\n1. extra
content has been removed from `pageTitle` and moved
to\r\n`rightSideItems`.\r\n\r\n\r\n## Screen: \r\n\r\n<img
width=\"1226\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/221a1d80-7686-47e3-b0d1-b8c8eada9374\">\r\n\r\n\r\n\r\n>
[!NOTE] \r\n> On smaller screens (at certain resolutions) sometimes we
have an issue\r\ndescribed in elastic/eui#8039
. But It's not a\r\nblocker for that PR and will be fixed on EUI
side","sha":"a78a31d1873a7dca3d175870aee05801b056f5a4"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
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
Labels
apm:review backport:version Backport to applied version labels 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
6 participants