Make arrow panels more versatile. #4692

mstange · 2023-07-05T16:47:10Z

This PR implements some changes to the arrow panel code which make arrow panels usable for the dropped functions panel in the call tree settings.

With the previous implementation, I encountered these issues:

When the source view was open, the panel would be clipped to the call tree bounds and could not overlap the source view. The overflow:auto clip from the splitter view was applying to the panel.
With a narrow viewport or an unusually positioned anchor element, the panel would sometimes be positioned partially offscreen.
It was tricky to enforce a maximum height on the panel in a way that squished the list box inside the panel.
The panel would reposition itself based on the size of the anchor element while the panel was open. E.g. you would click an "x" button to remove a dropped function, and the anchor button's label would change from "10 dropped functions" to "9 dropped functions", causing the button to shrink. Then the panel would shift slightly to the left, moving the "x" button out from under the mouse cursor.

This commit fixes all of these issues.

The most important change is the switch to position:fixed: Now the panel will no longer be clipped by overflow:hidden/auto ancestor of the button that opens the panel.
Furthermore, the panel position is only computed during opening and then remains unchanged while the panel is open.
And there is some logic to make the panel fit onscreen (while also making sure that the arrow still points at the right place).

┆Issue is synchronized with this Jira Task

This PR implements some changes to the arrow panel code which make arrow panels usable for the dropped functions panel in the call tree settings. With the previous implementation, I encountered these issues: - When the source view was open, the panel would be clipped to the call tree bounds and could not overlap the source view. The overflow:auto clip from the splitter view was applying to the panel. - With a narrow viewport or an unusually positioned anchor element, the panel would sometimes be positioned partially offscreen. - It was tricky to enforce a maximum height on the panel in a way that squished the list box inside the panel. - The panel would reposition itself based on the size of the anchor element *while the panel was open*. E.g. you would click an "x" button to remove a dropped function, and the anchor button's label would change from "10 dropped functions" to "9 dropped functions", causing the button to shrink. Then the panel would shift slightly to the left, moving the "x" button out from under the mouse cursor. This commit fixes all of these issues. The most important change is the switch to position:fixed: Now the panel will no longer be clipped by overflow:hidden/auto ancestor of the button that opens the panel. Furthermore, the panel position is only computed during opening and then remains unchanged while the panel is open. And there is some logic to make the panel fit onscreen (while also making sure that the arrow still points at the right place).

codecov · 2023-07-05T16:55:50Z

Codecov Report

Patch coverage: 95.00% and no project coverage change.

Comparison is base (eac238a) 88.33% compared to head (0354a40) 88.34%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4692   +/-   ##
=======================================
  Coverage   88.33%   88.34%           
=======================================
  Files         299      299           
  Lines       26709    26724   +15     
  Branches     7206     7209    +3     
=======================================
+ Hits        23594    23608   +14     
- Misses       2902     2903    +1     
  Partials      213      213

Impacted Files	Coverage Δ
src/components/app/MenuButtons/Permalink.js	`81.81% <ø> (ø)`
src/components/app/MenuButtons/index.js	`88.17% <ø> (ø)`
...rc/components/shared/ButtonWithPanel/ArrowPanel.js	`91.48% <92.85%> (-0.18%)`	⬇️
src/components/shared/ButtonWithPanel/index.js	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

julienw

This looks good to me, but I noticed an issue when the permalink window auto opens after uploading, but only when we sanitize the profile by unchecking some checkbox. I believe this is because there's a transition, and then some computations are not correct anymore at the end of the transition (probably anchorRectRelativeToPage.top but maybe others too). It's not clear to me how to fix this with this new logic though... What do you think?

src/components/shared/ButtonWithPanel/ArrowPanel.js

julienw · 2023-07-06T12:46:29Z

src/components/shared/ButtonWithPanel/ArrowPanel.js

+        left: panelLeft,
+        top: anchorPoint.top,
+        width: adjustedPanelWidth,
+        height: 0, // ignored


do we need this one at all if we don't use it?

We don't need it except to make the types happy. I'm putting it in a state field called panelRect. What would you suggest instead? I could change it to instead have three fields, panelLeft etc, but I'm not sure if that's better.

Yeah I was wondering about removing height everywhere if we don't use it... Did you have other ideas about it?

We use anchorRectRelativeToViewport.height.

julienw · 2023-07-06T13:19:59Z

src/components/shared/ButtonWithPanel/ArrowPanel.css

+  display: flex;
+  flex-flow: column nowrap;
+  padding-top: var(--internal-arrow-clip-height);
+  margin-bottom: auto;


It's not clear to me what this margin-bottom does, can you please explain?

Oops, leftover from something I was trying. It does nothing, I'll remove it.

julienw · 2023-07-06T13:21:11Z

src/components/shared/ButtonWithPanel/ArrowPanel.css

+  color: black;
+  line-height: 1.3;
+  pointer-events: none;
+  text-align: left;


It's not clear to me why there's a text-align here?
If you decide to keep it, what do you think about using start instead?

I don't remember why I put a text-align there. I'll remove it.

src/components/shared/ButtonWithPanel/ArrowPanel.css

julienw · 2023-07-06T13:25:33Z

src/components/shared/ButtonWithPanel/ArrowPanel.css

-  --internal-approx-distance-to-bottom: 100px;
+  min-height: 0;
+  flex-flow: column nowrap;
+  flex-shrink: 1;


optional: For this I'd use flex: auto which is IMO more explicit.

julienw · 2023-07-06T16:41:20Z

src/components/shared/ButtonWithPanel/ArrowPanel.js

+  // from the right edge of the panel.
+  // If set to "left", the arrow will be positioned `distanceFromEdge` pixels
+  // from the left edge of the panel.
+  anchorEdge: 'left' | 'right',


Would it be easy to support 'start' | 'end' instead?

mstange · 2023-07-12T01:46:50Z

This looks good to me, but I noticed an issue when the permalink window auto opens after uploading, but only when we sanitize the profile by unchecking some checkbox. I believe this is because there's a transition

Ugh good catch, that's indeed a problem. I'll have to think about it a bit. (What I really want is a way to get the location of an element "as if all ancestor transforms were reset to the identity transform", but I don't think I get to have that.)

mstange · 2023-07-12T01:57:23Z

Maybe we can just delay the opening until after the transition completes. Do you know where the code that opens the permalink panel in this case lives?

julienw · 2023-07-12T15:44:18Z

src/components/shared/ButtonWithPanel/ArrowPanel.js

+      left: anchorRectRelativeToPage.left - win.scrollX,
+      top: anchorRectRelativeToPage.top - win.scrollY,


It's not super clear to me why we remove scrollX/scrollY.
According to MDN left/top are already relative to the viewport, and we need to add them to get values relative to the document.

julienw · 2023-07-12T15:46:16Z

Maybe we can just delay the opening until after the transition completes. Do you know where the code that opens the permalink panel in this case lives?

We use this property initialOpen:

profiler/src/components/app/MenuButtons/Permalink.js

Line 74 in 2c3253c

initialOpen={this.props.isNewlyPublished}

This value comes from this reducer:

profiler/src/reducers/app.js

Lines 195 to 209 in 2c3253c

    
           /** 
        
            * This reducer holds the state for whether or not a profile was newly uploaded 
        
            * or not. This way we can show the permalink to the user. 
        
            */ 
        
           const isNewlyPublished: Reducer<boolean> = (state = false, action) => { 
        
             switch (action.type) { 
        
               case 'PROFILE_PUBLISHED': 
        
               case 'SANITIZED_PROFILE_PUBLISHED': 
        
                 return true; 
        
               case 'DISMISS_NEWLY_PUBLISHED': 
        
                 return false; 
        
               default: 
        
                 return state; 
        
             } 
        
           };

The "profile sanitized" action is dispatched here:

profiler/src/actions/publish.js

Lines 291 to 306 in 2c3253c

    
           if (removeProfileInformation) { 
        
             const { committedRanges, oldThreadIndexToNew, profile } = 
        
               sanitizedInformation; 
        
             // Hide the old UI gracefully. 
        
             await dispatch(hideStaleProfile()); 
        
             // Update the UrlState so that we are sanitized. 
        
             dispatch( 
        
               profileSanitized( 
        
                 hash, 
        
                 committedRanges, 
        
                 oldThreadIndexToNew, 
        
                 profileName, 
        
                 prePublishedState 
        
               ) 
        
             );

The hideStaleProfile action is the one controlling the "fadeout" transition.
But then the "fadein" transition kicks in in

profiler/src/components/app/ProfileViewer.css

Lines 65 to 68 in 2c3253c

    
           .profileViewerFadeInSanitized { 
        
             animation-duration: 500ms; 
        
             animation-name: profileViewerFadeInSanitized; 
        
           }

.
This one is controlled by this state:

profiler/src/reducers/publish.js

Lines 185 to 203 in 2c3253c

    
           /** 
        
            * This piece of state controls the animation of hiding the profile when it's stale. 
        
            * Stale profiles are ones that have been mutated in some way. This happens when 
        
            * the user goes from an original profile to a sanitized profile, or when they revert 
        
            * to the original. We want to display a helpful animation when this happens, and so 
        
            * this piece of state controls it. 
        
            */ 
        
           const isHidingStaleProfile: Reducer<boolean> = (state = false, action) => { 
        
             switch (action.type) { 
        
               case 'HIDE_STALE_PROFILE': 
        
                 return true; 
        
               case 'VIEW_FULL_PROFILE': 
        
               case 'VIEW_ORIGINS_PROFILE': 
        
               case 'VIEW_ACTIVE_TAB_PROFILE': 
        
                 return false; 
        
               default: 
        
                 return state; 
        
             } 
        
           };

So maybe we can indeed tweak initialOpen above to follow the same logic as the fadein transition. We probably need to add yet another state for that? It's kind of hacky and fragile though...

For width and height you could always use offsetWidth/offsetHeight, but I don't have an idea for top and left.

julienw · 2023-07-12T15:55:38Z

Maybe hideStaleProfile could return 2 promises, one for the fadeOut, one for the fadeIn(with 2 different setTimeout), then this would make it possible to await them separately. But this is not great, and not guaranteed to be synchronized with the CSS transitions, given the react machinery (probably with some rAF) handling the fadeout to fadein action.

mstange requested a review from julienw July 5, 2023 16:47

mstange self-assigned this Jul 5, 2023

julienw reviewed Jul 6, 2023

View reviewed changes

julienw reviewed Jul 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make arrow panels more versatile. #4692

Make arrow panels more versatile. #4692

mstange commented Jul 5, 2023 •

edited by data-sync-user

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading

julienw left a comment

julienw Jul 6, 2023

mstange Jul 12, 2023

julienw Jul 12, 2023

mstange Jul 12, 2023

julienw Jul 6, 2023

mstange Jul 12, 2023

julienw Jul 6, 2023

mstange Jul 12, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

mstange commented Jul 12, 2023

mstange commented Jul 12, 2023

julienw Jul 12, 2023

julienw commented Jul 12, 2023

julienw commented Jul 12, 2023 •

edited

Loading

		left: anchorRectRelativeToPage.left - win.scrollX,
		top: anchorRectRelativeToPage.top - win.scrollY,

Make arrow panels more versatile. #4692

Are you sure you want to change the base?

Make arrow panels more versatile. #4692

Conversation

mstange commented Jul 5, 2023 • edited by data-sync-user Loading

codecov bot commented Jul 5, 2023 • edited Loading

Codecov Report

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstange commented Jul 12, 2023

mstange commented Jul 12, 2023

Choose a reason for hiding this comment

julienw commented Jul 12, 2023

julienw commented Jul 12, 2023 • edited Loading

mstange commented Jul 5, 2023 •

edited by data-sync-user

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading

julienw commented Jul 12, 2023 •

edited

Loading