stavroskasidis / BlazorContextMenu

A context menu component for Blazor !
MIT License
532 stars 58 forks source link

SubMenu component doesn't use ContextMenuBase.ZIndex #121

Closed rummelsworth closed 2 years ago

rummelsworth commented 2 years ago

Describe the bug

Unlike the ContextMenu component, the SubMenu component doesn't use ContextMenuBase.ZIndex.

This can lead to situations where, for example, the context menu trigger is on the right side of the page, it opens up the context menu to the left, then opens up a submenu back to the right, the top-level menu text can appear on top of the submenu area. It can depend on the top-to-bottom order of the context menu items in the DOM, since z-index isn't specified for them. If the "underneath" item is after the submenu, you'll see the item's whole text on top of the submenu. If not, you'll at least see the little submenu arrow on top.

The natural way to fix this would be for the developer-user to set ZIndex on the ContextMenu as well as all SubMenus. This doesn't work because SubMenu ignores ZIndex despite having access to it.

To Reproduce

Steps to reproduce the behavior:

  1. Clone/download the repro repo here:
  2. Build & run the "bug repro" project (a Blazor WASM app).
  3. Right-click the trigger paragraph on the right side of the index page.
    • Make sure you right-click pretty close to the right edge of the paragraph, so that the submenu is forced to open to the left.
  4. Navigate to the submenu and then the sub-submenu.
  5. See the top-level menu elements appear on top of the sub-submenu area.

Expected behavior

Each submenu and all of its content should appear on top of all ancestor-ish menu content, at least as long as I'm properly setting the ZIndex property on all SubMenu components.

Screenshots

image

Desktop:

Additional context

The fix (or a workaround) appears to be as simple as adding the same z-index setting already in ContextMenu.razor to SubMenu.razor. The developer still has to arrange their own ZIndex values over the submenus, but that's probably not a big deal. Otherwise, the library itself would have to try to intelligently set the ZIndex values. Maybe that's easy? Dunno.

stavroskasidis commented 2 years ago

Thank you for your detailed report. I will fix asap

stavroskasidis commented 2 years ago

Fixed in 1.14. Thank you

rummelsworth commented 2 years ago

Will v1.14 be releasing to nuget soon? There's no rush on my end, as I'd already taken a local fork of v1.13 to put the little fix into. Just curious. Thanks again!

stavroskasidis commented 2 years ago

Ha forgot to merge the PR. Thanks for the reminder