Fix #1652 - Listbox popover does not move when page scrolls

[charliepark]

Preamble: I know, per the contributing guidelines, you might not get to this PR. No worries! I hope it helps, though!

---

Currently, when a user opens up a Listbox component, the popover is frozen in place. Although the coordinates for the Listbox's positioning are created relative to where the target is on the page, the coordinates don't update as the user scrolls around on the page:
BEFORE:

Floating UI, the library used in console to create dropdowns, has an optional autoUpdate utility. (More on autoUpdate in their docs.)

This PR passes in the whileElementsMounted: autoUpdate, attribute, anchoring the Listboxes to the on-screen element that summoned them. Note how the dropdown now scrolls up and off the top of the screen:
AFTER:

Getting this to work correctly required two other changes I'll mention:

1. The TopBar component needed to explicitly have a z-index greater than the Listbox component, or else the floating popover would hover above the TopBar element. So this PR has TopBar getting z-50 and Listbox getting dropped from z-50 to z-40.
2. The test suite hadn't required a ResizeObserver previously, but with the additional attribute getting passed in, it required a polyfill. I added a dev dependency to package.json. This is the same dev dependency the Floating UI library uses (here).

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Sep 6, 2023 3:44pm

[david-crespo]

Thank you! I’ll check this out later today but it looks great.

[david-crespo]

Looks like the higher z-index on the topbar now puts it on top of the side modal. I think setting a higher z-index on SideModal should fix it. Compare to https://oxide-console-preview.vercel.app/projects/mock-project/vpcs-new

Unfortunately my images in #1652 are broken, or you probably would have seen that the original spot I noticed this was in a scrolling side modal. (My guess is something went wrong when we made the repo public.)

[charliepark]

Good catch!

After you pointed that out, I wanted to make sure others weren't slipping through the cracks, so clicked through a bunch of different scenarios, and realized that the z-indexes needed a little bit of tuning. I ended up with a decent understanding of the stacking priority, and figured it might make sense to more explicitly declare it. (I recognize you maybe already have something like this in Figma / internally somewhere.) I'll suggest it in a second, but first, some screenshots of the current state of things, now that I fixed the sidebar/topbar issue, and made a few other adjustments …

The SideModal, above the TopBar:

… and the env-dependent MswBanner:

The ListBox, above a tooltip:

The TopBar, above a ListBox:

The Menu, above the TopBar, above a ListBox:

The ListBox, above a Modal:

… and a slightly longer one …

With all of those "stacked", we end up with this setup:
Alerts (Toasts) need to be above the SideModal.¹
The SideModal needs to be above the TopBar.
The TopBar needs to be above the Listbox.
The Listbox needs to be above Modals.
Modals need to be above the regular content.

And, translated to the utility classes …
Toasts get z-50.
SideModal gets z-40.
TopBar gets z-30.
ListBox gets z-20.
Modals get z-10.
Regular content has no specified z-index, so, effectively, 0.

You'll note that Modals and SideModals need to have different layers from one another, and that ListBoxes come in between them. This means that any SideModals in the future that have ListBoxes in them will need to have some sort of special-casing on the ListBox component to give it a z-index of z-50. I looked, and was unable to find any SideModals that had ListBoxes in them. If you know of any, please let me know.

I believe that covers it, but if I missed any edge cases (or glaringly obvious cases!), let me know!

---

¹ Alerts, technically, don't have to be above the SideModal, as the ToastStack is currently in the bottom left corner, while SideModals are on the right. But should Toasts ever move to the right side, they should be above the SideModal. In general, it's probably safe to think of Alerts as the "top-most" layer of the stack.

[david-crespo]

Thanks for the analysis, that all seems right. There is at least one listbox in a side modal. If you stop the instance here and click add network interface you can see it. It would be nice if there was a way to avoid putting a special z-index on that listbox manually but we can do that if we have to.

https://console-git-fork-charliepark-1652-fix-froz-092e61-oxidecomputer.vercel.app/projects/mock-project/instances/db1/network-interfaces

[charliepark]

(Mentioned this at the end, but I'm about to go camping for the weekend, so no need for you to think about this until next week!)

---

Listboxes-in-sidebar-modals makes sense. And re-reading your message from earlier, you had explicitly mentioned a dropdown in a sidebar. Womp womp.

I tried rethinking the layers and the components, to see if there was some way around the stacking order that I specified earlier in the PR, but it seemed that — with each of the Listbox, TopBar, and SideModal needing to be on top in various scenarios — that we'd need two ways of handling listboxes, at different layers of the stack.

I looked into a few options, and have two that work. I'm open to other approaches if you think of any. I went ahead and implemented Option Two, below, but wanted to note the options and my thoughts.

The working Listbox-in-Sidebar:

Fundamentally, the issue we're having to work around is that the Floating UI library renders the menu options outside of the components in the DOM that the user is interacting with. It puts each menu inside its own portal. We need a way to convey the appropriate stackpoint to each of the menus' portals — z-20 to regular Listboxes; z-50 to sidebar Listboxes.

---

Option One: Native CSS

The idea: Use native CSS rules and the portal structure used by the Floating UI library; use increased specificity to target menus within sidebars, as distinct from more generic menus.

The key CSS (in menu-list.css):

/* general menus and Listboxes */
.ox-menu {
  z-index: 20;
}
/* menus and Listboxes contained in sidebars */
.ox-side-modal + [data-floating-ui-portal] .ox-menu {
  z-index: 50;
}

The explanation: When the sidebar is open there are actually several portals that Floating UI adds to the DOM. One has the class ox-side-modal. When the sidebar has Listboxes inside it, each of those Listbox components gets its own portal, which has a data attribute of data-floating-ui-portal. The portal has an ox-menu child within it, which contains the Listbox items.

The pros: Doesn't require special-casing / special props when calling Listbox components / any other special treatment. "Just works." If Floating UI doesn't alter how they structure their code, could work for a long time without anyone having to think about it.

The cons: A little fragile / magic. If Floating UI changes the structure of their portal code, it could silently break the rendering in the app.

Recommended if … you think that the Floating UI library isn't likely to change its structure/data-attributes/etc., or if you really want to prioritize not having to add a new prop to the .tsx files that explicitly sets the z-index.

---

Option Two: Explicitly passing props

The idea: add a new prop to the Listbox component; control utility classes based on prop value, and dynamically assign z-index to Listbox when called. Prop could either be an isInSidebar: boolean (default = false) or zIndex: 'z-20'|'z-50' (default = z-20).

The key code (Listbox.tsx):

// pass `isInSidebar` as a prop from network-interface-create.tsx → ListboxField → Listbox 
...
className: {`... ${isInSidebar ? 'z-50' : 'z-20'} ... `}

The pros: Explicitly calls the zIndex for the Listbox. More robust than other approach. Is still pretty straightforward.

The cons: Requires remembering to pass in the extra prop when calling the component. A little more work.

Recommended if … you want to prioritize reliability over time at the expense of an extra prop; if you think Floating UI (or other libraries that manipulate the DOM within the app) might change without warning.

---

I'd recommend the path that doesn't depend on an outside library staying consistent. (It's what I've done in the most recent commit.) I think transparency / clear intention in the .tsx (despite a new prop) is better than cleverness with the CSS and theoretically not requiring extra thinking in the .tsx. As I mentioned above, though, I'm super-open to pushback if you disagree, or have another approach I haven't thought of.

In order to make it easier to poke / test out, I've committed the Option Two version to this branch; just let me know if you'd rather go a different way.

(Also, about to go camping, and it's Labor Day weekend, so obviously no need to engage with this until next week!)

[david-crespo]

This is maybe uncharacteristic for me, but here I prefer a more magic solution because it doesn't rely on us to remember to pass the prop correctly. I'm not that worried about the solution breaking because I bet there's a way we could test with Playwright that it's working in one spot, and we wouldn't have to remember to test every spot either (which is good, because otherwise it's no better than having to remember to pass the prop).

I like your idea about detecting the floating-ui portals, though I wonder — is it just detecting that there is both a listbox and a side modal at the same time? I wonder if you could get into a weird situation where you have a listbox open in the main content and then you open a side modal and somehow the listbox wrongly gets the z-index that it would from being inside the side modal. Opening a side modal probably closes any open listboxes anyway, so this is pretty unlikely.

Another magic approach would be to use context to let Listbox detect when it's rendered inside of SideModal. Kinda janky because it couples Listbox to SideModal, but that's kind of the whole point here.

const ModalContext = createContext(false);

function SideModal({ children }) {
  return <ModalContext.Provider value={true}>{children}</ModalContext.Provider>
}

function Listbox({ children }) {
  const isInsideModal = useContext(ModalContext);
  return <div className={isInsideModal ? "z-50" : "z-20"}>{children}</div>
}

This would not be sensitive to floating-ui changing. Coincidentally, we already have a context like this defined for the regular non-side modal, though we're not using it anywhere.

console/libs/ui/lib/modal/Modal.tsx

Lines 17 to 21 in bfb680e

	const ModalContext = createContext(false)
	export const useIsInModal = () => {
	return useContext(ModalContext)
	}

[charliepark]

Ah, yeah, can see useContext as being a good approach. Will play around with it when I'm back early next week and make sure there aren't edge cases we're not thinking about, but on first glance it seems like it could be a pretty elegant solution for this situation.

[charliepark]

The useContext approach was really solid. Great call.

I added some tests (one unit test for a new function, some Playwright tests as well), and also moved the Tailwind classes into a zIndex object that will hopefully help with maintenance and any new layers needed in the future. If the zIndex approach isn't what you want, no worries, though I think having it all in one place means people will have less to hold in their heads. There were a few other z-10/z-20 instances in the codebase that I didn't switch to using the zIndex object, but it would be easy to do that if you think it'd be helpful.

I discovered that there was a flaw in my earlier ordering suggestion: there's at least one occasion (upload an image) where both a SideModal and a regular Modal need to be open at the same time, with the Modal floating above the SideModal. I adjusted the layers in the zIndex object to account for that scenario.

One curious aspect to Playwright: there were a few tests where content we expect to be obscured is still "see-able" by Playwright. Manual testing, and watching Playwright run through the test, confirms that the layers work as they should. It could be that my tests are missing something, or that Playwright has a funny relationship with grid-positioned content, or something else. Open to any thoughts you have on the tests I called out in lines 54–71 of z-index.e2e.ts.

This PR's getting a little rangey, but I think it's close. As always, happy to incorporate any feedback.

[david-crespo]

Fantastic, thank you. I'll have to think about the Playwright bits tomorrow.

I love the centralized list of z-indexes, I agree it's better to see them together. Here's a cute idea — if you put them directly in the tailwind config theme.extend as utilities

zIndex: {
  toast: '50',
  modalDropdown: '50',
  modal: '40',
  sideModalDropdown: '40',
  sideModal: '30',
  topBar: '20',
  popover: '10',
  contentDropdown: '10',
  content: '0',
},

you can avoid the syntactic sadness of string interpolation

- className={`${zIndex.modalDropdown} m-4`}
+ className="z-modalDropdown m-4"

[charliepark]

Here's a cute idea — if you put them directly in the tailwind config theme.extend as utilities

So clean!

[david-crespo]

Ok, I manually tested and it looks pretty great. Also the diff with whitespace changes hidden (SideModal indentation) shows that the PR is more focused and narrow than it looks at first. Still need to look at the tests for that issue you mentioned. Maybe if Playwright is too smart fo be fooled by visual overlap, we could do some screenshot diff testing. I am happy to mess around with that in a followup rather than asking you to spend even more time on this.

I found one last tiny issue in both Modal and SideModal, though it's so small and hard to run into (you need a tiny window) that I would be fine logging it as an issue and not fixing it right now. I tried adding a higher z-index to the modal footer and it didn't work. Maybe something about positioning or not sharing a parent or something?

[david-crespo]

FYI you'll have to npx playwright install again after I merged main here.

[david-crespo]

It's not very well documented and I could not find anyone actually doing this, but before Playwright will click something it checks a bunch of stuff, like that it's attached to the DOM, stable, blah blah blah. Most importantly, it checks that it can receive events, i.e., there is nothing above it stealing its clicks. So one way to check whether something is obscured is to assert that it is "visible" (doesn't actually mean in Playwright what the word means) and in viewport, but an attempt to click it still fails. There's a trial option to click() that lets you do the actionability checks without actually doing the click, and I set the timeout short so it wouldn't sit there for a while. Then in order to assert that it fails, you have to wrap it in a expect().rejects.toThrow, which is also very poorly documented and messes me up every time.

In any case, the tests seem to work — when I change the z-indexes to make them fail, they do in fact fail.

[david-crespo]

Awesome work, and thanks for going along with all the back and forth!

[charliepark]

Hey, awesome! Great job on the new tests / thanks for explaining the situation there.

Thank you for being up for the back and forth as well! 😄 This was fun, and I'm glad we got the bug fixed!

[benjaminleonard]

And let me know if you'd like some company merch for your trouble!