primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.12k stars 533 forks source link

Portal Hardcoded Z-Index Buries Components #4622

Open ntrappe-msft opened 4 months ago

ntrappe-msft commented 4 months ago

Description

Hi, I wanted to bring up a few problems that arise from mixing Primer and non-Primer components. The problems emerge because the Primer Portal sets a div whose children will inherit a z-index of 1. This works fine if you're only working with Primer components that abide by this fake DOM hierarchy. But, if you start mixing in non-Primer components with their own z-indices, those Primer components will become buried.

Example

Let's say we created a header component from <div> and added an ActionMenu.

return (
    <div id='header-wrapper' style='position: fixed; z-index: 50;'>
        <ActionMenu id='action-menu'>
            <ActionMenu.Button>Color Mode</ActionMenu.Button>
            <ActionMenu.Overlay>
                <ActionList id='action-menu-list'>
                    <ActionList.Item>Light Mode</ActionList.Item>
                    <ActionList.Item>Dark Mode</ActionList.Item>
                </ActionList>
            </ActionMenu.Overlay>
        <ActionMenu id='action-menu'>
    </div>
)

On the screen below, you'll see the header as a blue bar with the button Color Mode. When we click on the button, an overlay pops up with the options for Light Mode and Dark Mode. The top half of the overlay is covered because the z-index of the header is higher than it.

Shows the header partially obscuring the top of the overlay for the MenuList

If we query the div responsible for setting the z-index to 1 and set it instead to 100, you'll notice that the overlay is no longer covered but fully visible.

Shows the header no longer obscuring the overlay for MenuList because we have overwritten the z-index of the div from Portal

Problem

If you take a look at the DOM, you'll see the header component and a Portal component, with the ActionMenu attached to the Portal component. Because the header isn't a Primer component, it doesn't get nested in the Portal root so it doesn't agree to the same z-index hierarchy.

<div id="header-wrapper">
<div id="__primerPortalRoot__">
   <div style="position: relative; z-index: 1;">
     <div class="Overlay__StyledOverlay-sc">{etc}</div>
   </div>
</div>

The only workaround I've found it to query the root for my React DOM, descend down the DOM to the div responsible, then overwrite it. It's pretty hacky and not great if you've got a number of components that you need to adjust.

#root > div > div > div {
    z-index: 100 !important; 
}

Fixes

I'm open to hearing your thoughts and I'm curious about what you'd recommend as best practices when mixing Primer and non-Primer components or modifying z-indices. I've been debating on whether I should: (1) Opt out of using non-Primer components to avoid this (e.g., just use Box). (2) Put non-Primer components in the same React Portal. (3) Keep using the hacky way of overwriting the z-index.

Steps to reproduce

(More examples of how to reproduce included in the description above).

  1. Create a non-Primer component like a div. Set the z-index to more than 1.
  2. Add a primer component like Overlay or ActionMenu as a child.
  3. Check the DOM and see that all components of Portal have a z-index of 1.
  4. See that the component is partially covered by the non-Primer component.
  5. In Inspector, modify the Portal div to be a higher z-index and see that it's no longer covered.

Version

v36.19.0

Browser

Safari

github-actions[bot] commented 4 months ago

Uh oh! @ntrappe-msft, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

siddharthkp commented 4 months ago

Hi!

Thanks for taking the time to report this issue!

For now, I would recommend (3) Keep using the hacky way of overwriting the z-index or exploring an alternate workaround with a custom Portal root

I say that because long term, we are hoping to remove this batch of problems by using the popover API. We have a tracking issue open, but it's not prioritised yet: github/primer#2864 (private github repo)

vvk147 commented 2 months ago

@ntrappe-msft

As suggested by @siddharthkp there can be a workaround using a custom portal root. Below is the snippet demonstrating the same.

import { useEffect } from 'react';
import { registerPortalRoot } from '@primer/react';

const HeaderWrapper = () => {
  useEffect(() => {
    const portalRoot = document.getElementById("__primerPortalRoot__");
    if (portalRoot) {
      registerPortalRoot(portalRoot, "__primerPortalRoot__");
    }
  }, []);

  return (
    <>
      <div id="__primerPortalRoot__"></div>
      <div id="header-wrapper" style={{ position: 'fixed', zIndex: 50 }}>
        <ActionMenu id="action-menu">
          <ActionMenu.Button>Color Mode</ActionMenu.Button>
          <ActionMenu.Overlay portalContainerName="__primerPortalRoot__">
            <ActionList id="action-menu-list">
              <ActionList.Item>Light Mode</ActionList.Item>
              <ActionList.Item>Dark Mode</ActionList.Item>
            </ActionList>
          </ActionMenu.Overlay>
        </ActionMenu>
      </div>
    </>
  );
};

export default HeaderWrapper;

Explanation:

Add Portal Root Element: The <div id="__primerPortalRoot__"></div> is added at the top level of the component to ensure it is available in the DOM.