Skip to content

fix: edit dialog accessiblity validation logic available in shadow dom #3507

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nayounsang
Copy link
Contributor

@nayounsang nayounsang commented Apr 28, 2025

Description

close #3484

  • Fixed accessibility validation logic for Dialog Title and Description.
    • Passing ContentRef props <TitleWarning titleId={context.titleId} contentRef={contentRef} />
    • Validate in the Content contentRef.current?.querySelector(#${titleId});
  • Strictly check if it is inside Content as the error message says
  • Compatible with Shadow DOM (The document-based logic of the existing logic does not work when used in shadow DOM)
  • (Performance improvement is also expected as it checks a narrow range inside the content. But I didn't think it was important so I didn't bother measuring it haha)

This is test code. All passed. But I will include it if you give me feedback that it needs to be committed.

    describe('when in Shadow DOM', () => {
      let shadowRoot: ShadowRoot;
      let shadowContainer: HTMLElement;

      beforeEach(() => {
        // make shadow root
        const host = document.createElement('div');
        document.body.appendChild(host);
        shadowRoot = host.attachShadow({ mode: 'open' });

        // render dialog in the shadow root
        shadowContainer = document.createElement('div');
        shadowRoot.appendChild(shadowContainer);

        cleanup();
        rendered = render(
          <Dialog.Root defaultOpen>
            <Dialog.Trigger>{OPEN_TEXT}</Dialog.Trigger>
            <Dialog.Overlay />
            <Dialog.Content>
              <Dialog.Title>{TITLE_TEXT}</Dialog.Title>
              <Dialog.Description>Description</Dialog.Description>
              <Dialog.Close>{CLOSE_TEXT}</Dialog.Close>
            </Dialog.Content>
          </Dialog.Root>,
          { container: shadowContainer }
        );
      });

      afterEach(() => {
        document.body.innerHTML = '';
      });

      it('should not show any errors when title is present in Shadow DOM', () => {
        expect(consoleErrorMockFunction).not.toHaveBeenCalled();
      });

      it('should not show any warnings when description is present in Shadow DOM', () => {
        consoleWarnMockFunction.mockClear();
        expect(consoleWarnMockFunction).not.toHaveBeenCalled();
      });
    });
 ✓ packages/react/dialog/src/dialog.test.tsx (10 tests) 272ms
   ✓ given a default Dialog > should have no accessibility violations in default state 76ms
   ✓ given a default Dialog > after clicking the trigger > when no description has been provided > should warn to the console 22ms
   ✓ given a default Dialog > after clicking the trigger > when no title has been provided > should display an error in the console 60ms
   ✓ given a default Dialog > after clicking the trigger > when aria-describedby is set to undefined > should not warn to the console 32ms
   ✓ given a default Dialog > after clicking the trigger > should open the content 9ms
   ✓ given a default Dialog > after clicking the trigger > should have no accessibility violations 26ms
   ✓ given a default Dialog > after clicking the trigger > should focus the close button 8ms
   ✓ given a default Dialog > after clicking the trigger > when pressing escape > should close the content 11ms
   ✓ given a default Dialog > after clicking the trigger > when in Shadow DOM > should not show any errors when title is present in Shadow DOM 15ms
   ✓ given a default Dialog > after clicking the trigger > when in Shadow DOM > should not show any warnings when description is present in Shadow DOM 13ms

 Test Files  1 passed (1)
      Tests  10 passed (10)
   Start at  17:19:52
   Duration  984ms

Copy link

changeset-bot bot commented Apr 28, 2025

🦋 Changeset detected

Latest commit: ff57bf2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@radix-ui/react-dialog Patch
@radix-ui/react-alert-dialog Patch
radix-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -516,10 +519,10 @@ For more information, see https://radix-ui.com/primitives/docs/components/${titl

React.useEffect(() => {
if (titleId) {
const hasTitle = document.getElementById(titleId);
const hasTitle = contentRef.current?.querySelector(`#${titleId}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

titleId needs to be sanitized in case it contains : - which is the case for react <19

Suggested change
const hasTitle = contentRef.current?.querySelector(`#${titleId}`);
const hasTitle = contentRef.current?.querySelector(`#${titleId.replaceAll(':', '\\:')}`);

could also use CSS.escape instead of manual string replace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @radnan thx for review.
I found this PR was posted before, so I plan to close this PR and post your review on the previous PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chaance This bug is in high demand for a fix. What can I do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@radix-ui/react-dialog: Accessibility error inside shadow dom
2 participants