Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
17 changes: 7 additions & 10 deletions src/accessibility/context_menu/ContextMenuButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { ComponentProps } from "react";
import React, { ComponentProps, forwardRef, Ref } from "react";

import AccessibleButton from "../../components/views/elements/AccessibleButton";

Expand All @@ -27,14 +27,10 @@ type Props<T extends keyof JSX.IntrinsicElements> = ComponentProps<typeof Access
};

// Semantic component for representing the AccessibleButton which launches a <ContextMenu />
export const ContextMenuButton = <T extends keyof JSX.IntrinsicElements>({
label,
isExpanded,
children,
onClick,
onContextMenu,
...props
}: Props<T>): JSX.Element => {
export const ContextMenuButton = forwardRef(function <T extends keyof JSX.IntrinsicElements>(
{ label, isExpanded, children, onClick, onContextMenu, ...props }: Props<T>,
ref: Ref<HTMLElement>,
) {
return (
<AccessibleButton
{...props}
Expand All @@ -44,8 +40,9 @@ export const ContextMenuButton = <T extends keyof JSX.IntrinsicElements>({
aria-label={label}
aria-haspopup={true}
aria-expanded={isExpanded}
ref={ref}
>
{children}
</AccessibleButton>
);
};
});
16 changes: 7 additions & 9 deletions src/accessibility/context_menu/ContextMenuTooltipButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { ComponentProps } from "react";
import React, { ComponentProps, forwardRef, Ref } from "react";

import AccessibleTooltipButton from "../../components/views/elements/AccessibleTooltipButton";

Expand All @@ -26,13 +26,10 @@ type Props<T extends keyof JSX.IntrinsicElements> = ComponentProps<typeof Access
};

// Semantic component for representing the AccessibleButton which launches a <ContextMenu />
export const ContextMenuTooltipButton = <T extends keyof JSX.IntrinsicElements>({
isExpanded,
children,
onClick,
onContextMenu,
...props
}: Props<T>): JSX.Element => {
export const ContextMenuTooltipButton = forwardRef(function <T extends keyof JSX.IntrinsicElements>(
{ isExpanded, children, onClick, onContextMenu, ...props }: Props<T>,
ref: Ref<HTMLElement>,
) {
return (
<AccessibleTooltipButton
{...props}
Expand All @@ -41,8 +38,9 @@ export const ContextMenuTooltipButton = <T extends keyof JSX.IntrinsicElements>(
aria-haspopup={true}
aria-expanded={isExpanded}
forceHide={isExpanded}
ref={ref}
>
{children}
</AccessibleTooltipButton>
);
};
});
2 changes: 1 addition & 1 deletion src/accessibility/roving/RovingAccessibleButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const RovingAccessibleButton = <T extends keyof JSX.IntrinsicElements>({
if (focusOnMouseOver) onFocusInternal();
onMouseOver?.(event);
}}
inputRef={ref}
ref={ref}
tabIndex={isActive ? 0 : -1}
/>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const RovingAccessibleTooltipButton = <T extends keyof JSX.IntrinsicEleme
onFocusInternal();
onFocus?.(event);
}}
inputRef={ref}
ref={ref}
tabIndex={isActive ? 0 : -1}
/>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/GenericDropdownMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ export function GenericDropdownMenu<T>({
<>
<ContextMenuButton
className="mx_GenericDropdownMenu_button"
inputRef={button}
ref={button}
isExpanded={menuDisplayed}
onClick={(ev: ButtonEvent) => {
openMenu();
Expand Down
5 changes: 3 additions & 2 deletions src/components/structures/SpaceHierarchy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
*/

import React, {
ComponentProps,
Dispatch,
KeyboardEvent,
KeyboardEventHandler,
Expand Down Expand Up @@ -349,7 +350,7 @@ const Tile: React.FC<ITileProps> = ({
})}
onClick={hasPermissions && onToggleClick ? onToggleClick : onPreviewClick}
onKeyDown={onKeyDown}
inputRef={ref}
ref={ref}
onFocus={onFocus}
tabIndex={isActive ? 0 : -1}
>
Expand Down Expand Up @@ -664,7 +665,7 @@ const ManageButtons: React.FC<IManageButtonsProps> = ({ hierarchy, selected, set
const disabled = !selectedRelations.length || removing || saving;

let Button: React.ComponentType<React.ComponentProps<typeof AccessibleButton>> = AccessibleButton;
let props = {};
let props: Partial<ComponentProps<typeof AccessibleTooltipButton>> = {};
if (!selectedRelations.length) {
Button = AccessibleTooltipButton;
props = {
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/SpaceRoomView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ const SpaceLandingAddButton: React.FC<{ space: Room }> = ({ space }) => {
<>
<ContextMenuButton
kind="primary"
inputRef={handle}
ref={handle}
onClick={openMenu}
isExpanded={menuDisplayed}
label={_t("action|add")}
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/ThreadPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export const ThreadPanelHeader: React.FC<{
<>
<ContextMenuButton
className="mx_ThreadPanel_dropdown"
inputRef={button}
ref={button}
isExpanded={menuDisplayed}
onClick={(ev: ButtonEvent) => {
openMenu();
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/UserMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ export default class UserMenu extends React.Component<IProps, IState> {
<ContextMenuButton
className="mx_UserMenu_contextMenuButton"
onClick={this.onOpenMenuClick}
inputRef={this.buttonRef}
ref={this.buttonRef}
label={_t("a11y|user_menu")}
isExpanded={!!this.state.contextMenuPosition}
onContextMenu={this.onContextMenu}
Expand Down
5 changes: 4 additions & 1 deletion src/components/views/audio_messages/PlayPauseButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import AccessibleTooltipButton from "../elements/AccessibleTooltipButton";
import { _t } from "../../../languageHandler";
import { Playback, PlaybackState } from "../../../audio/Playback";

type Props = Omit<ComponentProps<typeof AccessibleTooltipButton>, "title" | "onClick" | "disabled" | "element"> & {
type Props = Omit<
ComponentProps<typeof AccessibleTooltipButton>,
"title" | "onClick" | "disabled" | "element" | "ref"
> & {
// Playback instance to manipulate. Cannot change during the component lifecycle.
playback: Playback;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ export class FallbackAuthEntry extends React.Component<IAuthEntryProps> {
}
return (
<div>
<AccessibleButton kind="link" inputRef={this.fallbackButton} onClick={this.onShowFallbackClick}>
<AccessibleButton kind="link" ref={this.fallbackButton} onClick={this.onShowFallbackClick}>
{_t("auth|uia|fallback_button")}
</AccessibleButton>
{errorSection}
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/context_menus/KebabContextMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const KebabContextMenu: React.FC<KebabContextMenuProps> = ({ options, tit

return (
<>
<ContextMenuButton {...props} onClick={openMenu} title={title} isExpanded={menuDisplayed} inputRef={button}>
<ContextMenuButton {...props} onClick={openMenu} title={title} isExpanded={menuDisplayed} ref={button}>
<ContextMenuIcon className="mx_KebabContextMenu_icon" />
</ContextMenuButton>
{menuDisplayed && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ const ThreadListContextMenu: React.FC<ThreadListContextMenuProps> = ({
onClick={openMenu}
title={_t("right_panel|thread_list|context_menu_label")}
isExpanded={menuDisplayed}
inputRef={button}
ref={button}
data-testid="threadlist-dropdown-button"
/>
{menuDisplayed && (
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/dialogs/spotlight/Option.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const Option: React.FC<OptionProps> = ({ inputRef, children, endAdornment
{...props}
className={classNames(className, "mx_SpotlightDialog_option")}
onFocus={onFocus}
inputRef={ref}
ref={ref}
tabIndex={-1}
aria-selected={isActive}
role="option"
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/dialogs/spotlight/TooltipOption.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const TooltipOption: React.FC<TooltipOptionProps> = ({ inputRef, classNam
{...props}
className={classNames(className, "mx_SpotlightDialog_option")}
onFocus={onFocus}
inputRef={ref}
ref={ref}
tabIndex={-1}
aria-selected={isActive}
role="option"
Expand Down
43 changes: 24 additions & 19 deletions src/components/views/elements/AccessibleButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
limitations under the License.
*/

import React, { HTMLAttributes, InputHTMLAttributes } from "react";
import React, { forwardRef, FunctionComponent, HTMLAttributes, InputHTMLAttributes, Ref } from "react";
import classnames from "classnames";

import { getKeyBindingsManager } from "../../../KeyBindingsManager";
Expand Down Expand Up @@ -66,7 +66,6 @@ type DynamicElementProps<T extends keyof JSX.IntrinsicElements> = Partial<
* Extends props accepted by the underlying element specified using the `element` prop.
*/
type Props<T extends keyof JSX.IntrinsicElements> = DynamicHtmlElementProps<T> & {
inputRef?: React.Ref<Element>;
/**
* The base element type. "div" by default.
*/
Expand Down Expand Up @@ -101,22 +100,26 @@ interface RenderedElementProps extends React.InputHTMLAttributes<Element> {
* as a button. Identifies the element as a button, setting proper tab
* indexing and keyboard activation behavior.
*
* If a ref is passed, it will be forwarded to the rendered element as specified using the `element` prop.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If a ref is passed, it will be forwarded to the rendered element as specified using the `element` prop.
* If a ref is provided, it will be forwarded to the rendered element (as specified using the `element` prop).
* If `element` is a DOM element, the ref will be updated to point to the rendered DOM node.

Copy link
Member Author

Choose a reason for hiding this comment

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

If element is a DOM element, the ref will be updated to point to the rendered DOM node.

element can only be a string though, not a DOM element. It can be a name of a DOM element but it also can't be anything else (other than undefined where the value is div) so not sure what this 2nd line is trying to say.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that element can either be an intrinsic DOM element (div, button, or whatever), or a React component (MyFancyButton). (See https://www.typescriptlang.org/docs/handbook/jsx.html#type-checking, etc)

  • If it's a react component, we can't really say much about what happens to the ref other than that it is forwarded to that component for it to do with as it chooses. (It might set it to 42, for all we know.)
  • However, if it's an intrinsic DOM element, then we can be much more helpful: the act of "forwarding" it means that ref.content will be updated to point to the rendered DOM node corresponding to that element. This is what the user of AccessibleButton needs to know, and is the reason that I wanted to add that second sentence.

Now: if you're saying that element can't in fact be a React component and can only ever be an intrinsic DOM element, then we can clarify this further (since "forwarding" is entirely an implementation detail of AccessibleButton). Maybe something like:

Suggested change
* If a ref is passed, it will be forwarded to the rendered element as specified using the `element` prop.
* If a ref is provided, it will be updated to point to the rendered DOM node (which will normally be a `div`,
* unless overridden via the `element` property).

Copy link
Member Author

@t3chguy t3chguy Dec 21, 2023

Choose a reason for hiding this comment

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

image

The T type generic can only be one of these keys:

image
(continued...)

So you cannot use a React component here

Copy link
Member

Choose a reason for hiding this comment

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

yup I misread the typescript. (#include "I shouldn't have to reverse-engineer typescript types.rant")

*
* @param {Object} props react element properties
* @returns {Object} rendered react
*/
export default function AccessibleButton<T extends keyof JSX.IntrinsicElements>({
element = "div" as T,
onClick,
children,
kind,
disabled,
inputRef,
className,
onKeyDown,
onKeyUp,
triggerOnMouseDown,
...restProps
}: Props<T>): JSX.Element {
const AccessibleButton = forwardRef(function <T extends keyof JSX.IntrinsicElements>(
{
element = "div" as T,
onClick,
children,
kind,
disabled,
className,
onKeyDown,
onKeyUp,
triggerOnMouseDown,
...restProps
}: Props<T>,
ref: Ref<HTMLElement>,
): JSX.Element {
const newProps: RenderedElementProps = restProps;
if (disabled) {
newProps["aria-disabled"] = true;
Expand Down Expand Up @@ -170,7 +173,7 @@ export default function AccessibleButton<T extends keyof JSX.IntrinsicElements>(
}

// Pass through the ref - used for keyboard shortcut access to some buttons
newProps.ref = inputRef;
newProps.ref = ref;

newProps.className = classnames("mx_AccessibleButton", className, {
mx_AccessibleButton_hasKind: kind,
Expand All @@ -180,11 +183,13 @@ export default function AccessibleButton<T extends keyof JSX.IntrinsicElements>(

// React.createElement expects InputHTMLAttributes
return React.createElement(element, newProps, children);
}
});

AccessibleButton.defaultProps = {
// Type assertion required due to forwardRef type workaround in react.d.ts
(AccessibleButton as FunctionComponent).defaultProps = {
role: "button",
tabIndex: 0,
};
(AccessibleButton as FunctionComponent).displayName = "AccessibleButton";

AccessibleButton.displayName = "AccessibleButton";
export default AccessibleButton;
21 changes: 8 additions & 13 deletions src/components/views/elements/AccessibleTooltipButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { SyntheticEvent, FocusEvent, useEffect, useState } from "react";
import React, { SyntheticEvent, FocusEvent, forwardRef, useEffect, Ref, useState, ComponentProps } from "react";

import AccessibleButton from "./AccessibleButton";
import Tooltip, { Alignment } from "./Tooltip";
Expand All @@ -25,7 +25,7 @@ import Tooltip, { Alignment } from "./Tooltip";
*
* Extends that of {@link AccessibleButton}.
*/
type Props<T extends keyof JSX.IntrinsicElements> = React.ComponentProps<typeof AccessibleButton<T>> & {
type Props<T extends keyof JSX.IntrinsicElements> = ComponentProps<typeof AccessibleButton<T>> & {
/**
* Title to show in the tooltip and use as aria-label
*/
Expand Down Expand Up @@ -60,16 +60,10 @@ type Props<T extends keyof JSX.IntrinsicElements> = React.ComponentProps<typeof
onHideTooltip?(ev: SyntheticEvent): void;
};

function AccessibleTooltipButton<T extends keyof JSX.IntrinsicElements>({
title,
tooltip,
children,
forceHide,
alignment,
onHideTooltip,
tooltipClassName,
...props
}: Props<T>): JSX.Element {
const AccessibleTooltipButton = forwardRef(function <T extends keyof JSX.IntrinsicElements>(
{ title, tooltip, children, forceHide, alignment, onHideTooltip, tooltipClassName, ...props }: Props<T>,
ref: Ref<HTMLElement>,
) {
const [hover, setHover] = useState(false);

useEffect(() => {
Expand Down Expand Up @@ -108,12 +102,13 @@ function AccessibleTooltipButton<T extends keyof JSX.IntrinsicElements>({
onFocus={onFocus}
onBlur={hideTooltip}
aria-label={title || props["aria-label"]}
ref={ref}
>
{children}
{props.label}
{(tooltip || title) && tip}
</AccessibleButton>
);
}
});

export default AccessibleTooltipButton;
2 changes: 1 addition & 1 deletion src/components/views/elements/AppTile.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ export default class AppTile extends React.Component<IProps, IState> {
className="mx_AppTileMenuBar_widgets_button"
label={_t("common|options")}
isExpanded={this.state.menuDisplayed}
inputRef={this.contextMenuButton}
ref={this.contextMenuButton}
onClick={this.onContextMenuClick}
>
<MenuIcon className="mx_Icon mx_Icon_12" />
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/elements/Dropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ export default class Dropdown extends React.Component<DropdownProps, IState> {
aria-haspopup="listbox"
aria-expanded={this.state.expanded}
disabled={this.props.disabled}
inputRef={this.buttonRef}
ref={this.buttonRef}
aria-label={this.props.label}
aria-describedby={`${this.props.id}_value`}
aria-owns={`${this.props.id}_input`}
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/elements/ImageView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ export default class ImageView extends React.Component<IProps, IState> {
className="mx_ImageView_button mx_ImageView_button_more"
title={_t("common|options")}
onClick={this.onOpenContextMenu}
inputRef={this.contextMenuButton}
ref={this.contextMenuButton}
isExpanded={this.state.contextMenuDisplayed}
/>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/elements/PollCreateDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export default class PollCreateDialog extends ScrollableBaseModal<IProps, IState
disabled={this.state.busy || this.state.options.length >= MAX_OPTIONS}
kind="secondary"
className="mx_PollCreateDialog_addOption"
inputRef={this.addOptionRef}
ref={this.addOptionRef}
>
{_t("poll|options_add_button")}
</AccessibleButton>
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/messages/CallEvent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
import defaultDispatcher from "../../../dispatcher/dispatcher";
import type { ViewRoomPayload } from "../../../dispatcher/payloads/ViewRoomPayload";
import { Action } from "../../../dispatcher/actions";
import { AccessibleButtonKind, ButtonEvent } from "../elements/AccessibleButton";
import type { AccessibleButtonKind, ButtonEvent } from "../elements/AccessibleButton";
import MemberAvatar from "../avatars/MemberAvatar";
import { LiveContentSummary, LiveContentType } from "../rooms/LiveContentSummary";
import FacePile from "../elements/FacePile";
Expand Down
Loading