-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
(Accessibility) Implement client routing solution for all products #135760
Comments
Pinging @elastic/kibana-accessibility (Project:Accessibility) |
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
Pinging @elastic/eui-design (EUI) |
Pinging @elastic/kibana-core (Team:Core) |
From the slack discussion opened by @1Copenut
Mostly FMI here, but IIRC from when I was doing a11y, the aria-live and/or role=alert elements don't have to be focusable, and therefor don't have to be at the top of the document? Or do they? Not asking about the skip link here, but about the EuiScreenReaderLive and what it should output.
Should not be an issue, we should be able to find a place in the chrome (the static part of the UI with the header and left nav) for these element to be first on focus. The problem doing so is that they will be out of scope/range from individual application, meaning that is any context required for these components depends on the currently mounted app, Core will have to expose APIs for application to consume to populate this context.
This one could be tricky, depending on how you see it implemented. See, technically Kibana is not a regular React App. The root dom level, managed by Core (that we call Do know know exactly what the live element should output, and how you see the component hooked to history changes?
Where should the skip link direct to? If it's to an element that doesn't depend on the currently displayed Kibana application (e.g the element where Core does mount the current app), here kibana/packages/core/rendering/core-rendering-browser-internal/src/rendering_service.tsx Lines 63 to 69 in f7ab9b7
This shouldn't be an issue. If the link's target should be dynamic depending on the currently mounted App, this could be more bothersome, given the currently mounted application would need to propagate the info about where the link should point to to Core for us to update the link depending on it. |
@pgayvallet Thank you for the clear, thorough responses. I'll restate questions and answer using the same format. Adding @jennypavlova to this thread. She's started work on this item for the Infrastructure team, and brought the questions about source order placement. Her work is the first implementation of this new pattern. My hope is to not cause her too much rework, and to have a solution that works for all teams. Question 1
I tried a few different solutions with and without setting focus on the Question 2
So far I have been advising to use the Question 3
The live element will output HTML like the following snippet. The rendered output can also be observed by inspecting source in the public EUI docs pages. <!-- https://eui.elastic.co/#/utilities/accessibility#screen-reader-live-region -->
<div class="css-hus3oj-euiScreenReaderOnly" tabindex="-1">
<div
aria-atomic="true"
aria-live="off"
role="status"
>
Accessibility — Elastic UI Framework
</div>
<div
aria-atomic="true"
aria-hidden="true"
aria-live="off"
role="status"
>
</div> Question 4
If the |
Question 1
Thanks, make sense. I do remember that SR/browser behavior consistency was a nightmare at the time. Question 2 + Question 3
The title of the page can be set by applications using the Note that if the SR-only div is supposed to just output the title of the page, then it can be implemented directly in our Question 4
Yea, this div will always be present, and will always be the parent of any application currently displayed. If that works with you, then that means that this part too can be internally managed by Core without the need for other teams to do anything. |
@1Copenut Thank you for adding me to the discussion. Which is the correct page title? Should it be for example:
Which separator should we use Regarding the way the title is set:
@pgayvallet You mentioned that the title should be set with |
Sorry to jump in here, I just have an opinion about:
My 2c: just because this div is wrapped around the parent app doesn't mean it should necessarily be used as the
Hope that helps! |
Great points @constancecchen. I see this as an opportunity to capture your guidance in the EUI Getting Started > Accessibility section. I'll take that as an action item. |
Sorry, I meant
It's against our guidelines and shouldn't be done, specifically for scenarios like this where we need to have an unified way to track changes to the title. Changes performed by setting
Sigh... what can I say, using |
Yea, that's what I thought too, which is why I asked the question. Would have been too easy otherwise 😅
It implies more structural Core changes. Let's say that as long as this ID is static per full application and therefore can be specified during the Al alternative to that would be to specify an arbitrary
I may be wrong, but IIRC, an a11y skiplink must be a valid |
Excellent discussion, and I feel we're getting closer to a workable spec with each comment.
In the ideal scenario there would be one ID value that all teams point their skip link With that said, I think we'll be further ahead if we can use your first option, to have an ID that's static per full application, thus can be specified during the
You're right, it should be a valid link with a valid Looking through WCAG SC 2.4.1 - Bypass Blocks again, I don't see anything our component is doing that would run counter to the guidance. SPAs definitely created some new use cases. |
Yea this would be fine. I was mostly asking because of the 3 step workflow described by @constancecchen in #135760 (comment) doesn't always target the element by an ID. |
There is not anything inherently more accessible about a skip link with a But don't just take my word for it, here's the official W3 spec that has no mention/requirement of
And WebAIM has even more interesting context on the flexibility of the spec:
EDIT: After speaking to @1Copenut more about this on Slack, I was incorrect in my assumption about the accessibility distinctions between |
Okay, thanks for the clarifications @constancecchen. This is good news, as it will allow us to be more flexible and allow potential 'fallback' on the target of the skipLink. |
The update to skip link behavior was merged into EUI 68.0.0. We should have the pieces we need in place after this version of EUI is merged into Kibana. |
Broke down this work into the following tickets, which should close this issue once all done: |
Thank you @vadimkibana for putting these together. I'll add a few comments and a link to an app I built as a best case example. |
I've added #149847 for documentation on how to test with screen readers and assigned myself. |
This should be closed now by #150461 |
Description
The EUI team has been working on a long-term solution for a universal client-side routing announcement for screen readers. We have adopted this solution into the EUI Docs and are now wanting to bring it to Kibana products.
This meta issue will be a way to track implementation work for products within Kibana.
Tracking issue: #42379
Acceptance criteria
EuiScreenReaderLive
is ideally the first thing in the React DOM source order, soEuiSkipLink
can be the second element in the React DOM source orderWe're trying to smooth out potential bumps in the road by settling on a pattern that adds a new component as the very first component in the
div#root
container (or whatever your team is declaring as the React root container). This means the skip link can be added or moved to be the second component, and the rest of your application structure shouldn't need to be adjusted.Observability
Relevant WCAG Criteria:
The text was updated successfully, but these errors were encountered: