Fix call node context menu being hidden behind source view bottom box#6045
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6045 +/- ##
==========================================
- Coverage 83.78% 83.77% -0.01%
==========================================
Files 329 329
Lines 34528 34547 +19
Branches 9659 9574 -85
==========================================
+ Hits 28930 28943 +13
- Misses 5169 5175 +6
Partials 429 429 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fatadel
left a comment
There was a problem hiding this comment.
Hey @canova, thanks for the PR! I've tested it locally and it seems to work fine.
However, I've found an alternate fix worth considering: just drop position: relative; z-index: 0 from .DetailsContainer. Tested locally, fixes the same bug. It seems like nothing inside Details depends on that stacking context.
|
Thanks for the review @fatadel! I was thinking that I saw one behavior difference, previously the screenshots would render on top of the context menu: But with these position and z-index rules removed, it renders below it: But I think this is actually a better behavior. Beause the context menu is an explicit gesture, as oppose to just hovering. So I think context menu should render on top of the screenshots. It looks like we added these rules in #4606, but I can't see anything that breaks in the splitter when I remove them. So I think they are safe to remove. I'll update the patch |
adac6b0 to
0086188
Compare
DetailsContainer set `position: relative; z-index: 0`, creating a stacking context that trapped the context menus (rendered inside DetailsContainer) at z-index 0. The BottomBox is a sibling of DetailsContainer that comes later in the DOM, so it painted on top of the menus whenever the source view was open. Removing the stacking context from DetailsContainer lets the context menu's --z-context-menu compete at .profileViewer level and paint above the BottomBox. The parent .profileViewer still provides a stacking context for the KeyboardShortcut overlay.
0086188 to
7ad991d
Compare
|
LGTM! |
Changes: [Nazım Can Altınova] Fix call node context menu being hidden behind source view bottom box (#6045) [Nazım Can Altınova] Pass `--use-env-proxy` only when the node version is >= 24 (#6064) [fatadel] Upgrade @firefox-devtools/react-contextmenu to 5.2.4 (#6066) [Markus Stange] Switch profiler-edit from minimist to commander (#6065) [Markus Stange] Support reading profiles from JsonSlabs files (#6037) [Florian Quèze] Don't fail profile processing when a marker's stack field is not a backtrace (#6069) [fatadel] Replace the footer-links overlay with a settings menu (#6042) [fatadel] Upgrade @types/node to match Node 24 (#6070) [fatadel] Remove unused undici-types package (#6074) [cathaysia] Update isLocalURL to include LAN addresses, .local domains, and hostn… (#5973) [Markus Stange] Fix from-url with binary profiles (#6072) [fatadel] Upgrade to React 19 (#6067) [Markus Stange] Add an insertStackLabels helper. (#6076) [fatadel] Drive counter tooltips from a tooltipRows schema (#6023) [fatadel] Add TrackPower--tooltip-average-power-microwatt (#6080) [Markus Stange] Downgrade to React 19.1 to fix unusable dev build performance. (#6082) [Nazım Can Altınova] Add source map symbolication and source view support (#6018) [spokodev] fix(FilterNavigatorBar): clip overflow so many breadcrumbs do not expand the parent (#6085) [Markus Stange] Move paddings inside the tree header cells. (#6002) [Markus Stange] Add an --insert-label-frames argument to the profiler-edit tool (#5966) [Markus Stange] Stop printing "error: too many arguments" during tests. (#6088) [Markus Stange] More additions to profiler-edit, for sp3 profiles (#6009) [Nazım Can Altınova] Do not rely on localized texts in the settings menu tests (#6101) And special thanks to our localizers: be: Andrei Mukamolau de: Ger de: Michael Köhler de: Ralf Duehnfahr el: Jim Spentzos en-CA: chutten en-GB: Ian Neal es-CL: ravmn fr: Théo Chevalier fr: wy fur: Fabio Tomat fy-NL: Fjoerfoks ia: Melo46 it: Francesco Lodolo [:flod] nl: Mark Heijl ru: Valery Ledovskoy sr: Марко Костић (Marko Kostić) sv-SE: Andreas Pettersson tr: Grk tr: Selim Şumlu zh-CN: Olvcpr423 zh-TW: Pin-guang Chen
The CallNodeContextMenu and MaybeMarkerContextMenu were rendered inside .Details, which sits inside .DetailsContainer. DetailsContainer sets
position: relative; z-index: 0, which creates a stacking context that capped the context menu's z-index at 0 relative to its siblings. Since the BottomBox is a sibling of DetailsContainer declared later in the DOM, it always painted on top of the context menus, and it makes them unreadable when the source view was open.Moved both context menus from Details.tsx into ProfileViewer.tsx, and placed after BottomBox, so they share a stacking context with both DetailsContainer and BottomBox. Their
--z-context-menu: 5now paints above the BottomBox as intended. Context menus are wired to triggers by id rather than DOM proximity, so the move does not affect the behavior.Technically MaybeMarkerContextMenu can't be visible while we still have the source view visible, but I think it makes sense to keep them together.
Before:
After:
Example profile:
Before / after