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

[Dashboard Navigation] Fix a11y concerns #174772

Merged
merged 14 commits into from
Jan 18, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ export const DashboardLinkComponent = ({
label={linkLabel}
external={link.options?.openInNewTab}
data-test-subj={error ? `${id}--error` : `${id}`}
aria-current={link.destination === parentDashboardId}
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('LinksEditor', () => {
onAddToDashboard: jest.fn(),
onClose: jest.fn(),
isByReference: false,
flyoutId: 'test-id',
};

const someLinks: Link[] = [
Expand Down
12 changes: 10 additions & 2 deletions src/plugins/links/public/components/editor/links_editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const LinksEditor = ({
initialLayout,
parentDashboard,
isByReference,
flyoutId,
}: {
onSaveToLibrary: (newLinks: Link[], newLayout: LinksLayoutType) => Promise<void>;
onAddToDashboard: (newLinks: Link[], newLayout: LinksLayoutType) => void;
Expand All @@ -79,6 +80,7 @@ const LinksEditor = ({
initialLayout?: LinksLayoutType;
parentDashboard?: DashboardContainer;
isByReference: boolean;
flyoutId: string; // used to manage the focus of this flyout after individual link editor flyout is closed
}) => {
const toasts = coreServices.notifications.toasts;
const isMounted = useMountedState();
Expand Down Expand Up @@ -120,6 +122,7 @@ const LinksEditor = ({
const newLink = await openLinkEditorFlyout({
parentDashboard,
link: linkToEdit,
mainFlyoutId: flyoutId,
ref: editLinkFlyoutRef,
});
if (newLink) {
Expand All @@ -137,7 +140,7 @@ const LinksEditor = ({
}
}
},
[editLinkFlyoutRef, orderedLinks, parentDashboard]
[editLinkFlyoutRef, orderedLinks, parentDashboard, flyoutId]
);

const hasZeroLinks = useMemo(() => {
Expand Down Expand Up @@ -172,7 +175,12 @@ const LinksEditor = ({
<EuiFlexItem grow={false}>
<EuiToolTip content={LinksStrings.editor.panelEditor.getTechnicalPreviewTooltip()}>
{/* The EuiBadge needs an empty title to prevent the default tooltip */}
<EuiBadge color="hollow" tabIndex={0} title="">
<EuiBadge
color="hollow"
tabIndex={0}
title=""
aria-label={LinksStrings.editor.panelEditor.getTechnicalPreviewTooltip()}
>
{LinksStrings.editor.panelEditor.getTechnicalPreviewLabel()}
</EuiBadge>
</EuiToolTip>
nickpeihl marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export const LinksEditorSingleLink = ({
size="xs"
iconType="pencil"
onClick={editLink}
aria-label={LinksStrings.editor.getEditLinkTitle()}
aria-label={LinksStrings.editor.getEditLinkTitle(linkLabel)}
data-test-subj="panelEditorLink--editBtn"
/>
</EuiToolTip>
Expand All @@ -170,7 +170,7 @@ export const LinksEditorSingleLink = ({
<EuiButtonIcon
size="xs"
iconType="trash"
aria-label={LinksStrings.editor.getDeleteLinkTitle()}
aria-label={LinksStrings.editor.getDeleteLinkTitle(linkLabel)}
color="danger"
onClick={deleteLink}
data-test-subj="panelEditorLink--deleteBtn"
Expand Down
12 changes: 7 additions & 5 deletions src/plugins/links/public/components/links_strings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ export const LinksStrings = {
i18n.translate('links.editor.updateButtonLabel', {
defaultMessage: 'Update link',
}),
getEditLinkTitle: () =>
i18n.translate('links.editor.editLinkTitle', {
defaultMessage: 'Edit link',
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
getEditLinkTitle: (label?: string) =>
i18n.translate('links.editor.editLinkTitle.hasLabel', {
defaultMessage: 'Edit {label} link',
values: { label: label ?? '' },
}),
getDeleteLinkTitle: () =>
getDeleteLinkTitle: (label?: string) =>
i18n.translate('links.editor.deleteLinkTitle', {
defaultMessage: 'Delete link',
nickpeihl marked this conversation as resolved.
Show resolved Hide resolved
defaultMessage: 'Delete {label} link',
values: { label: label ?? '' },
}),
getCancelButtonLabel: () =>
i18n.translate('links.editor.cancelButtonLabel', {
Expand Down
8 changes: 7 additions & 1 deletion src/plugins/links/public/editor/open_editor_flyout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import React from 'react';
import { skip, take } from 'rxjs/operators';
import { v4 as uuidv4 } from 'uuid';

import { EuiLoadingSpinner, EuiPanel } from '@elastic/eui';
import { DashboardContainer } from '@kbn/dashboard-plugin/public/dashboard_container';
Expand Down Expand Up @@ -58,6 +59,8 @@ export async function openEditorFlyout(
}

return new Promise((resolve, reject) => {
const flyoutId = `linksEditorFlyout-${uuidv4()}`;

const closeEditorFlyout = (editorFlyout: OverlayRef) => {
if (overlayTracker) {
overlayTracker.clearOverlays();
Expand Down Expand Up @@ -124,6 +127,7 @@ export async function openEditorFlyout(
const editorFlyout = coreServices.overlays.openFlyout(
toMountPoint(
<LinksEditor
flyoutId={flyoutId}
initialLinks={initialLinks}
initialLayout={attributes?.layout}
onClose={onCancel}
Expand All @@ -135,10 +139,12 @@ export async function openEditorFlyout(
{ theme: coreServices.theme, i18n: coreServices.i18n }
),
{
tabIndex: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

@Heenawter Which flyout is this tabIndex being assigned to? I wasn't able to find it in the DOM using a quick document.querySelectorAll('[tabindex="1"]') but had some unexpected tabbing behavior with Firefox + VO. It's entirely possible the new MacOS or VO has disrupted that flow; Firefox seemed okay by itself.

If there is a positive tabindex being applied, that could disrupt the natural tabbing order, so I'd like to evaluate that separately if it does appear in a place I didn't find.

Copy link
Contributor Author

@Heenawter Heenawter Jan 18, 2024

Choose a reason for hiding this comment

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

Oh, good catch - I think I accidentally left this in when I was testing different things for refocusing the main flyout after editing/adding links. I've removed it, and it doesn't seem to impact the focusing behaviour 🙇

id: flyoutId,
maxWidth: 720,
ownFocus: true,
outsideClickCloses: false,
onClose: onCancel,
outsideClickCloses: false,
className: 'linksPanelEditor',
'data-test-subj': 'links--panelEditor--flyout',
}
Expand Down
8 changes: 8 additions & 0 deletions src/plugins/links/public/editor/open_link_editor_flyout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { LinkEditor } from '../components/editor/link_editor';
export interface LinksEditorProps {
link?: Link;
parentDashboard?: DashboardContainer;
mainFlyoutId: string;
ref: React.RefObject<HTMLDivElement>;
}

Expand All @@ -34,6 +35,7 @@ export type UnorderedLink = Omit<Link, 'order'>;
export async function openLinkEditorFlyout({
ref,
link,
mainFlyoutId, // used to manage the focus of this flyout after inidividual link editor flyout is closed
parentDashboard,
}: LinksEditorProps): Promise<UnorderedLink | undefined> {
const unmountFlyout = async () => {
Expand All @@ -44,6 +46,12 @@ export async function openLinkEditorFlyout({
// wait for close animation before unmounting
setTimeout(() => {
if (ref.current) ReactDOM.unmountComponentAtNode(ref.current);

// return focus to the main flyout div to align with a11y standards
const flyoutElement = document.getElementById(mainFlyoutId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This works well when I add and edit links. Can we include logic to also reset this focus to the modal container when I delete a link using the inline trashcan icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 11468ac 🎉

if (flyoutElement) {
flyoutElement.focus();
}
}, 180);
});
};
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/translations/translations/fr-FR.json
Original file line number Diff line number Diff line change
Expand Up @@ -4778,8 +4778,6 @@
"links.description": "Utiliser des liens pour accéder aux tableaux de bord et aux sites web couramment utilisés.",
"links.editor.addButtonLabel": "Ajouter un lien",
"links.editor.cancelButtonLabel": "Fermer",
"links.editor.deleteLinkTitle": "Supprimer le lien",
"links.editor.editLinkTitle": "Modifier le lien",
"links.editor.horizontalLayout": "Horizontal",
"links.editor.unableToSaveToastTitle": "Erreur lors de l'enregistrement du Panneau de liens",
"links.editor.updateButtonLabel": "Mettre à jour le lien",
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -4793,8 +4793,6 @@
"links.description": "リンクを使用して、よく使用されるダッシュボードや Web サイトに移動します。",
"links.editor.addButtonLabel": "リンクを追加",
"links.editor.cancelButtonLabel": "閉じる",
"links.editor.deleteLinkTitle": "リンクを削除",
"links.editor.editLinkTitle": "リンクを編集",
"links.editor.horizontalLayout": "横",
"links.editor.unableToSaveToastTitle": "リンクパネルの保存エラー",
"links.editor.updateButtonLabel": "リンクを更新",
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -4886,8 +4886,6 @@
"links.description": "使用链接以导航到常用仪表板和网站。",
"links.editor.addButtonLabel": "添加链接",
"links.editor.cancelButtonLabel": "关闭",
"links.editor.deleteLinkTitle": "删除链接",
"links.editor.editLinkTitle": "编辑链接",
"links.editor.horizontalLayout": "水平",
"links.editor.unableToSaveToastTitle": "保存链接面板时出错",
"links.editor.updateButtonLabel": "更新链接",
Expand Down
81 changes: 81 additions & 0 deletions x-pack/test/accessibility/apps/group1/dashboard_links.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getService, getPageObjects }: FtrProviderContext) {
const a11y = getService('a11y');
const deployment = getService('deployment');
const testSubjects = getService('testSubjects');
const kibanaServer = getService('kibanaServer');
const dashboardAddPanel = getService('dashboardAddPanel');
const { common, dashboard, home, dashboardLinks } = getPageObjects([
'common',
'dashboard',
'home',
'dashboardLinks',
]);

const DASHBOARD_NAME = 'Test Links Panel A11y';

describe('Dashboard links a11y tests', () => {
before(async () => {
await kibanaServer.savedObjects.cleanStandardList();
await common.navigateToUrl('home', '/tutorial_directory/sampleData', {
useActualUrl: true,
});
await home.addSampleDataSet('flights');
await common.navigateToApp('dashboard');
await dashboard.gotoDashboardLandingPage();
await dashboard.clickNewDashboard();
await dashboard.saveDashboard(DASHBOARD_NAME, { exitFromEditMode: false });
});

after(async () => {
await dashboard.clickQuickSave();
await common.navigateToUrl('home', '/tutorial_directory/sampleData', {
useActualUrl: true,
});
await home.removeSampleDataSet('flights');
await kibanaServer.savedObjects.cleanStandardList();
});

it('Empty links editor flyout', async () => {
await dashboardAddPanel.clickEditorMenuButton();
await dashboardAddPanel.clickAddNewEmbeddableLink('links');
await a11y.testAppSnapshot();
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
});

it('Add dashboard link flyout', async () => {
await testSubjects.click('links--panelEditor--addLinkBtn');
await testSubjects.exists('links--linkEditor--flyout');
await a11y.testAppSnapshot();
});

it('Add external link flyout', async () => {
const radioOption = await testSubjects.find('links--linkEditor--externalLink--radioBtn');
const label = await radioOption.findByCssSelector('label[for="externalLink"]');
await label.click();
await a11y.testAppSnapshot();
await dashboardLinks.clickLinkEditorCloseButton();
});

it('Non-empty links editor flyout', async () => {
await dashboardLinks.addDashboardLink('[Flights] Global Flight Dashboard');
await dashboardLinks.addDashboardLink(DASHBOARD_NAME);
await dashboardLinks.addExternalLink(`${deployment.getHostPort()}/app/bar`);
await a11y.testAppSnapshot();
});

it('Links panel', async () => {
await dashboardLinks.toggleSaveByReference(false);
await dashboardLinks.clickPanelEditorSaveButton();
await testSubjects.existOrFail('links--component');
await a11y.testAppSnapshot();
});
});
}
3 changes: 2 additions & 1 deletion x-pack/test/accessibility/apps/group1/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ export default ({ loadTestFile }: FtrProviderContext): void => {
loadTestFile(require.resolve('./uptime'));
loadTestFile(require.resolve('./spaces'));
loadTestFile(require.resolve('./advanced_settings'));
loadTestFile(require.resolve('./dashboard_panel_options'));
loadTestFile(require.resolve('./dashboard_controls'));
loadTestFile(require.resolve('./dashboard_links'));
loadTestFile(require.resolve('./dashboard_panel_options'));
loadTestFile(require.resolve('./users'));
loadTestFile(require.resolve('./roles'));
loadTestFile(require.resolve('./ingest_node_pipelines'));
Expand Down