Affected versions

• 96.0a1 (20211101215926)
• 95.0b1 (20211101163752)

Affected platforms

• Windows 10 x64
• Ubuntu 21.04
• macOS 10.15

Steps to reproduce

1. Open Firefox and Customize Menu.
2. Add Profiler to the Overflow menu.

Expected result

• Profiler has a name after the icon.

Actual result

• No name after the profiler icon inside overflow menu.

Regression range

• Last good revision: dc2ca800da7c1cdfd2e1d7705f7bcf53b1bc6e5c
  First bad revision: 86b88ba429a395af57cba3c2cb59304a49225df3
  Pushlog:
  https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=dc2ca800da7c1cdfd2e1d7705f7bcf53b1bc6e5c&tochange=86b88ba429a395af57cba3c2cb59304a49225df3

Notes

• Attached a screenshot.

I inspected with the browser toolbox, and I think we're doing the right thing in the profiler code, so I'm thinking of a bug in the custom components we're using. Indeed we're using a rarely used feature of this custom components to have this split in 2 buttons, and I believe that the use of this feature in the overflow menu might be broken. I'll look at this closer later today.

Hey Gijs, I'd appreciate some pointers about this issue. Fluent sets the label after localization. From inspecting the node, I see the label attribute is properly set, but the value isn't reflected inside the component. As a reminder this is a "button-and-view" widget so it's possible this case was missed when this widget type was implemented.

Thanks!

(In reply to Julien Wajsberg [:julienw] from comment #2)

Hey Gijs, I'd appreciate some pointers about this issue. Fluent sets the label after localization. From inspecting the node, I see the label attribute is properly set, but the value isn't reflected inside the component. As a reminder this is a "button-and-view" widget so it's possible this case was missed when this widget type was implemented.

Please can you provide more detail? On what node is the label attribute set, and where is it / is it not reflected? What code would you expect to take care of that reflection? I'm afraid I don't have time right now to try to reproduce this myself.

As a complete hunch, at what point in time is the label attribute set, and does the button have a visible label when it is in the customize mode "palette" (not put in any of the toolbars) ? Is it possible we're not synchronizing to the title attribute of the toolbarpaletteitem, or something?

Just want to point out that regression range pushlog contained two bugs and maybe this is not the correct regressor that I assumed it is.

(In reply to Alexandru Trif, QA [:atrif] from comment #4)

Just want to point out that regression range pushlog contained two bugs and maybe this is not the correct regressor that I assumed it is.

Yes, this is likely the right one :-) Thanks for finding the regression range in the first place!

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

(In reply to Julien Wajsberg [:julienw] from comment #2)

Hey Gijs, I'd appreciate some pointers about this issue. Fluent sets the label after localization. From inspecting the node, I see the label attribute is properly set, but the value isn't reflected inside the component. As a reminder this is a "button-and-view" widget so it's possible this case was missed when this widget type was implemented.

Please can you provide more detail? On what node is the label attribute set, and where is it / is it not reflected? What code would you expect to take care of that reflection? I'm afraid I don't have time right now to try to reproduce this myself.

I'll detail what I've seen when inspecting with the Browser Toolbox.

For other buttons, this looks like this (simplified):

<toolbarbutton data-l10n-id="navbar-downloads" removable="true" overflows="false" tooltip="dynamic-shortcut-tooltip" indicator="true" label="Downloads" cui-areatype="menu-panel" cui-anchorid="nav-bar-overflow-button" context="customizationPanelItemContextMenu">
  <stack class="toolbarbutton-badge-stack">...</stack>
  <label class="toolbarbutton-text" crop="right" flex="1" value="Downloads"/>
</toolbarbutton>

We see that the toolbarbutton's label atribute is properly copied to the label's value attribute.

For the profiler button, this looks like this (simplified):

<toolbaritem id="profiler-button" widget-id="profiler-button" widget-type="button-and-view" removable="true" overflows="true" data-l10n-id="profiler-popup-button-idle" label="Profiler" tooltiptext="Record a performance profile" cui-areatype="menu-panel" cui-anchorid="nav-bar-overflow-button" context="customizationPanelItemContextMenu">
  <toolbarbutton class="toolbarbutton-1 subviewbutton-nav" id="profiler-button-button">
    <image class="toolbarbutton-icon"/>
    <label class="toolbarbutton-text" crop="right" flex="1"/>
  </toolbarbutton>
  <toolbarbutton id="profiler-button-dropmarker" class="toolbarbutton-1 toolbarbutton-combined-buttons-dropmarker">...</toolbarbutton></toolbaritem>

Here we see that the toolbaritem's label atribute isn't properly copied to the toolbarbutton's label attribute, and as a result to the label's value attribute.

As a reference, this is what I see in beta:

<toolbaritem id="profiler-button" widget-id="profiler-button" widget-type="button-and-view" removable="true" overflows="true" label="Profiler" tooltiptext="Record a performance profile" context="customizationPanelItemContextMenu" cui-areatype="menu-panel" cui-anchorid="nav-bar-overflow-button">
  <toolbarbutton class="toolbarbutton-1 subviewbutton-nav" id="profiler-button-button" label="Profiler" tooltiptext="Record a performance profile">
    <image class="toolbarbutton-icon" label="Profiler"/>
    <label class="toolbarbutton-text" crop="right" flex="1" value="Profiler"/>
  </toolbarbutton>
  <toolbarbutton id="profiler-button-dropmarker" class="toolbarbutton-1 toolbarbutton-combined-buttons-dropmarker">...</toolbarbutton>
</toolbaritem>

Here the inner toolbarbutton inherits properly all values and as a result the inner label too.

The difference is that now we use Fluent (with the l10nId property when configuring the widget), but before we were using the l10n properties.

As a complete hunch, at what point in time is the label attribute set,

I believe it's set by the Fluent library after data-l10n-id is set and the element added to the DOM.

and does the button have a visible label when it is in the customize mode "palette" (not put in any of the toolbars) ?

Yes

Is it possible we're not synchronizing to the title attribute of the toolbarpaletteitem, or something?

I believe this is a synchronization issue indeed :-)

I think I mostly need pointers to the implementation of toolbaritem (especially) and toolbarbutton, I can't find where these custom components are defined and implemented. Then I can probably fix the problem myself :-)

Thanks!

The attribute handling for toolbar buttons is at https://searchfox.org/mozilla-central/rev/88cd13997fb0747cdcd78638fc762ff2d75e1fc5/toolkit/content/widgets/toolbarbutton.js#97-111,113-114,117-119 .

There is no implementation part for toolbaritem; it's meant to just be a container without any logic. Probably the data-l10n-id should (also?) be set on the button directly.

I see, thanks for the comment. I was looking for some special handling of toolbaritem but couldn't find it, but now I see this is in toolbarbutton instead.

In the "old way", there is indeed a special handling:

        node.setAttribute("label", this.getLocalizedProperty(aWidget, "label"));
        if (button != node) {
          button.setAttribute("label", node.getAttribute("label"));
        }

button is different than node in the case of button-with-view. In this case we add the label for both nodes.
Same thing goes for the tooltip.

But we don't have such a special handling for the l10nId option.

I'm trying to add this.
I'll add this. Thanks for the pointers!

https://hg.mozilla.org/mozilla-central/rev/f7f1cc665cce

Julien, do you want to request an uplift to beta or should this patch ship in 96? Thanks

Verified fixed with 96.0a1 (20211109093712) on Windows 10x64, macOS 10.15 and Ubuntu 21.04. The Profiler icon has now a name when placed inside Customize Menu.

Comment on attachment 9249240 [details]
Bug 1738871 - [CustomizableUI] Support the button-and-view custom widget type in the overflow menu when Fluent is used r=gijs

Beta/Release Uplift Approval Request

• User impact if declined: No label when the profiler icon is in the overflow menu
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We're adding a new attribute and not removing anything, therefore this is low risk.
• String changes made/needed: none, this is reusing a previous string

Comment on attachment 9249240 [details]
Bug 1738871 - [CustomizableUI] Support the button-and-view custom widget type in the overflow menu when Fluent is used r=gijs

Low risk, approved for 95 beta 6, thanks.

https://hg.mozilla.org/releases/mozilla-beta/rev/b3eca1dfbf68

Profiler name is correctly displayed with Firefox 95.0b6 on Windows 10x64, macOS 10.15 and Ubuntu 21.04.