[Discover] Remove custom h1 focus logic. Unskip tests. (#155613)

Closes #152131 Closes #152938 https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2163
elastic · May 22, 2023 · 633444e · 633444e
1 parent 97e6c84
commit 633444e
Show file tree

Hide file tree

Showing 9 changed files with 15 additions and 82 deletions.
diff --git a/packages/core/chrome/core-chrome-browser-internal/src/ui/header/screen_reader_a11y.test.tsx b/packages/core/chrome/core-chrome-browser-internal/src/ui/header/screen_reader_a11y.test.tsx
@@ -24,33 +24,20 @@ describe('ScreenReaderRouteAnnouncements', () => {
     expect(component).toMatchSnapshot();
   });
 
-  it('does not set the focusOnRegionOnTextChange for canvas or discover', () => {
+  it('does not set the focusOnRegionOnTextChange for canvas', () => {
     const noFocusComponentCanvas = mount(
       <ScreenReaderRouteAnnouncements
         appId$={new BehaviorSubject('canvas')}
         customBranding$={new BehaviorSubject({})}
         breadcrumbs$={new BehaviorSubject([])}
       />
     );
-    const noFocusComponentDiscover = mount(
-      <ScreenReaderRouteAnnouncements
-        appId$={new BehaviorSubject('discover')}
-        customBranding$={new BehaviorSubject({})}
-        breadcrumbs$={new BehaviorSubject([])}
-      />
-    );
 
     expect(
       noFocusComponentCanvas
         .debug()
         .includes('<EuiScreenReaderLive focusRegionOnTextChange={false}>')
     ).toBeTruthy();
-
-    expect(
-      noFocusComponentDiscover
-        .debug()
-        .includes('<EuiScreenReaderLive focusRegionOnTextChange={false}>')
-    ).toBeTruthy();
   });
 
   it('sets the focusOnRegionOnTextChange to true for other apps', () => {

diff --git a/packages/core/chrome/core-chrome-browser-internal/src/ui/header/screen_reader_a11y.tsx b/packages/core/chrome/core-chrome-browser-internal/src/ui/header/screen_reader_a11y.tsx
@@ -45,10 +45,8 @@ export const ScreenReaderRouteAnnouncements: FC<{
 
   // 1. Canvas dynamically updates breadcrumbs *and* page title/history on every name onChange,
   // which leads to focus fighting if this is enabled
-  // 2. Discover has custom h1 focus behavior on route change, which should probably
-  // be removed in favor of this for a more consistent SR experience
   const appId = useObservable(appId$);
-  const disableFocusForApps = ['canvas', 'discover'];
+  const disableFocusForApps = ['canvas'];
   const focusRegionOnTextChange = !disableFocusForApps.includes(appId || '');
 
   return (

diff --git a/src/plugins/discover/public/application/context/context_app.tsx b/src/plugins/discover/public/application/context/context_app.tsx
@@ -169,11 +169,6 @@ export const ContextApp = ({ dataView, anchorId, referrer }: ContextAppProps) =>
     };
   };
 
-  const contextAppTitle = useRef<HTMLHeadingElement>(null);
-  useEffect(() => {
-    contextAppTitle.current?.focus();
-  }, []);
-
   return (
     <Fragment>
       {fetchedState.anchorStatus.value === LoadingStatus.FAILED ? (
@@ -184,8 +179,6 @@ export const ContextApp = ({ dataView, anchorId, referrer }: ContextAppProps) =>
             id="contextAppTitle"
             className="euiScreenReaderOnly"
             data-test-subj="discoverContextAppTitle"
-            tabIndex={-1}
-            ref={contextAppTitle}
           >
             {i18n.translate('discover.context.pageTitle', {
               defaultMessage: 'Documents surrounding #{anchorId}',

diff --git a/src/plugins/discover/public/application/doc/components/doc.tsx b/src/plugins/discover/public/application/doc/components/doc.tsx
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-import React, { useEffect, useRef } from 'react';
+import React, { useEffect } from 'react';
 import { FormattedMessage } from '@kbn/i18n-react';
 import { EuiCallOut, EuiLink, EuiLoadingSpinner, EuiPage, EuiPageBody } from '@elastic/eui';
 import type { DataView } from '@kbn/data-views-plugin/public';
@@ -46,11 +46,6 @@ export function Doc(props: DocProps) {
   const { locator, chrome, docLinks } = useDiscoverServices();
   const indexExistsLink = docLinks.links.apis.indexExists;
 
-  const singleDocTitle = useRef<HTMLHeadingElement>(null);
-  useEffect(() => {
-    singleDocTitle.current?.focus();
-  }, []);
-
   useEffect(() => {
     chrome.setBreadcrumbs([
       ...getRootBreadcrumbs(props.referrer),
@@ -64,8 +59,6 @@ export function Doc(props: DocProps) {
         id="singleDocTitle"
         className="euiScreenReaderOnly"
         data-test-subj="discoverSingleDocTitle"
-        tabIndex={-1}
-        ref={singleDocTitle}
       >
         {i18n.translate('discover.doc.pageTitle', {
           defaultMessage: 'Single document - #{id}',

diff --git a/src/plugins/discover/public/application/main/components/layout/discover_layout.test.tsx b/src/plugins/discover/public/application/main/components/layout/discover_layout.test.tsx
@@ -161,17 +161,6 @@ describe('Discover component', () => {
     ).not.toBeNull();
   }, 10000);
 
-  test('the saved search title h1 gains focus on navigate', async () => {
-    const container = document.createElement('div');
-    document.body.appendChild(container);
-    const component = await mountComponent(dataViewWithTimefieldMock, undefined, {
-      attachTo: container,
-    });
-    expect(
-      component.find('[data-test-subj="discoverSavedSearchTitle"]').getDOMNode()
-    ).toHaveFocus();
-  }, 10000);
-
   describe('sidebar', () => {
     test('should be opened if discover:sidebarClosed was not set', async () => {
       const component = await mountComponent(dataViewWithTimefieldMock, undefined);

diff --git a/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx b/src/plugins/discover/public/application/main/components/layout/discover_layout.tsx
@@ -175,11 +175,6 @@ export function DiscoverLayout({ navigateTo, stateContainer }: DiscoverLayoutPro
 
   const contentCentered = resultState === 'uninitialized' || resultState === 'none';
 
-  const savedSearchTitle = useRef<HTMLHeadingElement>(null);
-  useEffect(() => {
-    savedSearchTitle.current?.focus();
-  }, []);
-
   const textBasedLanguageModeErrors = useMemo(() => {
     if (isPlainRecord) {
       return dataState.error;
@@ -255,8 +250,6 @@ export function DiscoverLayout({ navigateTo, stateContainer }: DiscoverLayoutPro
         id="savedSearchTitle"
         className="euiScreenReaderOnly"
         data-test-subj="discoverSavedSearchTitle"
-        tabIndex={-1}
-        ref={savedSearchTitle}
       >
         {savedSearch.title
           ? i18n.translate('discover.pageTitleWithSavedSearch', {

diff --git a/test/functional/apps/context/_context_accessibility.ts b/test/functional/apps/context/_context_accessibility.ts
@@ -32,22 +32,17 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       await kibanaServer.uiSettings.replace({});
     });
 
-    it('should navigate to the single doc view and give focus to the title h1 on navigate', async () => {
+    it('should give focus to the table Load link when Tab is pressed', async () => {
       await dataGrid.clickRowToggle({ rowIndex: 0 });
       const rowActions = await dataGrid.getRowActions({ rowIndex: 0 });
       await rowActions[1].click();
-      const titleElement = await testSubjects.find('discoverContextAppTitle');
-      const activeElement = await find.activeElement();
-      expect(await titleElement.getAttribute('data-test-subj')).to.eql(
-        await activeElement.getAttribute('data-test-subj')
-      );
-    });
-
-    it('should give focus to the table tab link when Tab is pressed', async () => {
+      await PageObjects.header.waitUntilLoadingHasFinished();
+      await browser.pressKeys(browser.keys.TAB);
+      await browser.pressKeys(browser.keys.SPACE);
       await browser.pressKeys(browser.keys.TAB);
-      const dataViewSwitchLink = await testSubjects.find('showQueryBarMenu');
+      const loadMoreLink = await testSubjects.find('predecessorsLoadMoreButton');
       const activeElement = await find.activeElement();
-      expect(await dataViewSwitchLink.getAttribute('data-test-subj')).to.eql(
+      expect(await loadMoreLink.getAttribute('data-test-subj')).to.eql(
         await activeElement.getAttribute('data-test-subj')
       );
     });

diff --git a/test/functional/apps/discover/group1/_discover_accessibility.ts b/test/functional/apps/discover/group1/_discover_accessibility.ts
@@ -31,8 +31,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
     return (await targetElement._webElement.getId()) === (await activeElement._webElement.getId());
   };
 
-  // Failing: See https://github.com/elastic/kibana/issues/152938
-  describe.skip('discover accessibility', () => {
+  describe('discover accessibility', () => {
     before(async () => {
       log.debug('load kibana index with default index pattern');
       await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover');
@@ -46,15 +45,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       await kibanaServer.savedObjects.clean({ types: ['search', 'index-pattern'] });
     });
 
-    it('should give focus to the saved search title h1 on navigate', async () => {
-      expect(await hasFocus('discoverSavedSearchTitle')).to.be(true);
-    });
-
-    it('should give focus to the data view switch link when Tab is pressed', async () => {
-      await browser.pressKeys(browser.keys.TAB);
-      expect(await hasFocus('discover-dataView-switch-link')).to.be(true);
-    });
-
     describe('top nav menu buttons', () => {
       const focusAndPressButton = async (buttonTestSubject: string | WebElementWrapper) => {
         const button =

diff --git a/test/functional/apps/discover/group1/_doc_accessibility.ts b/test/functional/apps/discover/group1/_doc_accessibility.ts
@@ -40,20 +40,15 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       await kibanaServer.savedObjects.clean({ types: ['search', 'index-pattern'] });
     });
 
-    it('should navigate to the single doc view and give focus to the title h1 on navigate', async () => {
+    it('should give focus to the first tab link when Tab is pressed', async () => {
       await dataGrid.clickRowToggle({ rowIndex: 0 });
       const rowActions = await dataGrid.getRowActions({ rowIndex: 0 });
       await rowActions[0].click();
-      const titleElement = await testSubjects.find('discoverSingleDocTitle');
-      const activeElement = await find.activeElement();
-      expect(await titleElement.getAttribute('data-test-subj')).to.eql(
-        await activeElement.getAttribute('data-test-subj')
-      );
-    });
-
-    it('should give focus to the first tab link when Tab is pressed', async () => {
-      const tableTab = await testSubjects.find('docViewerTab-0');
+      await PageObjects.header.waitUntilLoadingHasFinished();
       await browser.pressKeys(browser.keys.TAB);
+      await browser.pressKeys(browser.keys.SPACE);
+      await browser.pressKeys(browser.keys.TAB);
+      const tableTab = await testSubjects.find('docViewerTab-0');
       const activeElement = await find.activeElement();
       expect(await tableTab.getAttribute('data-test-subj')).to.eql(
         await activeElement.getAttribute('data-test-subj')