1815130 - No header appears when opening the Profiler panel while it's in the overflow menu

Status: VERIFIED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Itiel]

STR:

1. Add the profiler to the toolbar
2. Pin it to the overflow menu
3. Open the profiler from the overflow menu

AR:
No panel header

ER:
A panel header should appear

Comment 1

[Itiel]

Gijs, got an idea on how to approach this?
This whole thing revolves around this [1] code, and more specifically, the return here [2] doesn't allow the creation of a new panel-header box in line 1406 onwards.
I can't think of an idea on how to make this generic enough and not overly specific to a certain panel... Any hints would be appreciated :)

[1] https://searchfox.org/mozilla-central/rev/0bf957f909ae1f3d19b43fd4edfc277342554836/browser/components/customizableui/PanelMultiView.jsm#1378-1399
[2] https://searchfox.org/mozilla-central/rev/0bf957f909ae1f3d19b43fd4edfc277342554836/browser/components/customizableui/PanelMultiView.jsm#1392

Comment 2

[:Gijs (he/him)]

(In reply to Itiel from comment #1)

Gijs, got an idea on how to approach this?
This whole thing revolves around this [1] code, and more specifically, the return here [2] doesn't allow the creation of a new panel-header box in line 1406 onwards.
I can't think of an idea on how to make this generic enough and not overly specific to a certain panel... Any hints would be appreciated :)

[1] https://searchfox.org/mozilla-central/rev/0bf957f909ae1f3d19b43fd4edfc277342554836/browser/components/customizableui/PanelMultiView.jsm#1378-1399
[2] https://searchfox.org/mozilla-central/rev/0bf957f909ae1f3d19b43fd4edfc277342554836/browser/components/customizableui/PanelMultiView.jsm#1392

I'm a bit confused by these links - what gets passed to the headerText setter? It sounds like it's an empty string? The block you highlighted should already manipulate an existing header element or remove it, depending on the assignment to headerText being an empty string or not. If there's no headerText, it's expected we don't show the header, and the bug is probably that there isn't header text for this view. I can't quickly see from https://searchfox.org/mozilla-central/source/devtools/client/performance-new/popup/menu-button.jsm.js how the subview is being shown in this case, so not sure what triggers that. Perhaps Florian can help as it looks like he reviewed some of this code?

Comment 3

[Julien Wajsberg [:julienw]]

Hey Gijs,

The header is statically defined in the markup already, see https://searchfox.org/mozilla-central/rev/188d0f76a73e0671d12e744a71e9f5701668cc37/browser/base/content/appmenu-viewcache.inc.xhtml#249-254

I don't think we set anything to headerText imperatively otherwise.

Hope this helps!

Comment 4

[:Gijs (he/him)]

If I had to guess something is calling openPopup in PanelMultiView which tries to show the view as a main view, when it's supposed to be shown as a subview. But I don't know the profiler code and couldn't figure it out by looking at the code - it's not clear to me how it opens the subview when clicking the dropdown part of the button; I don't see any code for this in the menu-button.jsm.js file.

But basically I expect we're hitting https://searchfox.org/mozilla-central/rev/188d0f76a73e0671d12e744a71e9f5701668cc37/browser/components/customizableui/PanelMultiView.jsm#813 in _showMainView so to fix this someone would need to check if I'm right and if so, figure out why that happens.

Comment 5

[Julien Wajsberg [:julienw]]

All this part should be handled in CustomizableUI.jsm because we use the widget type "button-and-view". I believe Florian implemented this type, but the goal was to reuse as much existing code as possible.

I believe that what you're looking for is in https://searchfox.org/mozilla-central/rev/08362489086b10de96e7a199b267ea5504c01583/browser/components/customizableui/CustomizableUI.jsm#2191-2207 ?

Comment 6

[Itiel]

(In reply to :Gijs (he/him) from comment #2)

(In reply to Itiel from comment #1)

Gijs, got an idea on how to approach this?
This whole thing revolves around this [1] code, and more specifically, the return here [2] doesn't allow the creation of a new panel-header box in line 1406 onwards.
I can't think of an idea on how to make this generic enough and not overly specific to a certain panel... Any hints would be appreciated :)

[1] https://searchfox.org/mozilla-central/rev/0bf957f909ae1f3d19b43fd4edfc277342554836/browser/components/customizableui/PanelMultiView.jsm#1378-1399
[2] https://searchfox.org/mozilla-central/rev/0bf957f909ae1f3d19b43fd4edfc277342554836/browser/components/customizableui/PanelMultiView.jsm#1392

I'm a bit confused by these links - what gets passed to the headerText setter? It sounds like it's an empty string? The block you highlighted should already manipulate an existing header element or remove it, depending on the assignment to headerText being an empty string or not. If there's no headerText, it's expected we don't show the header, and the bug is probably that there isn't header text for this view. I can't quickly see from https://searchfox.org/mozilla-central/source/devtools/client/performance-new/popup/menu-button.jsm.js how the subview is being shown in this case, so not sure what triggers that. Perhaps Florian can help as it looks like he reviewed some of this code?

Actually it seems the value is not an empty string. See attached.

Comment 7

[Itiel]

Attachment: Debugger screenshot

Comment 8

[:Gijs (he/him)]

(In reply to Itiel from comment #6)

Actually it seems the value is not an empty string. See attached.

OK, but then I'm confused. You said the header is not displayed and the code you linked to removes the header from the DOM in certain circumstances. Is that not, in fact, what is happening? What is happening? :-)

Comment 9

[:Gijs (he/him)]

Looks like the problem is just https://searchfox.org/mozilla-central/rev/08362489086b10de96e7a199b267ea5504c01583/browser/themes/shared/customizableui/panelUI-shared.css#1849 ? Am I missing something?

Comment 10

[Itiel]

(In reply to :Gijs (he/him) from comment #8)

(In reply to Itiel from comment #6)

Actually it seems the value is not an empty string. See attached.

OK, but then I'm confused. You said the header is not displayed and the code you linked to removes the header from the DOM in certain circumstances. Is that not, in fact, what is happening? What is happening? :-)

I as understand things, that code is responsible to add a header to a non-overflowed panel if a title (value) is specified; add a header to a panel that don't normally have a header, if it appears in the overflow; and remove a header if it's not needed. And maybe other edgecases.
The Profiler has a custom header (with the info icon and all that), and that confuses that code in a sense that it thinks it already has a header with a value, so it just proceeds to update the textcontent and bails out, hence not creating the header. Not that it removes it, it just doesn't create the header to begin with.
Bug 1577257 regressed this because before, the code looked only at the first child to see if it's a header or not and act accordingly, and since the header "didn't exist" then it proceeded to create the header. Sort of like doing the right thing in the wrong way.

(In reply to :Gijs (he/him) from comment #9)

Looks like the problem is just https://searchfox.org/mozilla-central/rev/08362489086b10de96e7a199b267ea5504c01583/browser/themes/shared/customizableui/panelUI-shared.css#1849 ? Am I missing something?

This is still relevant -- that's the custom header that should be hidden when in the overflow, because a. the info icon shouldn't appear then and b. the back icon should be present. The code I linked before should handle creating a generic header instead.

Am I making sense? :(

Comment 11

[:Gijs (he/him)]

(In reply to Itiel from comment #10)

(In reply to :Gijs (he/him) from comment #8)

(In reply to Itiel from comment #6)

Actually it seems the value is not an empty string. See attached.

OK, but then I'm confused. You said the header is not displayed and the code you linked to removes the header from the DOM in certain circumstances. Is that not, in fact, what is happening? What is happening? :-)

I as understand things, that code is responsible to add a header to a non-overflowed panel if a title (value) is specified; add a header to a panel that don't normally have a header, if it appears in the overflow; and remove a header if it's not needed. And maybe other edgecases.
The Profiler has a custom header (with the info icon and all that), and that confuses that code in a sense that it thinks it already has a header with a value, so it just proceeds to update the textcontent and bails out, hence not creating the header. Not that it removes it, it just doesn't create the header to begin with.
Bug 1577257 regressed this because before, the code looked only at the first child to see if it's a header or not and act accordingly, and since the header "didn't exist" then it proceeded to create the header. Sort of like doing the right thing in the wrong way.

Ahhh. OK, I had missed this link and so from the comments here I was mostly very confused. Sorry.

(In reply to :Gijs (he/him) from comment #9)

Looks like the problem is just https://searchfox.org/mozilla-central/rev/08362489086b10de96e7a199b267ea5504c01583/browser/themes/shared/customizableui/panelUI-shared.css#1849 ? Am I missing something?

This is still relevant -- that's the custom header that should be hidden when in the overflow, because a. the info icon shouldn't appear then and b. the back icon should be present. The code I linked before should handle creating a generic header instead.

Am I making sense? :(

Yes. I'm tempted to suggest that one of the following should happen:

• somehow integrate the fancy (i) button into the default header instead of writing a completely custom one. IIRC there's already an attribute to show the default header when showing the main view? (showheader). And other panels have a similar pattern (e.g. protections panel)
• expose some way of inserting a "standard" back button into the custom header.

I'm not really convinced that the (i) icon shouldn't appear in the subview. Why should it not appear in the overflow menu but appear when clicking the button while it's on the toolbar?

A more hacky solution would be adding some kind of opt-in attribute that would insert and show a "standard" header anyway in this kind of case, but that seems ugly.

Does that help?

Comment 12

[Itiel]

Attachment: Bug 1815130 - Handle custom panel headers better when they are in the overflow panel r?gijs

Comment 13

[Itiel]

(In reply to :Gijs (he/him) from comment #11)

Yes. I'm tempted to suggest that one of the following should happen:

• somehow integrate the fancy (i) button into the default header instead of writing a completely custom one. IIRC there's already an attribute to show the default header when showing the main view? (showheader). And other panels have a similar pattern (e.g. protections panel)
• expose some way of inserting a "standard" back button into the custom header.

A more hacky solution would be adding some kind of opt-in attribute that would insert and show a "standard" header anyway in this kind of case, but that seems ugly.

Does that help?

It does, thanks!

I'm not really convinced that the (i) icon shouldn't appear in the subview. Why should it not appear in the overflow menu but appear when clicking the button while it's on the toolbar?

I only followed tradition, that's how it has been since the inception of the Profiler :)
But fair enough, this is addressed in the patch.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66993f4ad9f2
Handle custom panel headers better when they are in the overflow panel r=Gijs

Comment 15

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/66993f4ad9f2

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:itiel_yn8, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 17

[Itiel]

Hmm I'm conflicted. Gijs, what do you think?

Comment 18

[:Gijs (he/him)]

(In reply to Itiel from comment #17)

Hmm I'm conflicted. Gijs, what do you think?

It's only the third day of beta so I think uplift is safe enough that it could be considered. Equally it's a pre-existing issue and the use of the overflow panel isn't all that common so it may not be worth doing - it's up to you. :-)

Comment 19

[Itiel]

Okay, I'd feel more comfortable if it bakes in Nightly for now. I feel like there could be edge cases I missed.

Comment 20

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue on an affected Nightly build from 2023-02-05, using the STR from Comment 0 on Win 10.
Verified as fixed on Firefox 112.0b9 (20230330182947) and Firefox 113.0a1 (20230403092724) across the following platforms: Win 10 x64, Ubuntu 20.04 and macOS 10.15.