Better dropdowns

[Gazook89]

Description

We have a problem with the snippet menus getting cut-off by the divider bar, since those are normal absolutely-positioned elements within a pane that gets clipped.

This is a basic implementation of new menus using the Anchor Positioning API. It creates a new component Dropdown.jsx. It could be used elsewhere, such as in the Navigation bar, but for now I've limited it to just the snippet menus to avoid going overboard. The original nav components are still in, as a result, but could be replaced further down the line.

improvements

The menus are not clipped by the pane, they exist in the top layer.

The menus are "click to open" rather than on hover. This is more accessible for anyone with motor disability, but helpful for everyone. For developers, you can now click a menu open, and then actually inspect it without having to use the dev tools to manually force the "hover" state.

Menus are dismissed by clicking anywhere off the menu (or on the original trigger). Question: do people prefer that the menu closes after clicking a snippet, or leave the menu open? I've made it so menus close after selection by default, with an attribute no-dismiss="true" that can be added for a menu if you want it to remain open. It's not used in this PR, but could be somewhere else. The attribute takes "true" as a string because react custom attributes only take strings.

next steps

The menus have no complex algorithms for determining placement right now, but they could (fairly easily). The menus right now default to opening down, aligned to the left edge of the trigger. Submenus open to the right, aligned to the top edge of the trigger. Both menus and submenus can flip inline if no space.

Anchor Positioning is now supported in the big browsers since about the beginning of this year. If we want to support further back, that's possible using resize observers, position calculations, absolute positioning, etc. I have it set up and working in my old UI Overhaul PR. But to keep this small I left it out for now. The Oddbird polyfill has been added, and is conditionally applied only if the users browser doesn't support AP.

The menu items are basically the same structure as exists in live, but they could be improved. I would later create a basic 'button' component that would be used in menubars (navbar, snippetBar) and within menus.

Related Issues or Discussions

• Closes Snippet dropdowns cut off behind the divider bar #3840

QA Instructions, Screenshots, Recordings

Is able to cross the divider bar by existing in the top-layer:

If the window is made small enough that a menu would go off screen, it instead has fallback positions. This shows a nested snippet menu automatically re-positioning to the left of the parent menu, rather than default right, so it stays within the window if possible:

To Test:

In the Snippet Bar of the editor:

• Clicking a snippet menu opens it
  • it opens below, aligned to left of, the menu trigger
• Menus remain open and only close when a click happens anywhere else
  • the menus stay open if clicking a submenu trigger
  • clicking a menuitem button performs the snippet action and closes the menu
• Menus open in top-layer, not obscured by divider
  • Menus use fallback options for positions if the window width is decreased to not allow space.
• Menus are tab-navigable
  • tab for next button
  • shift-tab for previous button
  • space or enter to click a button, including opening a submit
  • esc to close current menu (closes a submenu but keeps parent menu open)
  • tab into opened submenu
• browser support
  • works in chrome
  • works in safari
  • works in firefox
  • works in iOS
  • works on Android
  • works on older browsers (polyfill should do this, and i haven't tested lately but did test after the bulk of the work was done, on older Firefox (pre-Anchor Positioning/Popover API's).
• no animals were hurt in the production of this PR.

The same list, but as markdown

In the Snippet Bar of the editor:

- [ ] Clicking a snippet menu opens it
  - [ ] it opens below, aligned to left of, the menu trigger
- [ ] Menus remain open and only close when a click happens anywhere else
  - [ ] the menus stay open if clicking a submenu trigger
  - [ ] clicking a menuitem button performs the snippet action and closes the menu
- [ ] Menus open in `top-layer`, not obscured by divider
  - [ ] Menus use fallback options for positions if the window width is decreased to not allow space.
- [ ] Menus are tab-navigable
  - [ ] `tab` for next button
  - [ ] `shift-tab` for previous button
  - [ ] `space` or `enter` to click a button, including opening a submit
  - [ ] `esc` to close current menu (closes a submenu but keeps parent menu open)
  - [ ] `tab` into opened submenu
- [ ] browser support
  - [ ] works in chrome
  - [ ] works in safari
  - [ ] works in firefox 
  - [ ] works in iOS
  - [ ] works on Android
  - [ ] works on older browsers (polyfill should do this, and i haven't tested *lately* but did test after the bulk of the work was done, on older Firefox (pre-Anchor Positioning/Popover API's).
- [ ] no animals were hurt in the production of this PR.

[5e-Cleric]

Did this feature have a nice degradation? what will be shown in older browsers?

[Gazook89]

What happens in older browsers is the items are absolutely positioned at 0, 0 of the next relative positioned ancestor. So in the case of the snippet bar, the appear at the top left of the editor panel. In the case it's a submenu, they stack on top of each other. In the future, the nav bar menus would stack at the top left of the window.

That said, I do have a whole fallback/polyfill for older browsers that I can add if we want. It's not really that much extra, but I really wanted to limit the scope of the PR, or make clear how simple it could be.

Here is a screenshot with Anchor Positioning turned off, with a menu and submenu opened:

Note, that could look a lot better with other improvements like a thin border and a little drop-shadow, which would make it much more clear that two menus are on top of each other.

[calculuschild]

Is there a convenient polyfill like we do for core-js, so we just import something and don't have to worry about maintaining any custom stuff?
Like we do with this?

homebrewery/client/homebrew/homebrew.jsx

Line 1 in c12bdcd

import 'core-js/es/string/to-well-formed.js'; // Polyfill for older browsers

[Gazook89]

Yes, https://anchor-positioning.oddbird.net/

I believe I have mentioned that before and tried it way back when, but couldn't get it work with require.js or whatever. But it could be tried again.

[calculuschild]

Ok. At this point, I think we've proven to ourselves that any code change that is not backwards compatible with old browser versions will always cause a panic, and we end up having to rush to make a fix. If we know this one will need a polyfill let's just handle it here.

[5e-Cleric]

Yeah here the polyfill is very needed and this feature is great so lets add in the polyfill and testing as soon as we can.

[Gazook89]

Trying to get this to work. I first tried just the basic CDN script based on browser support but hit an issue. To get around that, I went the "install as NPM package" route, but setup to lazy-load in the case that anchor positioning isn't supported by the browser.

The issue is that React removes unsupported inline style properties at runtime. So the way I have this setup is elements with style={{ positionAnchor: ... }}, but since that property isn't supported by the browser React cuts it out.

The polyfill looks through the computed styles of the dom for positionAnchor in order to target its work, but if React is removing all those identifiers then it obviously won't work.

So instead, I could use style={{ '--position-anchor': ... }} to set a custom property, which is always allowed by browsers and thus by React. That seems to have some effect and the polyfill is finding the correct elements. But the result is stretching the anchor elements across the whole viewport.

Now I have to figure out why the polyfill just isn't working as expected.

[Gazook89]

It might be because I think I have the positioned element as a child of the anchor when they likely should be siblings.

I believe I nested them in order to work with my own mini polyfill, but if using oddbird then I can not do that.

So I'll check that when I'm home.

[5e-Cleric]

Overall great changes, i assume its not ready for merge, since i see an empty file, but looks good, left a few small comments

[Gazook89]

I moved the tests into the tests/components/dropdown/ folder. And cleaned up the little things you commented on.

Regarding the "empty file", i assume you mean the styleMock.js? I tried removing it, but then tests didn't run. This is one of those things that an LLM put together, so I queried what was up with that file. The answer is basically "Jest needs this file to substitute in for the .less import, since Jest runs in Node and not a browser.

The full answer

The file is there purely to satisfy Jest when a component imports a stylesheet.

Jest runs in Node, not a browser bundler, so an import like dropdown.less is not something it can execute on its own. The moduleNameMapper entry in package.json tells Jest: when you see .less or .css, load styleMock.js instead. That mock can be basically empty, because the test only needs the import to resolve, not to actually apply styles.

So the file’s purpose is:

Prevent import errors for style files.
Let component tests focus on DOM structure and behavior.
Keep the test environment independent from the CSS pipeline.
If you remove it, Jest will try to resolve dropdown.less directly and fail. The mock can stay minimal, but it does need to exist as a resolvable module.

I assume that with additional component tests this will continue to be needed, but that this one file will work for all of those tests.

I believe this is ready.

[5e-Cleric]

Regarding the "empty file", i assume you mean the styleMock.js? I tried removing it, but then tests didn't run. This is one of those things that an LLM put together, so I queried what was up with that file. The answer is basically "Jest needs this file to substitute in for the .less import, since Jest runs in Node and not a browser.

Eh, no? we have plenty of less files everywhere else and no mock file is needed for each of them, so we can do them like they are done elsewhere.

[G-Ambatte]

we have plenty of less files everywhere else and no mock file is needed for each of them

I don't believe that we actually test any rendered elements anywhere else; I half-recall a discussion along the lines of "visual elements don't require automated testing as any issue with them will be immediately noticed".
I'm prepared to learn I've misremembered/am just flat wrong about that, of course. But in keeping with that line of thought for literally every other React component in the project, I'm not sure why testing has become necessary here.

---

another small thing, lets remove the beta tags on most snippets, they have been stable for like... forever.

I don't disagree that the BETA snippets have been stable for a long time, but removing the tags is unrelated to the purpose of this PR and probably should have been handled in a small focused PR of its own, where the merits of such a change could have been discussed and decided independently of these other menu changes.

That said, it's probably not worth the effort to unwind it from the current state of the PR.

---

On testing, I'm receiving a console error of Each child in a list should have a unique "key" prop error for the render method of Dropdown; when it is passed a child from SnippetGroup, it is creating multiple sibling components without unique keys.

A little experimentation seems to show that this issue is resolved by changing line 339 of snippetbar.jsx to the following:

<Dropdown groupName={snippet.name} icon={snippet.icon} key={snippet.name}>

---

On an aesthetic note, if the width of the editor panel is less than 1026px, the snippet bar is increased to two lines - the menus are all much wider than they were previously. The Editor tools collapse to the extreme left of the window and the mouseover label is no longer visible on the screen (see image below for example of UNDO being mostly off-screen).

---

Attempting to comprehend the ref function of AnchoredTrigger/Box is more than I currently have mental capacity to achieve, so I have not yet conducted an actual capital-C-R Code Review.

[Gazook89]

every other React component in the project, I'm not sure why testing has become necessary here

I don't think historical inertia is enough of a reason to not test, on it's own. If we don't want to test because it's hard or something, we don't have to. But I have found in other projects that testing components is helpful. Being able to run a script to test different combinations of attributes/props rather than making small alterations and reloading the page each time saves time and is more consistent. There are plenty of things that are not "immediately noticed" that we later go back and patch. And having tests helps understand what things are important and why. If these tests are not doing that then they can be improved, but I wouldn't say "components don't need tests".

beta tags

I mostly agree here. I didn't really care one way or another if we left them in. But, to support the idea of removal, we don't really have any user-created open issues for any of these, so to some degree they must be "okay".

unique key

fixed, thank you for pointing that out and offering the fix.

menus are all much wider

The idea is that the menu trigger should have the same "grid" as it's menu items. Our current site does not-- the icon space in the trigger is smaller than the icon space in the menu items:

Live	PR

tools collapse to the extreme left

This is set by some magic numbers in snippetbar.less on a couple lines, which I can fix quickly for this PR. In the future, a better layout should be done to avoid these magic numbers.

mouseover label is no longer visible

The tooltips is not something I'm working on here, but I have another branch not yet pushed to a PR that does tackle this. The tooltips use static positioning. They should be upgraded to also using Anchor Positioning and Popover API, just like these menus. And then it will be a non-issue. This was another reason that my UI Overhaul PR became so unwieldy-- lots of this UI stuff gets tangled up in a way that is hard to address one thing at a time. If people would like, I can make a branch from this branch and we can merge that first. But I'm not going to do that until there is a general mumbling of acceptance.

Thanks for reviewing

[Gazook89]

I should also add that I believe that the menus can be made more visually distinct from the menubars they come from, but I didn't do that in this PR. I didn't want to add a bunch of changes in the .less files that weren't directly related to functionality. But things like a thin border, subtle box shadow, small margins around expanding menus could make menus contrast better with the elements they overlap.

[5e-Cleric]

How ready is this? can we merge?

[Gazook89]

How ready is this? can we merge?

I guess I do have a question about how to manage the extra space that the snippet menus take now, related to this:

tools collapse to the extreme left

I thought I would just adjust the magic numbers we have for the breakpoints in the snippetbar.less for the wrapping, but as I do so I'm thinking:

A. It might be better to just make the snippet bar two rows, permanently.

In my older UI Overhaul PR, I had something that was two rows as well, though it was a little more refined. Moving to two rows with this PR would be a step in that direction:

B. If not "A", then maybe the snippet menu triggers are too wide, and that matching the trigger menu spacing to the menu item spacing isn't such a big deal.

I guess I'm up for whichever, but I would lean towards "A".

I'm out of town for the next week, so I can adjust that shortly after I get back.

[5e-Cleric]

How ready is this? can we merge?

I guess I do have a question about how to manage the extra space that the snippet menus take now, related to this:

tools collapse to the extreme left

I thought I would just adjust the magic numbers we have for the breakpoints in the snippetbar.less for the wrapping, but as I do so I'm thinking:

A. It might be better to just make the snippet bar two rows, permanently.

Not against it, but i do have to say, 70% of the time, i don't have the need for two rows at all. there is plenty of space in screen.

And if we do end up having an editor that we can move out of the screen or to the bottom, the double line would be unnecessary.

That said, it is a change we can undo at any point, i think it is preferred, barring dissagreement from @calculuschild

I guess I'm up for whichever, but I would lean towards "A".

So do i

I'm out of town for the next week, so I can adjust that shortly after I get back.

As always, no hurry.

[dbolack-ab]

This is a massive improvement.