Skip to content

fix: Properly clean up inert if element is hidden via sub dialog combobox #8739

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.

Sign up for GitHub

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

Merged
devongovett merged 2 commits into from
Aug 20, 2025
Merged

fix: Properly clean up inert if element is hidden via sub dialog combobox #8739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c80a5d7
fix: Properly clean up inert if element is hidden via sub dialog comb…
LFDanLu Aug 19, 2025
712fa08
get rid of tests in favor of story
LFDanLu Aug 19, 2025
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
5 changes: 5 additions & 0 deletions packages/@react-aria/overlays/src/ariaHideOutside.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ export function ariaHideOutside(targets: Element[], options?: AriaHideOutsideOpt
element.setAttribute('aria-hidden', 'true');
} else {
element.removeAttribute('aria-hidden');
if (element instanceof windowObj.HTMLElement) {
// We only ever call setHidden with hidden = false when the nodeCount is 1 aka
// we are trying to make the element visible to screen readers again, so remove inert as well
element.inert = false;
}
}
};

Expand Down
85 changes: 84 additions & 1 deletion packages/react-aria-components/stories/Modal.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@
* governing permissions and limitations under the License.
*/

import {Button, Dialog, DialogTrigger, Heading, Modal, ModalOverlay} from 'react-aria-components';
import {Button, ComboBox, Dialog, DialogTrigger, Heading, Input, Label, ListBox, Modal, ModalOverlay, Popover, TextField} from 'react-aria-components';
import {Meta, StoryFn} from '@storybook/react';
import React, {useEffect} from 'react';
import './styles.css';
import {MyListBoxItem} from './utils';
import styles from '../example/index.css';


export default {
title: 'React Aria Components/Modal',
Expand Down Expand Up @@ -188,3 +191,83 @@ export const ModalInteractionOutsideDefaultOverlayExample: ModalStory = () => {
</DialogTrigger>
);
};

function InertTest() {
Copy link
Member

@snowystinger snowystinger Aug 19, 2025

Choose a reason for hiding this comment

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

Is this a hard one to write a unit test for?
I saw you removed the tests in favour of a story?

Copy link
Member Author

@LFDanLu LFDanLu Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, the mock I added proved to cause all sorts of problems in other many other tests (breaking things like column resizing, drag and drop, etc). I figured I'd get a test story out first rather than continue flailing with the mock, but I'd much rather have a test than a story tbh.

The mock seems to need to be specified at the global test setup level too in order to make ariaHideOutside's 'inert' in HTMLElement.prototype; check pass too :/

Copy link
Member Author

@LFDanLu LFDanLu Aug 19, 2025

Choose a reason for hiding this comment

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

Did some digging to figure out what what causing the rest of the test failures, turns out it is basically due to the test now taking the inert path but not actually being able to set the inert attribute on the elements, resulting in the various getByRole/etc calls in the test yielding more elements than expected since they aren't being aria-hidden anymore. I could work around this by adding a && process.env.NODE_ENV !== 'test' check in the inert path of ariaHideOutside and make sure to set the NODE_ENV to something different in the specific ariaHideOutside test that needs to check the inert path but that feels gross tbh

return (
<DialogTrigger>
<Button>Open modal</Button>
<ModalOverlay
isDismissable
style={{
position: 'fixed',
zIndex: 100,
top: 0,
left: 0,
bottom: 0,
right: 0,
background: 'rgba(0, 0, 0, 0.5)',
display: 'flex',
alignItems: 'center',
justifyContent: 'center'
}}>
<Modal
style={{
background: 'Canvas',
color: 'CanvasText',
border: '1px solid gray',
padding: 30
}}>
<Dialog>
{() => (
<>
<TextField>
<Label>First name</Label>
<Input />
</TextField>
<DialogTrigger>
<Button>Combobox Trigger</Button>
<Popover placement="bottom start">
<Dialog>
{() => (
<ComboBox
menuTrigger="focus"
autoFocus
name="combo-box-example"
data-testid="combo-box-example"
allowsEmptyCollection>
<Label style={{display: 'block'}}>Test</Label>
<div style={{display: 'flex'}}>
<Input />
<Button>
<span aria-hidden="true" style={{padding: '0 2px'}}>▼</span>
</Button>
</div>
<ListBox
Copy link
Member Author

@LFDanLu LFDanLu Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note that this ListBox is NOT wrapped in a Popover. This setup is quite intentional since wrapping it in a Popover means it won't dismiss when you click outside into the root level Dialog (which is odd but is probably because everything outside the subdialog is made inert probably due to #5338).

By having the combobox and its parent dialog close when clicking into the root dialog, ariaHideOutside's cleanup will run multiple times (once per popover) with the combobox's ariaHideOutside being the last cleanup. Previously this wouldn't cause the inert set by its parent dialog's ariaHideOutside to get cleared because the combobox's own call doesn't have shouldUseInert but now we always clean up aria-hidden and inert

className={styles.menu}>
<MyListBoxItem>Foo</MyListBoxItem>
<MyListBoxItem>Bar</MyListBoxItem>
<MyListBoxItem>Baz</MyListBoxItem>
<MyListBoxItem href="http://google.com">Google</MyListBoxItem>
</ListBox>
</ComboBox>
)}
</Dialog>
</Popover>
</DialogTrigger>
</>
)}
</Dialog>
</Modal>
</ModalOverlay>
</DialogTrigger>
);
}

export const InertTestStory = {
render: () => <InertTest />,
parameters: {
description: {
data: 'You should be able to click "Combobox Trigger" and then click on the textfield, closing the subdialog. A second click should move focus into the textfield'
}
}
};