[Discover] Added focus to h1 element when client side routing is executed #133846

davismcphee · 2022-06-08T02:05:27Z

Summary

This PR adds support for focusing on the h1 element when navigating to Discover.

Resolves #133277.

Checklist

~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
(Tested screen readers in Chrome and Safari) This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

davismcphee · 2022-06-08T02:09:50Z

src/plugins/discover/public/application/main/components/layout/discover_layout.test.tsx

+function mountComponent(
+  indexPattern: DataView,
+  prevSidebarClosed?: boolean,
+  mountOptions: { attachTo?: HTMLElement } = {}


toHaveFocus doesn't work without the component being mounted to an element in the body, so I added mountOptions to pass in a mount point.

element.focus() not setting document.activeElement enzymejs/enzyme#2337

document.activeElement is always null after upgrading to jsdom 15/16 (jest 25) jsdom/jsdom#2924

davismcphee · 2022-06-08T17:23:28Z

@1Copenut This PR adds support for focusing on the h1 when navigating to Discover. Based on this comment on the issue, can you confirm whether or not this work is still necessary?

If it is, another question: After focusing on the h1 and pressing tab, the field search in the sidebar gains focus. Is this acceptable or should another element gain focus?

Thanks!

1Copenut · 2022-06-08T20:18:55Z

@davismcphee Yes, please proceed forward with this improvement. The EUI team is exploring a live region solution but it may not work at Kibana scale and we do not want to block your team from implementing an a11y solution.

If users press TAB after the H1 receives focus, we want to let it move to the next natively focusable element. If that's the field search in the sidebar, that input will announce itself to the screen reader.

I took a look at a cloud 7.16.3 instance and mapped the H1 and where focus would go on next TAB keypress. I'll verify this with the local build of latest main branch, and update my image if something has changed.

kertal · 2022-06-09T04:49:18Z

src/plugins/discover/public/application/main/components/layout/discover_layout.tsx

@@ -236,7 +241,13 @@ export function DiscoverLayout({
          spaces={spaces}
          history={history}
        />
-        <h1 id="savedSearchTitle" className="euiScreenReaderOnly">
+        <h1


we could think of moving the <h1> to L223, so it's the first after the TopNavMemoized. Because when using tab currently the side bar field search gets focus. I think starting with the data view picker would be a better fit.

@kertal Great call. If the H1 can be moved to this new position, it would be a good reference point for future applications. We could use this as guidance to say "Pages and applications should have an H1 just after the memoized top navigation" as common guidance.

I'm attaching a screenshot I took from my local machine this morning with annotations to make sure I'm understanding the new location as you've described it.

yes, that's what I meant and also it works when doing it that way (short local test with Davis PR)

davismcphee · 2022-06-09T17:11:33Z

@1Copenut @kertal Thank you both for the confirmation and suggestions. I agree the suggested h1 placement make sense. I'll update the PR to include the new h1 position and mark as ready for review.

…al tests for Discover accessibility

davismcphee · 2022-06-14T00:30:20Z

src/plugins/discover/public/application/main/components/layout/discover_layout.test.tsx

@@ -174,6 +179,17 @@ describe('Discover component', () => {
    expect(component.find('[data-test-subj="discoverChartOptionsToggle"]').exists()).toBeTruthy();
  });

+  test('the saved search title h1 gains focus on navigate', () => {


@kertal I added some functional tests for accessibility which make this test sort of redundant. Do you think I should rollback changes to this test file or keep it?

I think it's fine to keep it as a unit test too

davismcphee · 2022-06-14T00:35:12Z

test/functional/apps/discover/_discover.ts

+    describe('discover functionality tests', () => {
+      before(async function () {
+        await setUpDiscoverPage();
+        await PageObjects.timePicker.setDefaultAbsoluteRange();


This call before each test caused my accessibility tests to fail since it changes the initial focus in the browser. To fix this I broke the tests up into 'accessibility tests' and 'functionality tests' so I could skip running this function in the before hook. I'm open to any suggestions about a better way to handle this though.

We could also move "discover accessibility tests" out to a new separate file and don't make this file even more complex.

Good idea, I'll pull these into a separate _discover_accessibility.ts file and update the PR.

elasticmachine · 2022-06-14T01:44:03Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta

Works as expected in Chrome and Firefox but for some reason in Safari it selects the search input after TAB.

jughosta · 2022-06-14T11:24:41Z

src/plugins/discover/public/application/main/components/layout/discover_layout.test.tsx

@@ -174,6 +179,17 @@ describe('Discover component', () => {
    expect(component.find('[data-test-subj="discoverChartOptionsToggle"]').exists()).toBeTruthy();
  });

+  test('the saved search title h1 gains focus on navigate', () => {


I think it's fine to keep it as a unit test too

jughosta · 2022-06-14T11:27:42Z

test/functional/apps/discover/_discover.ts

+      after(tearDownDiscoverPage);
+
+      it('should give focus to the saved search title h1 on navigate', async () => {
+        const skipButton = await testSubjects.find('discoverSavedSearchTitle');


Maybe titleElement instead of skipButton?

I agree, I borrowed this from another test and forgot to update the variable name. Will do that as well.

jughosta · 2022-06-14T11:31:26Z

test/functional/apps/discover/_discover.ts

+    describe('discover functionality tests', () => {
+      before(async function () {
+        await setUpDiscoverPage();
+        await PageObjects.timePicker.setDefaultAbsoluteRange();


We could also move "discover accessibility tests" out to a new separate file and don't make this file even more complex.

davismcphee · 2022-06-14T14:23:42Z

Works as expected in Chrome and Firefox but for some reason in Safari it selects the search input after TAB.

@jughosta It looks like by default in Safari you have to use ⌥ + Tab to tab through all focusable elements in the page. Otherwise you can change the default setting with Safari > Preferences > Advanced > Press Tab to highlight each item on a webpage. These solutions fixed the issue for me. Can you confirm one of them fixes it for you as well?

jughosta · 2022-06-14T15:03:52Z

These solutions fixed the issue for me. Can you confirm one of them fixes it for you as well?

Yes, I can confirm that it makes the focus behave as in other browsers. TIL, thanks!

… file

davismcphee · 2022-06-14T21:07:51Z

Updated with suggestions

kibana-ci · 2022-06-14T22:07:50Z

💚 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
`discover`	485.6KB	485.8KB	+172.0B

History

💚 Build #50777 succeeded a3b7080
💛 Build #50654 was flaky cadf625
💚 Build #49941 succeeded e1a1f8a
💚 Build #49744 succeeded 3154844

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

cc @davismcphee

jughosta

LGTM, thanks 👍

kertal · 2022-06-15T09:26:35Z

These solutions fixed the issue for me. Can you confirm one of them fixes it for you as well?

Yes, I can confirm that it makes the focus behave as in other browsers. TIL, thanks!

+1 for TIL! thx!

kertal · 2022-06-15T09:32:38Z

We should apply the same pattern when using

Surrounding documents
Single document
(in follow up PRs)

[Discover] Added focus to h1 element on navigate

3154844

davismcphee commented Jun 8, 2022

View reviewed changes

davismcphee self-assigned this Jun 8, 2022

Merge branch 'main' into feature-discover-focus-h1

e1a1f8a

kertal reviewed Jun 9, 2022

View reviewed changes

davismcphee added 2 commits June 13, 2022 21:25

[Discover] Moved Discover h1 to above the nav bar, and added function…

4ce0331

…al tests for Discover accessibility

Merge branch 'main' into feature-discover-focus-h1

cadf625

davismcphee commented Jun 14, 2022

View reviewed changes

davismcphee marked this pull request as ready for review June 14, 2022 01:44

davismcphee requested a review from a team as a code owner June 14, 2022 01:44

jughosta reviewed Jun 14, 2022

View reviewed changes

Merge branch 'main' into feature-discover-focus-h1

a3b7080

[Discover] Moved Discover accessibility functional tests to their own…

f4d2911

… file

jughosta approved these changes Jun 15, 2022

View reviewed changes

davismcphee merged commit adb61f7 into elastic:main Jun 15, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jun 15, 2022

davismcphee deleted the feature-discover-focus-h1 branch June 15, 2022 12:59

davismcphee mentioned this pull request Jun 15, 2022

(Accessibility) [Discover] Add focus to h1 when navigating to 'Surrounding documents' and 'Single document' pages #134521

Closed

1Copenut mentioned this pull request Feb 24, 2023

(Accessibility) Discover - remove custom H1 focus on render now that global solution is in place #152131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Added focus to h1 element when client side routing is executed #133846

[Discover] Added focus to h1 element when client side routing is executed #133846

davismcphee commented Jun 8, 2022

davismcphee Jun 8, 2022

davismcphee commented Jun 8, 2022

1Copenut commented Jun 8, 2022 •

edited

Loading

kertal Jun 9, 2022

1Copenut Jun 9, 2022

kertal Jun 9, 2022 •

edited

Loading

davismcphee commented Jun 9, 2022

davismcphee Jun 14, 2022

jughosta Jun 14, 2022

davismcphee Jun 14, 2022

jughosta Jun 14, 2022

davismcphee Jun 14, 2022

elasticmachine commented Jun 14, 2022

jughosta left a comment

jughosta Jun 14, 2022

jughosta Jun 14, 2022

davismcphee Jun 14, 2022

jughosta Jun 14, 2022

davismcphee commented Jun 14, 2022

jughosta commented Jun 14, 2022

davismcphee commented Jun 14, 2022

kibana-ci commented Jun 14, 2022

jughosta left a comment

kertal commented Jun 15, 2022

kertal commented Jun 15, 2022

[Discover] Added focus to h1 element when client side routing is executed #133846

[Discover] Added focus to h1 element when client side routing is executed #133846

Conversation

davismcphee commented Jun 8, 2022

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

davismcphee commented Jun 8, 2022

1Copenut commented Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

davismcphee commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 14, 2022

jughosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davismcphee commented Jun 14, 2022

jughosta commented Jun 14, 2022

davismcphee commented Jun 14, 2022

kibana-ci commented Jun 14, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

jughosta left a comment

Choose a reason for hiding this comment

kertal commented Jun 15, 2022

kertal commented Jun 15, 2022

1Copenut commented Jun 8, 2022 •

edited

Loading

kertal Jun 9, 2022 •

edited

Loading