1698799 - uBlock Origin icon is no longer displayed on the toolbar when opening/closing the Customize page

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Alex Cornestean]

Attachment: 2021-03-16_15h17_32.mp4

Beta (87.0/20210315170302)
Release (86.0.1/20210310152336)

[Steps to reproduce]:

1. Install uBlock Origin. Notice that the add-on icon is added to the toolbar
2. Open a new tab
3. Right click on an empty space on the toolbar and select the “Customize...” option. The “Customize Firefox/Nightly” page is displayed.
4. Drag the add-on icon from the toolbar to the item list
5. Close the “Customize Firefox/Nightly” page
6. Open the “Customize Firefox/Nightly” page again by performing Step 3
7. Observe that the uBlock Origin add-on icon from the item list is no longer displayed (only the name of the add-on is visible)
8. Drag the add-on from the item list back to the toolbar (Notice that when hovering over the add-on on the toolbar, the tooltip tag is displayed)
9. Close the “Customize Firefox/Nightly” page
10. Observe that the add-on icon is still not visible in the position where it was placed in Step 8. Hovering over the add-on icon position will highlight the area and the tooltip tag will be displayed
11. Access https://www.youtube.com/ from the same tab
12. Notice that the add-on icon is visible again

[Expected results]:
The uBlock Origin icon should be visible in all instances.

[Actual results]:
The uBlock Origin icon is no longer displayed when the icon is dragged to the Customize toolbar item list and the Customize page is closed and reopened. Dragging the “invisible” icon back to the toolbar and closing the Customize page will keep the invisible state of the add-on icon.

The icon becomes visible again once a page where the add-on becomes active is accessed. Hence the 11th step above.

It seems the issue occurs only on pages where the add-on is inactive i.e privileged Firefox pages (Add-ons Manager page, Options page, New Tab page etc.)

For further details, please also see the attached video.

Note: uBlock Origin appears to be the only affected add-on I could find at the moment.

Comment 1

[:Gijs (he/him)]

Is this a regression, and if so, when did it break? Does this happen irrespective of proton being enabled?

Comment 2

[Alex Cornestean]

A bisection revealed the following:

59:01.99 INFO: Last good revision: fed64af04da307da62a504be6d0b5554afb18566
59:01.99 INFO: First bad revision: f5f1227caf70f09bec9d9d5764783b6be7b5a5ed
59:01.99 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fed64af04da307da62a504be6d0b5554afb18566&tochange=f5f1227caf70f09bec9d9d5764783b6be7b5a5ed

The regressor seems to be: https://bugzilla.mozilla.org/show_bug.cgi?id=1558635

The issue also occurs with Proton enabled.

Comment 3

[:Gijs (he/him)]

Luca, do you know off-hand why this would be? I imagine I should have updated some extension code in bug 1558635 so that webextension actions are initialized when customize mode is entered, or something?

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Luca, do you know off-hand why this would be? I imagine I should have updated some extension code in bug 1558635 so that webextension actions are initialized when customize mode is entered, or something?

I don't recall anything special that we were doing for the extensions browserAction toolbar buttons specifically for customize mode.

I briefly tried to reproduce the issue (using the STR in comment 0) in today nightly and on Ubuntu's Firefox 87 build but I wasn't able to reproduce it (the ublock icon was still visible when I went back to the customize mode for the step 7 of the STR).

I'm wondering if there is something missing from STR or if it is not reproducible on Linux (or because of some other difference, eg. some about:config pref?).

Hi
Alex,
from the screencast attached and the fact that you did run mozregress I'm guessing that the issue is reproducing consistently for you on windows, does it also reproduce consistently on Linux for you?

Comment 5

[Alex Cornestean]

Hello Luca,

The issue is reproducing consistently on Windows, yes.

As per your request, I tested the latest Release (87.0/20210318103112), Beta (88.0b5/20210330185720) and Nightly (89.0a1/20210330215136) on Ubuntu 16.04 LTS. The issue is consistently reproducible on Linux, as well.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks a lot Alex,
and my apologies for not getting back to this sooner (it was planning to get back to this to try again to reproduce it and take a better look, but getting back to this took me a bit longer than I wanted).

Anyway, I've been finally able to reproduce it consistently ([1]).

After successfully reproducing the issue, I inspected the ublock toolbarbutton node using the BrowserToolbox when it does have and when it does missing the icon, but their styles seems still be exactly as expected in both cases, in particular:

• on the browserAction toolbarbutton element (which is being wrapped into a toolbarpaletteitem to be added to the customizemode palette) there is still the expected style property, which includes a number of --webextensions-* css variables which provide the moz-extension urls for the variants of the ublock browserAction icon (dark, light etc. which are all set to the same moz-extension url in the ublock case)
• then the extensions browserAction toolbarbutton elements have applied rules that are applying one of the urls from those CSS variable as the value to use in list-image-style (these rules are defined in browser.css here)

Another interesting detail that I did notice while inspecting it from the Browser Toolbox is that, when the ublock icon isn't rendered, if I toggle any of the styling (by toggling it off and then on again from the DevTools Inspector CSS Rules sidebar panel), the icon gets suddenly rendered, which seems to confirm that it should have been rendered with the styles the toolbarbutton node already have.

I think there is something odd at a CSS styles level that is probably going on, but I'm not entirely sure how Bug 1558635 affected that (may be something related to the fact that we started to use a template element), we may have triggered some pre-existing bugs in some corner cases? I could dig more, but I think that someone with more direct knowledge on that layer may be able to help us to more quickly find the actual root cause.

If we haven't figure out a proper fix and we want to apply a short term workaround as a first step (while we still look for a more proper fix), with the following small change ([2]) I was unable to reproduce the issue anymore with the STR I was using (and the icon was still visible again as expected):

diff --git a/browser/components/customizableui/CustomizeMode.jsm b/browser/components/customizableui/CustomizeMode.jsm
--- a/browser/components/customizableui/CustomizeMode.jsm
+++ b/browser/components/customizableui/CustomizeMode.jsm
@@ -973,6 +973,14 @@ CustomizeMode.prototype = {
       aNode = aNode.parentNode.replaceChild(wrapper, aNode);
     }
     wrapper.appendChild(aNode);
+    // NOTE: force refresh style on the webextensions browserAction wrapped node
+    // to make sure the icon will be rendered as expected (See Bug 1698799).
+    if (aNode.classList.contains("webextension-browser-action")) {
+      aNode.style.setProperty(
+        "list-image-style",
+        aNode.style.getPropertyValue("list-image-style")
+      );
+    }
     return wrapper;
   },

---

[1]: to reproduce it consistently more easily I had to always run the STR from a new empty profile and using the following reduced STR:

• create a new profile
• install ublock
• enter customize mode
• drag ublock icon in the customize mode palette
  • the ublock icon is still rendered as usually at this point
• switch to another tab (without closing the customize mode)
• switch back to the customize mode tab
  • the ublock icon is not rendered <===== bug reproduced

[2]: this is just forcing the existing style to be applied, similar to what toggling something CSS from the DevTools inspector was also doing, that style property is actually an empty string on the browserAction toolbarbutton element and so the fix is definitely a side-effect due to forcing the styles to be refreshed.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hey Gijs,
I did look into this again yesterday and in comment 6 there are some additional details about the issue that I collect and that I think that may help us to identify the issue.

Let me know wdyt.
(and if I can help more to investigate this bug and/or if we want to proceed with the workaround as a first step, in that case I may take a look into how to add a regression test).

Comment 8

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #7)

Hey Gijs,
I did look into this again yesterday and in comment 6 there are some additional details about the issue that I collect and that I think that may help us to identify the issue.

Thanks, this is great!

I think there's an issue here with how layout / style is responding to a node with CSS properties set (maybe specific to custom properties / variables?) moving from being a template node child to being in the main/normal DOM. If just invalidating style on the node by toggling some other property on/off in devtools is causing a change in rendering, and doing what should be an idempotent assignment like:

+      aNode.style.setProperty(
+        "list-image-style",
+        aNode.style.getPropertyValue("list-image-style")
+      );

also fixes it, then I'm reasonably confident this is something for the layout/style people to figure out, so passing to Emilio as a first step. ;-)

if we want to proceed with the workaround as a first step, in that case I may take a look into how to add a regression test

This regressed in 78 and wasn't found until 86, and 88 just shipped, so getting a workaround into 89 doesn't feel urgent - let's just fix it properly, and if that turns out to be risky we can either re-evaluate or just decide fixing in 90 is fine (for which we still have about 5 weeks). It also sounds like the CSSOM and DOM is all correct - but the rendered result isn't, so I'm not sure that a regression test at the frontend level is even possible.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I assume list-image-style in the patch above is list-style-image right? It's not possible that does fix it because that does nothing anyways, right? The style attribute of the ublock toolbarbutton is:

    --webextension-menupanel-image: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_16.png");
    --webextension-menupanel-image-light: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_16.png");
    --webextension-menupanel-image-dark: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_16.png");
    --webextension-menupanel-image-2x: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_32.png");
    --webextension-menupanel-image-2x-light: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_32.png");
    --webextension-menupanel-image-2x-dark: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_32.png");
    --webextension-toolbar-image: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_16.png");
    --webextension-toolbar-image-light: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_16.png");
    --webextension-toolbar-image-dark: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_16.png");
    --webextension-toolbar-image-2x: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_32.png");
    --webextension-toolbar-image-2x-light: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_32.png");
    --webextension-toolbar-image-2x-dark: url("moz-extension://f19598f8-cfe5-4715-bbd6-9f84b0068b94/img/icon_32.png");

According to devtools.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

I assume list-image-style in the patch above is list-style-image right? It's not possible that does fix it because that does nothing anyways, right? The style attribute of the ublock toolbarbutton is:

yes that was definitely a typo, sorry

Nonetheless it seems that even with a typo that does still force the icon to be rendered, which I assume is just side-effect for setting any property on aNode.style, which seems to match what was happening if I was toggling any CSS property applied to the element from the devtools panel, from what I recall it didn't matter if that property was related or not to the icon that wasn't rendered.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

This regressed in 78 and wasn't found until 86, and 88 just shipped, so getting a workaround into 89 doesn't feel urgent - let's just fix it properly, and if that turns out to be risky we can either re-evaluate or just decide fixing in 90 is fine (for which we still have about 5 weeks).

I totally agree, that would be also my preference. I'll keep an eye on this issue updates because at this point I'm too curious, but if feel free to needinfo me if there is any additional help I can provide and I missed to notice it.

It also sounds like the CSSOM and DOM is all correct - but the rendered result isn't, so I'm not sure that a regression test at the frontend level is even possible.

yeah, I assumed it was tricky (and I didn't like to workaround it until we at least tried to get to the bottom of it) and so I didn't even tried (I was thinking that maybe I could have tried to screenshot the element as drawn and be sure is not empty, but I didn't even to check if that would catch the issue and how the test case would look like).

Comment 12

[Emilio Cobos Álvarez (:emilio)]

The devtools panel does something crazy more expensive, and pretty sure that with the typo this is 100% a no-op. But anyhow I could repro on a clean profile so looking.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Ok, I take it back. It's not a no-op, because we somehow get into a state where we have no style declaration but have a style attribute, wat?

(rr) p $35->List((FILE*)stderr, 0)
XUL* toolbarbutton@0x7fe6fab95790 class="toolbarbutton-1 chromeclass-toolbar-additional panel-no-padding webextension-browser-action" id="ublock0_raymondhill_net-browser-action" widget-id="ublock0_raymondhill_net-browser-action" widget-type="view" removable="true" overflows="true" label="uBlock Origin" tooltiptext="uBlock Origin" badged="true" constrain-size="true" data-extensionid="[e-mail]" badgeStyle="background-color: rgba(102, 102, 102, 1); color: rgba(255, 255, 255, 1)" style="
      
        --webextension-menupanel-image: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_16-off.png\");
        --webextension-menupanel-image-light: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_16-off.png\");
        --webextension-menupanel-image-dark: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_16-off.png\");
      
      
        --webextension-menupanel-image-2x: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_32-off.png\");
        --webextension-menupanel-image-2x-light: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_32-off.png\");
        --webextension-menupanel-image-2x-dark: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_32-off.png\");
      
      
        --webextension-toolbar-image: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_16-off.png\");
        --webextension-toolbar-image-light: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_16-off.png\");
        --webextension-toolbar-image-dark: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_16-off.png\");
      
      
        --webextension-toolbar-image-2x: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_32-off.png\");
        --webextension-toolbar-image-2x-light: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_32-off.png\");
        --webextension-toolbar-image-2x-dark: url(\"moz-extension://c83a2958-0d84-4420-86c9-958e915f3271/img/icon_32-off.png\");
      
    " state=[20800000] flags=[00000002] primaryframe=(nil) refcount=6<
  XUL* stack@0x7fe6fabadb80 class="toolbarbutton-badge-stack" state=[20000000] flags=[00000000] primaryframe=(nil) refcount=5<
    XUL* image@0x7fe6fabadee0 class="toolbarbutton-icon" label="uBlock Origin" state=[20000000] flags=[00000000] primaryframe=(nil) refcount=3<>
    html:label@0x7fe6fabad160 class="toolbarbutton-badge" style="background-color: rgba(102, 102, 102, 1); color: rgba(255, 255, 255, 1)" state=[100020800000] flags=[00008080] primaryframe=(nil) refcount=2<>
  >
  XUL* label@0x7fe6fabad670 class="toolbarbutton-text" crop="right" flex="1" value="uBlock Origin" state=[20800000] flags=[00000002] primaryframe=(nil) refcount=3<>
>
$38 = void
(rr) p $35->mSlots->mStyle
$39 = [(nsICSSDeclaration *) 0x0]

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Hmm, nevermind, I'm confused there. mSlots->mStyle is not quite what I should be looking at.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

But we style don't have a parsed style declaration, that smells.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

The attribute gets set here:

(rr) call DumpJSStack()
0 callback("20372.587539") ["chrome://browser/content/parent/ext-browserAction.js":582:11]

The owner document of the relevant item is loaded as data:

(rr) $3->mNodeInfo->mDocument->mLoadedAsData
true

So this condition avoids parsing it into a declaration block. The GetExistingStyle() returns true only if we've already accessed the style via CSSOM, so that's why the typo above still "fixes" it.

So the question is why are we setting the style attribute on a data doc.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Or why when we get bound to a real document we don't reparse it, but that's easy: !IsXULElement().

Comment 18

[Emilio Cobos Álvarez (:emilio)]

That is from bug 564863. Bug 564863 comment 17 seems to imply this was all preserving behavior somehow.

Comment 19

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

So the question is why are we setting the style attribute on a data doc.

I'm not even 100% sure what "data doc" is, but could this be relevant to the fact that the button gets moved into the content of the html:template tag that we use to parent all our unused toolbar widgets? (at step 5 in the STR in comment #0 )

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1698799 - Reparse style attribute on bind for XUL like we do for all other elements. r=smaug

UL is different. According to bug 564863 comment 17:

nsXULElement used to not call its superclass BindToTree to avoid this
reparsing.

Presumably for perf reasons? But since then we've added the MayHaveStyle
flag, which should make it fast. I don't see any reason this special
case makes any sense.

I could write a test for this patch (parse a XUL element in a data doc
with a style attribute, import it into a regular document), but honestly
this is well covered by WPT for all other elements, and this is removing
code, so I don't feel it buys us much.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

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

I'm not even 100% sure what "data doc" is, but could this be relevant to the fact that the button gets moved into the content of the html:template tag that we use to parent all our unused toolbar widgets? (at step 5 in the STR in comment #0 )

Yes, totally. The html template element content lives in a special, inert document. If the style attribute gets set while the node is bound to that document it doesn't get parsed instantly (this behavior matches HTML etc). But since for XUL we don't reparse when we move it to a different doc, well, sadness ensues...

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

The attribute gets set here:

(rr) call DumpJSStack()
0 callback("20372.587539") ["chrome://browser/content/parent/ext-browserAction.js":582:11]

Ah, That the code that is called when the extension is updating its browserAction icon,
and now I see also what was making ublock "special" and able to trigger this bug:

• the extension does change the browserAction icon to the "on" (colored one) to the "off" (greyed one) when Firefox is switching between a tab that is accessible to the extension (a regular webpage) and one that is not accessible to the extension (e.g. about pages or the customize mode as in this case)

• when the user enters the customize mode, ublock does react to the tab switch by updating its browserAction icon, which sets the style attribute on the toolbar button when the node is inside the inert data document and so we were not parsing it (and I guess that we have already invalidated the previous parsed style?)

• by touching the style properties (really any off them), I was forcing the style attribute to be parsed (which was kind of my guess, I was sure I was triggering just a side-effect) and the icon was rendered again

Thanks a lot Emilio,
I'm really glad we identified the actual underlying issue and prepared a proper fix.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/675e164528d0
Reparse style attribute on bind for XUL like we do for all other elements. r=smaug

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/675e164528d0

Comment 25

[Alex Cornestean]

Verified the fix on the latest Nightly (90.0a1/20210422213157) under Windows 10 x64 and Ubuntu 16.04 LTS.

The add-on icon is always displayed in the “Customize Firefox/Nightly” page item list when opening/closing the page and switching between the page and another tab, confirming the fix.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta?

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9217518 [details]
Bug 1698799 - Reparse style attribute on bind for XUL like we do for all other elements. r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: Comment 0
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Comment 0, comment 6 also works.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Removes XUL special case
• String changes made/needed: none

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9217518 [details]
Bug 1698799 - Reparse style attribute on bind for XUL like we do for all other elements. r=smaug

Approved for 89.0b13.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Alex Cornestean]

Verified the fix on the latest Beta (89.0b13/20210516190025) under Windows 10 x64 and Ubuntu 16.04 LTS.

The add-on icon is always displayed in the “Customize Firefox/Nightly” page item list when opening/closing the page and switching between the page and another tab, confirming the fix.