Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion packages/@react-aria/dialog/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"react": "^16.8.0"
},
"dependencies": {
"@babel/runtime": "^7.6.2"
"@babel/runtime": "^7.6.2",
"@react-aria/utils": "^3.0.0-rc.1"
},
"publishConfig": {
"access": "public"
Expand Down
10 changes: 9 additions & 1 deletion packages/@react-aria/dialog/src/useDialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
*/

import {AllHTMLAttributes, RefObject, useEffect} from 'react';
import {useSlotId} from '@react-aria/utils';

export interface DialogProps {
ref: RefObject<HTMLElement | null>,
Expand All @@ -19,10 +20,13 @@ export interface DialogProps {

interface DialogAria {
dialogProps: AllHTMLAttributes<HTMLElement>
titleProps: AllHTMLAttributes<HTMLElement>
}

export function useDialog(props: DialogProps): DialogAria {
let {ref, role = 'dialog'} = props;
let titleId = useSlotId();
titleId = props['aria-label'] ? undefined : titleId;

// Focus the dialog itself on mount, unless a child element is already focused.
useEffect(() => {
Expand All @@ -34,7 +38,11 @@ export function useDialog(props: DialogProps): DialogAria {
return {
dialogProps: {
role,
tabIndex: -1
tabIndex: -1,
'aria-labelledby': props['aria-labelledby'] || titleId
},
titleProps: {
id: titleId
}
};
}
14 changes: 13 additions & 1 deletion packages/@react-aria/utils/src/useId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* governing permissions and limitations under the License.
*/

import {useMemo, useState} from 'react';
import {useLayoutEffect, useMemo, useState} from 'react';

let map: Map<string, (v: string) => void> = new Map();

Expand Down Expand Up @@ -44,3 +44,15 @@ export function mergeIds(a: string, b: string): string {

return b;
}

export function useSlotId(): string {
let [id, setId] = useState(useId());
useLayoutEffect(() => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this is one of those times where this is the more appropriate of the two. We can change the id back to null if it isn't in the dom before the repaint. I don't know how it affects voice over though, this may still be an unnecessary optimization.

let setCurr = map.get(id);
if (setCurr && !document.getElementById(id)) {
setId(null);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So if the id does not find a home in the DOM, we set it back to null?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes

}
}, [id]);

return id;
}
4 changes: 2 additions & 2 deletions packages/@react-spectrum/dialog/src/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,13 @@ let sizeMap = {
function BaseDialog({children, slots, size, role, ...otherProps}: SpectrumBaseDialogProps) {
let ref = useRef();
let sizeVariant = sizeMap[size];
let {dialogProps} = useDialog({ref, role});
let {dialogProps, titleProps} = useDialog({ref, role, ...otherProps});
if (!slots) {
slots = {
container: {UNSAFE_className: styles['spectrum-Dialog-grid']},
hero: {UNSAFE_className: styles['spectrum-Dialog-hero']},
header: {UNSAFE_className: styles['spectrum-Dialog-header']},
heading: {UNSAFE_className: styles['spectrum-Dialog-heading']},
heading: {UNSAFE_className: styles['spectrum-Dialog-heading'], ...titleProps},
typeIcon: {UNSAFE_className: styles['spectrum-Dialog-typeIcon']},
divider: {UNSAFE_className: styles['spectrum-Dialog-divider'], size: 'M'},
content: {UNSAFE_className: styles['spectrum-Dialog-content']},
Expand Down
1 change: 0 additions & 1 deletion packages/@react-spectrum/dialog/stories/Dialog.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,6 @@ function render({width = 'auto', isDismissable = undefined, ...props}) {
);
}


function renderHero({width = 'auto', isDismissable = undefined, ...props}) {
return (
<div style={{display: 'flex', width, margin: '100px 0'}}>
Expand Down
61 changes: 61 additions & 0 deletions packages/@react-spectrum/dialog/test/Dialog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import {cleanup, fireEvent, render} from '@testing-library/react';
import {Dialog} from '../';
import {DialogContext} from '../src/context';
import {Header} from '@react-spectrum/view';
import {Heading} from '@react-spectrum/typography';
import {ModalProvider} from '@react-aria/dialog';
import React from 'react';

Expand All @@ -29,6 +31,8 @@ describe('Dialog', function () {

let dialog = getByRole('dialog');
expect(document.activeElement).toBe(dialog);
// if there is no heading, then we shouldn't auto label
expect(dialog).not.toHaveAttribute('aria-labelledby');
});

it('auto focuses the dialog itself if there is no focusable child', function () {
Expand Down Expand Up @@ -92,4 +96,61 @@ describe('Dialog', function () {
let dialog = getByRole('dialog');
expect(dialog).toHaveAttribute('aria-modal', 'true');
});

it('should be labelled by its header', function () {
let {getByRole} = render(
<ModalProvider>
<DialogContext.Provider value={{type: 'modal'}}>
<Dialog>
<Heading><Header>The Title</Header></Heading>
</Dialog>
</DialogContext.Provider>
</ModalProvider>
);

let dialog = getByRole('dialog');
let heading = getByRole('heading');

let id = heading.id;
expect(dialog).toHaveAttribute('aria-labelledby', id);
});

it('if aria-labelledby is specified, then that takes precedence over the title', function () {
let {getByRole} = render(
<div>
<ModalProvider>
<DialogContext.Provider value={{type: 'modal'}}>
<Dialog aria-labelledby="batman">
<Heading><Header>The Title</Header></Heading>
</Dialog>
</DialogContext.Provider>
</ModalProvider>
<span id="batman">Good grammar is essential, Robin.</span>
</div>
);

let dialog = getByRole('dialog');
let heading = getByRole('heading');

let id = heading.id;
expect(dialog).not.toHaveAttribute('aria-labelledby', id);
expect(dialog).toHaveAttribute('aria-labelledby', 'batman');
});

it('if aria-label is specified, then that takes precedence over the title', function () {
let {getByRole} = render(
<ModalProvider>
<DialogContext.Provider value={{type: 'modal'}}>
<Dialog aria-label="robin">
<Heading><Header>The Title</Header></Heading>
</Dialog>
</DialogContext.Provider>
</ModalProvider>
);

let dialog = getByRole('dialog');

expect(dialog).not.toHaveAttribute('aria-labelledby');
expect(dialog).toHaveAttribute('aria-label', 'robin');
});
});