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

Route change announcements for Infrastructure inventory and host views #135874

Closed
Tracked by #135760
smith opened this issue Jul 7, 2022 · 10 comments
Closed
Tracked by #135760

Route change announcements for Infrastructure inventory and host views #135874

smith opened this issue Jul 7, 2022 · 10 comments
Labels
blocked Feature:Metrics UI Metrics UI feature Feature:ObsHosts Hosts feature within Observability Feature:ObsInventory Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services

Comments

@smith
Copy link
Contributor

smith commented Jul 7, 2022

See parent issue #135760.

Implement recommended solution for announcing the new location for client-side router route changes.

Do this on the infrastructure inventory and individual host pages.

✔️ Acceptance criteria

New page location is announced to screen readers when viewing these pages.

@smith smith added Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Jul 7, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@jennypavlova
Copy link
Member

jennypavlova commented Sep 5, 2022

[WIP] Questions/Notes:

  • Should we announce only the page title after redirection from the main left navigation or also the titles of the pages inside the app (if the user clicks a button and he lands on a page with a different title)
  • Is there an example of EuiScreenReaderLive in an actual project or it will be used for the first time in this task? (I found only the example inside the docs)
    • Which content should we wrap inside the component (only the title or the page content? In case there is no title should it be the navigation link name and previous page or it should be the first text that appears on the page)
    • Should we use the component once on the router level or the component should be used on every page separately?
  • How to test - I tried the voice-over apple tool and navigating using the keyboard (Also tried a FF plugin ). What's the best practice?
  • I saw that after navigating there are different focus elements on different pages (for example on the Inventory page we focus on the search field) Should we do the same for the Host page for example? And should the reader first "read" the page title and then focus on input/different content?

@miltonhultgren
Copy link
Contributor

@elastic/kibana-accessibility might be best able to advice here.

@1Copenut
Copy link
Contributor

1Copenut commented Sep 6, 2022

Great questions @jennypavlova! I'll answer each bullet point as a point of its own.

  • We should announce each page title after redirection from the main left nav and titles of pages within the apps. So if I clicked "Integrations" from the left menu, I should hear it announced. If I clicked "pfSense" from the list of integrations, I should hear that announced too. My general rule of thumb is if the route / URL is different, we should be announcing it using this pattern.

  • The EUI team road-tested this pattern in the docs site https://eui.elastic.co/#/ . For EUI docs, we wrapped the page title with information about the site. So if I hit the accessibility route, screen readers would announce

    Accessibility — Elastic UI Framework
    

    Using the Integrations example from my previous point, the page <title> strings are perfect:

    - Browse integrations - Integrations - Elastic
    - pfSense - Integrations - Elastic
    
    • If there is no page <title> tag, we should announce a Left nav name - Elastic or H1 text - Elastic depending on which feels more descriptive of the page's intent.
    • If there's a breadcrumb like "Setup guide" we should announce Setup guide - Left nav name - Elastic
    • If you find pages without a <title> tag, that should also be logged as an accessibility defect. You can point teams to WCAG SC 2.4.2 - Page Titled (Level A) for the WCAG violation.
  • The best way to confirm these are working as expected is to turn on a screen reader like VoiceOver, and click a left navigation link. The page should load, and the EuiScreenReaderOnly text should announce immediately. Clicking a different page should yield the same behavior. We used a technique from GOV.UK to add two live regions inside our component to ensure screen readers do not silently absorb the changed text and not announce.

  • The new component handles focus automatically and will be our global focus management pattern going forward. The component has a tabindex="-1" and takes focus on first render. It should be the first item in the React app source order. The skip to content link should be the next item, so users can load a page, press TAB once, and surface the skip link.

    The EUI team worked this behavior into the EUI docs as well, to illustrate on every major route change, we will handle focus consistently and provide a consistent skip navigation placement and behavior.

@1Copenut
Copy link
Contributor

1Copenut commented Sep 6, 2022

The EUI team also put together a screen reader testing matrix that outlines what screen readers we support, and testing tiers: https://github.com/elastic/eui/blob/main/wiki/screen-reader-testing-matrix.md

@jennypavlova
Copy link
Member

jennypavlova commented Sep 8, 2022

Notes (for myself this time 😄):

  • EuiScreenReaderOnly route level use document title (double check if it can be accessed there?)
    • set focusRegionOnTextChange to true ( Make sure that there is no autofocus on other elements - like the search in inventory)
  • Implement skipLink
  • Make sure the sub-pages routes are announced as well
  • Unit testing - check focus

@jennypavlova jennypavlova changed the title Route change announcements for Infrastructure inventory and host views [On Hold] Route change announcements for Infrastructure inventory and host views Sep 12, 2022
@smith
Copy link
Contributor Author

smith commented Sep 21, 2022

Ongoing discussion on #135760 indicates that they might be able to solve this at the platform level. Putting this back into the backlog until we have finalized guidance on how to finish this.

@smith smith added the blocked label Sep 21, 2022
@smith smith changed the title [On Hold] Route change announcements for Infrastructure inventory and host views Route change announcements for Infrastructure inventory and host views Sep 21, 2022
jennypavlova added a commit that referenced this issue Sep 28, 2022
…crumbs #135874 (#140824)

* Use breadcrumbs to set page title intead of DocumentTitle

* Remove document title component and use breadcrumbs - Logs Page

* Remove no longer needed document title translations

* Use docTitle.change in error page and remove document title component

* Add document title check to the functional tests

* Move test to check title after load

* test: Title change

* Remove unnecessary docTitle type

* Add error page tests

* Update snapshot

* Functional tests: Wait for loading the header before checking the title

* Change test to use react testing library

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@roshan-elastic
Copy link

Hey @smith is this one still needed? Moving to bottom of backlog based on last comment

@roshan-elastic roshan-elastic added Feature:ObsHosts Hosts feature within Observability Feature:ObsInventory Feature:Metrics UI Metrics UI feature and removed Feature:Metrics UI Metrics UI feature Infra (UI) labels Feb 23, 2023
@smith
Copy link
Contributor Author

smith commented Feb 23, 2023

@roshan-elastic based on the discussion on the meta-issue this was created from it looks like we may not have to do anything here. I'm going to close this and we can reopen if needed.

@smith smith closed this as not planned Won't fix, can't repro, duplicate, stale Feb 23, 2023
@roshan-elastic
Copy link

Great - thanks @smith

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Feature:Metrics UI Metrics UI feature Feature:ObsHosts Hosts feature within Observability Feature:ObsInventory Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

No branches or pull requests

6 participants