AutoComplete when nested in a dialog can run into z-index/dom ordering issues

[mattcosta7]

Description

I have a dialog that renders an AutoComplete.

When initially opening, it appears

A minimal repro: https://stackblitz.com/edit/react-ts-twnyff?file=App.tsx

A hacky fix by rendering after a use effect - https://stackblitz.com/edit/react-ts-t8qx1j?file=App.tsx

Steps to reproduce

see repros.

import * as React from 'react';
import './style.css';
import { Dialog } from '@primer/react/drafts';
import { FormControl, Autocomplete, ThemeProvider } from '@primer/react';

export default function App() {
  const [dialogOpen, setDialogOpen] = React.useState(false);
  const [autocompleteTemplateList] = React.useState([]);
  const [selectedAutocompleteItemIds] = React.useState([]);

  React.useEffect(() => {
    setDialogOpen(true);
  }, []);

  if (!dialogOpen) return null;
  return (
    <ThemeProvider>
      <Dialog>
        <FormControl>
          <FormControl.Label visuallyHidden>Search templates</FormControl.Label>
          <Autocomplete>
            <Autocomplete.Input />
            <Autocomplete.Overlay>
              <Autocomplete.Menu
                items={autocompleteTemplateList}
                selectedItemIds={selectedAutocompleteItemIds}
              />
            </Autocomplete.Overlay>
          </Autocomplete>
        </FormControl>
      </Dialog>
    </ThemeProvider>
  );
}

with this component open, focus the input, and note that it's _behind_ the dialog/modal

Add an AutoComplete to a dialog's body

cc @iansan5653 @stephanieg0

Version

v35.20.0

Browser

No response

[mattcosta7]

import * as React from 'react';
import './style.css';
import { Dialog } from '@primer/react/drafts';
import { FormControl, Autocomplete, ThemeProvider } from '@primer/react';

function useIsPrimerHackRenderApplied(rendered: boolean) {
  /**
   * This is a hack to work around aprimer bug, that will mount the AutoComplete overlay
   * before the dialog's overlay, making it impossible to interact with
   */
  const [isDialogRenderedAlready, setIsDialogRenderedAlready] =
    React.useState(false);
  React.useEffect(() => {
    if (rendered) {
      setIsDialogRenderedAlready(true);
    } else {
      setIsDialogRenderedAlready(false);
    }
    // after the first render, then show that AutoComplete
  }, [rendered]);

  return isDialogRenderedAlready;
}

export default function App() {
  const [dialogOpen, setDialogOpen] = React.useState(false);
  const [autocompleteTemplateList] = React.useState([]);
  const [selectedAutocompleteItemIds] = React.useState([]);

  React.useEffect(() => {
    setDialogOpen(true);
  }, []);

  const hackApplied = useIsPrimerHackRenderApplied(dialogOpen);
  if (!dialogOpen) return null;
  return (
    <ThemeProvider>
      <Dialog>
        {hackApplied ? (
          <FormControl>
            <FormControl.Label visuallyHidden>
              Search templates
            </FormControl.Label>
            <Autocomplete>
              <Autocomplete.Input />
              <Autocomplete.Overlay>
                <Autocomplete.Menu
                  items={autocompleteTemplateList}
                  selectedItemIds={selectedAutocompleteItemIds}
                />
              </Autocomplete.Overlay>
            </Autocomplete>
          </FormControl>
        ) : null}
      </Dialog>
    </ThemeProvider>
  );
}

We can hack around this by defering the autocomplete from rendering until after the dialog - kinda - but this should probably be supported directly

[iansan5653]

It's an interesting bug that starts touching on React's admonitions to avoid side-effects when rendering. Turns out rendering into a portal is a side effect, and the result of that is that rendering order matters. But children always render before parents - they naturally have to, by nature of being returned by the parent.

Normally this doesn't matter because it's very rare to render an Overlay with an already-open Overlay in its children -- you never open two nested overlays at one time. And as long as they don't render in the same cycle it's totally fine - if the child overlay renders later due to user interaction, that works fine.

One possible answer here is to not render Overlay contents until they are visible. . Currently we always render the overlay and only hide it with CSS. Maybe we should just return null from Overlay when visibility == "hidden"? This would ensure that the overlay is placed at the end of the portal root's DOM at the moment they become visible, rather than at the moment the Overlay is first rendered This actually seems more performant (less unnecessary DOM on the page) but maybe there is a good reason it was done this way in the first place?

[keithamus]

The solution here is to use popover https://developer.chrome.com/docs/web-platform/popover-api/ which will resolve z-index issues.

[iansan5653]

Here's another simpler repro, using just Overlay.

popover does look like the solution we want, but unfortunately it doesn't look like the browser support will be there for a while.

[mattcosta7]

popover does look like the solution we want, but unfortunately it doesn't look like the browser support will be there for a while.

What's the latest on the polyfill @keithamus is that ready enough to be used internally in primer for these things? Would be a really nice solution

[keithamus]

We just rolled out the polyfill in Primer::Alpha::Overlay on PVC.

I believe the polyfill is ready to go, but it is not without its caveats. It still suffers from the same z-index issues (because :top-layer is not polyfillable). Although it does set an extremely high z-index of 2147483647 which may solve some minor issues.

For browsers which don't need the polyfill it will very much solve the issue. Right now the only people who have native popover are those in Chrome with either the popover Origin Trial, or experimental web features turned on. .com has the origin trial enabled for some users (add yourself to the popup_api feature flag if you want to try it). Chrome has built-in caps for origin trials to ensure only a maximum amount of traffic see the trial (IIRC it's 5%).

The Chrome team are aiming to ship in 113, which will be main in May IIRC. Both mozilla and safari are supportive of this API. Mozilla has started implementation and Webkit implementation is well underway. My general hunch is that it will be in major browsers in 2023.

So I would conclude in saying: it's the solution which solves this problem. It is not available native yet, but the polyfill gives us API compatibility so that as browsers add it - which seems very likely - the bugs will resolve. We should prepare ourselves toward this.

[lesliecdubs]

👋🏻 Thanks for bringing this up and for the discussion. It sounds like there are some workarounds so we're moving this to the backlog, but please give a shout if it starts to become more of an annoyance.

[mattcosta7]

👋🏻 Thanks for bringing this up and for the discussion. It sounds like there are some workarounds so we're moving this to the backlog, but please give a shout if it starts to become more of an annoyance.

@lesliecdubs since this ties in pretty directly with - #3003
and I don't think there are direct workarounds for that behavior - it is less annoyance and more impacting feature work.

[lesliecdubs]

Thanks @mattcosta7, noted.

@justinbyo can you take a look at the priority on this and let us know if you'd advise moving out of the backlog?

[liranelisha]

any updates on this issue? we just encountered it and it took up about a day's work of 2 of our developers to confirm it's coming from the Primar component and not something we did

[lesliecdubs]

Hi @liranelisha, I'm sorry for the trouble 😞

I'll bring this back up with the Primer leads to re-review the priority of this issue.

[keithamus]

The Chrome team are aiming to ship in 113, which will be main in May IIRC. Both mozilla and safari are supportive of this API. Mozilla has started implementation and Webkit implementation is well underway. My general hunch is that it will be in major browsers in 2023.

To update this comment Chrome 114 (latest stable) supports native popover. Safari 17 will support it (release likely September, already available in Safari TP). Firefox supports it flagged (dom.element.popover.enabled) and will likely enable it soon also. Chrome represents over 70% of GitHub traffic, so while the polyfill will still exhibit the bugs for users without popover, it will resolve the issue for the majority our user base, improving over time without cost.

[github-actions]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

[keithamus]

The Chrome team are aiming to ship in 113, which will be main in May IIRC. Both mozilla and safari are supportive of this API. Mozilla has started implementation and Webkit implementation is well underway. My general hunch is that it will be in major browsers in 2023.

To update this comment Chrome 114 (latest stable) supports native popover. Safari 17 will support it (release likely September, already available in Safari TP). Firefox supports it flagged (dom.element.popover.enabled) and will likely enable it soon also. Chrome represents over 70% of GitHub traffic, so while the polyfill will still exhibit the bugs for users without popover, it will resolve the issue for the majority our user base, improving over time without cost.

Another update here on this. popover shipped in Safari 17. Chrome has been stable since May. Firefox is due to ship at the end of January, and enabling the preview flag in Firefox shows it to be stable.

IMO any floating UI that exhibits z-index issues should be swiftly refactored to use popover.

[github-actions]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.