SelectPanel2: Only render children when open

[siddharthkp]

By default a <dialog> renders as open="false", but it does exist in the dom. This can be unintuitive in the react paradigm where you expect dialog to not be rendered in the dom until it's visible.

Places where this shows up:

1. Data fetching: Data that is meant to be fetched only when the Dialog is open might be fetched early
2. Testing: When there are multiple dialogs on the page, unless you are more specific, getting element by role dialog returns the first dialog, not the open dialog. screen.getByRole('dialog')
3. Imperative code: When there are multiple dialogs on the page, querySelector for dialog would return the first dialog, not necessarily the open dialog.

Before:

<!-- closed state: -->
<button type="button" aria-haspopup="true" aria-expanded="false"></button>
<dialog></dialog>

<!-- open state -->
<button type="button" aria-haspopup="true" aria-expanded="true"></button>
<dialog open></dialog>

After:

<!-- closed state: -->
<button type="button" aria-haspopup="true" aria-expanded="false"></button>
- <dialog></dialog>

<!-- open state -->
<button type="button" aria-haspopup="true" aria-expanded="true"></button>
<dialog open></dialog>

[changeset-bot]

🦋 Changeset detected

Latest commit: 828df34

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	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

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	113.71 KB (0%)
packages/react/dist/browser.umd.js	114.38 KB (0%)

[keithamus]

We should avoid doing this because it can cause issues for AT users. If we need to set up a button that points to a dialog, it cannot if the dialog is not on the page. For example, your examples are missing aria-details which sets up said relationship:

<button type="button" aria-haspopup="true" aria-expanded="true" aria-details="my-dialog"></button>
<dialog id="my-dialog" open></dialog>

To address some of your points:

Data fetching: Data that is meant to be fetched only when the Dialog is open might be fetched early

Code should avoid doing this, but we could render the dialog and skip the children rendering, if we deem it necessary. The problem, however, is that complex dialogs will experience slow INP if we end up rendering a lot of DOM as the dialog is rendered.

Testing: When there are multiple dialogs on the page, unless you are more specific, getting element by role dialog returns the first dialog, not the open dialog. screen.getByRole('dialog')

This seems true for all elements, no? I would not expect div to return anything but the first div, why would dialog be any different?

Imperative code: When there are multiple dialogs on the page, querySelector for dialog would return the first dialog, not necessarily the open dialog.

Is there an instance where there is a negative effect here? Same applies, as above I believe.

---

If there are concrete examples of where this is causing failure points we should investigate those.

I think for this PR to land I'd expect to see the <dialog> always be rendered, and other mitigations put in place to directly address the concrete failure modes.

[siddharthkp]

If we need to set up a button that points to a dialog, it cannot if the dialog is not on the page.
For example, your examples are missing aria-details which sets up said relationship

Interesting! This did not come up in the a11y spec, let me confirm with them once more.

but we could render the dialog and skip the children rendering, if we deem it necessary.

I think that's a fair solution, let me try that and see if it has any unintended side effects.

If there are concrete examples of where this is causing failure points we should investigate those.

There are concrete examples of this in dotcom 😢 I've lost multiple days at this point debugging jest tests in dotcom because of these. Folks I talked to were genuinely surprised by behavior. Honestly, it comes back to this:

This (having an element that is not yet "open" or visible but calls its effects) can be unintuitive in the react paradigm where you expect dialog to not be rendered in the dom until it's visible.

[siddharthkp]

Updated!

Rendering the dialog but not its contents now. It solves the main user-land problem (side effects & data fetching), but not the one with selectors in testing which I think is okay for now.

[keithamus]

Great, thanks for updating this!