Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add Tooltip to AccessibleButton #12443

Merged
merged 29 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a3fa53c
Deprecate AccessibleTooltipButton
t3chguy Jan 15, 2024
5cdd435
Fix tests
t3chguy Jan 15, 2024
d4ef09a
Fix tests
t3chguy Jan 15, 2024
597234d
Merge remote-tracking branch 'origin/t3chguy/fix/26762.2' into t3chgu…
t3chguy Jan 15, 2024
43768d8
Deprecate AccessibleTooltipButton
t3chguy Jan 15, 2024
2c69bcb
Fix tests
t3chguy Jan 15, 2024
8dd69e6
Merge remote-tracking branch 'origin/t3chguy/fix/26762.2' into t3chgu…
t3chguy Jan 15, 2024
42d7566
Iterate
t3chguy Jan 15, 2024
9d8b93d
Merge branch 'refs/heads/develop' into t3chguy/fix/26762.2
florianduros Apr 16, 2024
3d6f791
Fix `UserInfo-test.tsx`
florianduros Apr 16, 2024
5d1b5e6
Update `LoginWithQRFlow-test.tsx` snapshot
florianduros Apr 16, 2024
5dd8b68
Merge branch 'refs/heads/develop' into t3chguy/fix/26762.2
florianduros Apr 18, 2024
7ac5665
Remove tooltip provider from test
florianduros Apr 18, 2024
058dcf6
Fix `AccessibleButton`
florianduros Apr 18, 2024
14c0f22
Update snapshots
florianduros Apr 18, 2024
d324a1c
Revert to original import
florianduros Apr 19, 2024
ad7048a
Use title to populate aria-label
florianduros Apr 19, 2024
52909ee
Rollback AccessibleButton or Tooltip changes. Will come in another PR
florianduros Apr 19, 2024
3ddd50a
Rollback en.json change
florianduros Apr 19, 2024
fa9f3d5
Update snapshots
florianduros Apr 19, 2024
9321344
Fix `UserInfo`
florianduros Apr 19, 2024
0e32335
Update snapshots
florianduros Apr 19, 2024
f276b20
Use label instead of title in test
florianduros Apr 19, 2024
e7da4b8
Merge branch 'develop' into florianduros/add-tooltip-accessiblebutton
florianduros Apr 19, 2024
21d568d
Use label instead of title in TAC test
florianduros Apr 19, 2024
964d879
Use label instead of title in read-receipt test
florianduros Apr 19, 2024
bc2530d
Remove tooltip for ContextMenu
florianduros Apr 22, 2024
123fe12
Merge branch 'develop' into florianduros/add-tooltip-accessiblebutton
florianduros Apr 22, 2024
7d7539d
Add extra information for caption field
florianduros Apr 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions playwright/e2e/read-receipts/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,15 +403,15 @@ class Helpers {
* tests we only open the threads panel.)
*/
async closeThreadsPanel() {
await this.page.locator(".mx_RightPanel").getByTitle("Close").click();
await this.page.locator(".mx_RightPanel").getByLabel("Close").click();
await expect(this.page.locator(".mx_RightPanel")).not.toBeVisible();
}

/**
* Return to the list of threads, given we are viewing a single thread.
*/
async backToThreadsList() {
await this.page.locator(".mx_RightPanel").getByTitle("Threads").click();
await this.page.locator(".mx_RightPanel").getByLabel("Threads").click();
}

/**
Expand Down Expand Up @@ -539,7 +539,7 @@ class Helpers {
const threadPanel = this.page.locator(".mx_ThreadPanel");
await expect(threadPanel).toBeVisible();
await threadPanel.evaluate(($panel) => {
const $button = $panel.querySelector<HTMLElement>('.mx_BaseCard_back[title="Threads"]');
const $button = $panel.querySelector<HTMLElement>('.mx_BaseCard_back[aria-label="Threads"]');
// If the Threads back button is present then click it - the
// threads button can open either threads list or thread panel
if ($button) {
Expand Down
2 changes: 1 addition & 1 deletion playwright/e2e/spaces/threads-activity-centre/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ export class Helpers {
*/
assertThreadPanelFocused() {
return expect(
this.page.locator(".mx_ThreadPanel").locator(".mx_BaseCard_header").getByTitle("Close"),
this.page.locator(".mx_ThreadPanel").locator(".mx_BaseCard_header").getByLabel("Close"),
).toBeFocused();
}

Expand Down
1 change: 0 additions & 1 deletion src/accessibility/context_menu/ContextMenuButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ export const ContextMenuButton = forwardRef(function <T extends keyof JSX.Intrin
{...props}
onClick={onClick}
onContextMenu={onContextMenu ?? onClick ?? undefined}
title={label}
richvdh marked this conversation as resolved.
Show resolved Hide resolved
aria-label={label}
aria-haspopup={true}
aria-expanded={isExpanded}
Expand Down
24 changes: 23 additions & 1 deletion src/components/views/elements/AccessibleButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import React, { forwardRef, FunctionComponent, HTMLAttributes, InputHTMLAttributes, Ref } from "react";
import classnames from "classnames";
import { Tooltip } from "@vector-im/compound-web";

import { getKeyBindingsManager } from "../../../KeyBindingsManager";
import { KeyBindingAction } from "../../../accessibility/KeyboardShortcuts";
Expand Down Expand Up @@ -86,6 +87,15 @@ type Props<T extends keyof JSX.IntrinsicElements> = DynamicHtmlElementProps<T> &
* Event handler for button activation. Should be implemented exactly like a normal `onClick` handler.
*/
onClick: ((e: ButtonEvent) => void | Promise<void>) | null;
/**
* The tooltip to show on hover or focus.
*/
title?: string;
/**
* The caption is a secondary text displayed under the `title` of the tooltip.
* Only valid when used in conjunction with `title`.
*/
caption?: string;
};

/**
Expand Down Expand Up @@ -116,11 +126,14 @@ const AccessibleButton = forwardRef(function <T extends keyof JSX.IntrinsicEleme
onKeyDown,
onKeyUp,
triggerOnMouseDown,
title,
caption,
...restProps
}: Props<T>,
ref: Ref<HTMLElement>,
): JSX.Element {
const newProps: RenderedElementProps = restProps;
newProps["aria-label"] = newProps["aria-label"] ?? title;
if (disabled) {
newProps["aria-disabled"] = true;
newProps["disabled"] = true;
Expand Down Expand Up @@ -182,7 +195,16 @@ const AccessibleButton = forwardRef(function <T extends keyof JSX.IntrinsicEleme
});

// React.createElement expects InputHTMLAttributes
return React.createElement(element, newProps, children);
const button = React.createElement(element, newProps, children);

if (title) {
return (
<Tooltip label={title} caption={caption} isTriggerInteractive={!disabled}>
{button}
</Tooltip>
);
}
return button;
});

// Type assertion required due to forwardRef type workaround in react.d.ts
Expand Down
3 changes: 3 additions & 0 deletions src/components/views/elements/AccessibleTooltipButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ type Props<T extends keyof JSX.IntrinsicElements> = ComponentProps<typeof Access
onHideTooltip?(ev: SyntheticEvent): void;
};

/**
* @deprecated use AccessibleButton with `title` and `caption` instead.
*/
const AccessibleTooltipButton = forwardRef(function <T extends keyof JSX.IntrinsicElements>(
{ title, tooltip, children, forceHide, alignment, onHideTooltip, tooltipClassName, ...props }: Props<T>,
ref: Ref<HTMLElement>,
Expand Down
7 changes: 6 additions & 1 deletion src/components/views/right_panel/UserInfo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,12 @@ export function DeviceItem({
);
} else {
return (
<AccessibleButton className={classes} title={device.deviceId} onClick={onDeviceClick}>
<AccessibleButton
className={classes}
title={device.deviceId}
aria-label={deviceName}
onClick={onDeviceClick}
>
<div className={iconClasses} />
<div className="mx_UserInfo_device_name">{deviceName}</div>
<div className="mx_UserInfo_device_trusted">{trustedLabel}</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ exports[`<UserMenu> <UserMenu> when video broadcast when rendered should render
class="mx_AccessibleButton mx_UserMenu_contextMenuButton"
role="button"
tabindex="0"
title="User menu"
>
<div
class="mx_UserMenu_userAvatar"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ exports[`<DialogSidebar /> renders sidebar correctly with beacons 1`] = `
View list
</h4>
<div
aria-label="Close sidebar"
class="mx_AccessibleButton mx_DialogSidebar_closeButton"
data-state="closed"
richvdh marked this conversation as resolved.
Show resolved Hide resolved
data-testid="dialog-sidebar-close"
role="button"
tabindex="0"
title="Close sidebar"
>
<div
class="mx_DialogSidebar_closeButtonIcon"
Expand Down Expand Up @@ -113,11 +114,12 @@ exports[`<DialogSidebar /> renders sidebar correctly without beacons 1`] = `
View list
</h4>
<div
aria-label="Close sidebar"
class="mx_AccessibleButton mx_DialogSidebar_closeButton"
data-state="closed"
data-testid="dialog-sidebar-close"
role="button"
tabindex="0"
title="Close sidebar"
>
<div
class="mx_DialogSidebar_closeButtonIcon"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
exports[`<LeftPanelLiveShareWarning /> when user has live location monitor renders correctly when minimized 1`] = `
<DocumentFragment>
<div
aria-label="You are sharing your live location"
class="mx_AccessibleButton mx_LeftPanelLiveShareWarning mx_LeftPanelLiveShareWarning__minimized"
data-state="closed"
role="button"
tabindex="0"
title="You are sharing your live location"
>
<div
height="10"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,12 @@ exports[`<RoomLiveShareWarning /> when user has live beacons and geolocation is
Retry
</button>
<button
aria-label="Stop and close"
class="mx_AccessibleButton mx_RoomLiveShareWarning_closeButton"
data-state="closed"
data-testid="room-live-share-wire-error-close-button"
role="button"
tabindex="0"
title="Stop and close"
>
<div
class="mx_RoomLiveShareWarning_closeButtonIcon"
Expand Down
8 changes: 4 additions & 4 deletions test/components/views/elements/AppTile-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -377,12 +377,12 @@ describe("AppTile", () => {
});

it("clicking 'minimise' should send the widget to the right", async () => {
await userEvent.click(renderResult.getByTitle("Minimise"));
await userEvent.click(renderResult.getByLabelText("Minimise"));
expect(moveToContainerSpy).toHaveBeenCalledWith(r1, app1, Container.Right);
});

it("clicking 'maximise' should send the widget to the center", async () => {
await userEvent.click(renderResult.getByTitle("Maximise"));
await userEvent.click(renderResult.getByLabelText("Maximise"));
expect(moveToContainerSpy).toHaveBeenCalledWith(r1, app1, Container.Center);
});

Expand Down Expand Up @@ -435,7 +435,7 @@ describe("AppTile", () => {
});

it("clicking 'un-maximise' should send the widget to the top", async () => {
await userEvent.click(renderResult.getByTitle("Un-maximise"));
await userEvent.click(renderResult.getByLabelText("Un-maximise"));
expect(moveToContainerSpy).toHaveBeenCalledWith(r1, app1, Container.Top);
});
});
Expand All @@ -461,7 +461,7 @@ describe("AppTile", () => {
});

it("should display the »Popout widget« button", () => {
expect(renderResult.getByTitle("Popout widget")).toBeInTheDocument();
expect(renderResult.getByLabelText("Popout widget")).toBeInTheDocument();
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ exports[`AppTile destroys non-persisted right panel widget on room change 1`] =
class="mx_BaseCard_header"
>
<div
aria-label="Close"
class="mx_AccessibleButton mx_BaseCard_close"
data-state="closed"
data-testid="base-card-close-button"
role="button"
tabindex="0"
title="Close"
/>
<div
class="mx_BaseCard_headerProp"
Expand All @@ -37,7 +38,6 @@ exports[`AppTile destroys non-persisted right panel widget on room change 1`] =
class="mx_AccessibleButton mx_BaseCard_header_title_button--option"
role="button"
tabindex="0"
title="Options"
/>
</div>
</div>
Expand Down Expand Up @@ -136,20 +136,22 @@ exports[`AppTile for a pinned widget should render 1`] = `
class="mx_AppTileMenuBar_widgets"
>
<div
aria-label="Un-maximise"
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
data-state="closed"
role="button"
tabindex="0"
title="Un-maximise"
>
<div
class="mx_Icon mx_Icon_12"
/>
</div>
<div
aria-label="Minimise"
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
data-state="closed"
role="button"
tabindex="0"
title="Minimise"
>
<div
class="mx_Icon mx_Icon_12"
Expand All @@ -162,7 +164,6 @@ exports[`AppTile for a pinned widget should render 1`] = `
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
role="button"
tabindex="0"
title="Options"
>
<div
class="mx_Icon mx_Icon_12"
Expand Down Expand Up @@ -223,20 +224,22 @@ exports[`AppTile for a pinned widget should render permission request 1`] = `
class="mx_AppTileMenuBar_widgets"
>
<div
aria-label="Un-maximise"
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
data-state="closed"
role="button"
tabindex="0"
title="Un-maximise"
>
<div
class="mx_Icon mx_Icon_12"
/>
</div>
<div
aria-label="Minimise"
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
data-state="closed"
role="button"
tabindex="0"
title="Minimise"
>
<div
class="mx_Icon mx_Icon_12"
Expand All @@ -249,7 +252,6 @@ exports[`AppTile for a pinned widget should render permission request 1`] = `
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
role="button"
tabindex="0"
title="Options"
>
<div
class="mx_Icon mx_Icon_12"
Expand Down Expand Up @@ -377,20 +379,22 @@ exports[`AppTile preserves non-persisted widget on container move 1`] = `
class="mx_AppTileMenuBar_widgets"
>
<div
aria-label="Maximise"
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
data-state="closed"
role="button"
tabindex="0"
title="Maximise"
>
<div
class="mx_Icon mx_Icon_12"
/>
</div>
<div
aria-label="Minimise"
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
data-state="closed"
role="button"
tabindex="0"
title="Minimise"
>
<div
class="mx_Icon mx_Icon_12"
Expand All @@ -403,7 +407,6 @@ exports[`AppTile preserves non-persisted widget on container move 1`] = `
class="mx_AccessibleButton mx_AppTileMenuBar_widgets_button"
role="button"
tabindex="0"
title="Options"
>
<div
class="mx_Icon mx_Icon_12"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,24 @@ exports[`<LocationViewDialog /> renders map correctly 1`] = `
class="mx_ZoomButtons"
>
<div
aria-label="Zoom in"
class="mx_AccessibleButton mx_ZoomButtons_button"
data-state="closed"
data-testid="map-zoom-in-button"
role="button"
tabindex="0"
title="Zoom in"
>
<div
class="mx_ZoomButtons_icon"
/>
</div>
<div
aria-label="Zoom out"
class="mx_AccessibleButton mx_ZoomButtons_button"
data-state="closed"
data-testid="map-zoom-out-button"
role="button"
tabindex="0"
title="Zoom out"
>
<div
class="mx_ZoomButtons_icon"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,24 @@ exports[`<ZoomButtons /> renders buttons 1`] = `
class="mx_ZoomButtons"
>
<div
aria-label="Zoom in"
class="mx_AccessibleButton mx_ZoomButtons_button"
data-state="closed"
data-testid="map-zoom-in-button"
role="button"
tabindex="0"
title="Zoom in"
>
<div
class="mx_ZoomButtons_icon"
/>
</div>
<div
aria-label="Zoom out"
class="mx_AccessibleButton mx_ZoomButtons_button"
data-state="closed"
data-testid="map-zoom-out-button"
role="button"
tabindex="0"
title="Zoom out"
>
<div
class="mx_ZoomButtons_icon"
Expand Down
Loading
Loading