972286 - Australis: "<" mark appears for StarUI in PanelUI without subview

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Start Firefox
2. Click StarUI to bookmark
3. Open New Window

4. Right click on StarUI And Move to Menu
5. Open PanelUI

Actual Results:
"<" mark appears

Expexted Results:
No "<" mark

Comment 1

[Alice0775 White]

Reproducible: sometimes

Comment 2

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #0)
> Steps To Reproduce:
> 1. Start Firefox
> 2. Click StarUI to bookmark
> 3. Open New Window
> 
> 4. Right click on StarUI And Move to Menu
> 5. Open PanelUI
> 
> Actual Results:
> "<" mark appears
> 
> Expexted Results:
> No "<" mark

I can't reproduce with these steps on a clean profile. I've seen this, and I know it's a problem, but I don't think this is how it happens. It has something to do with the wrong class for the button somehow being persisted, but I don't yet understand how that happens.

Comment 3

[:Gijs (he/him)]

I can reliably reproduce this with the steps in bug 972267, at least on OS X.

Comment 4

[:Gijs (he/him)]

I believe this is fixed by the fixes for bug 972267 and bug 969780. Please reopen if you can still reproduce on builds after those landed.

Comment 5

[:Gijs (he/him)]

Stephen saw this again today. Sigh. :-(

Comment 6

[:Gijs (he/him)]

I think we can/should just workaround this.

Comment 7

[:Gijs (he/him)]

STR:

1. open firefox
2. move bookmarks widget to panel
3. click bookmarks widget in panel
4. (*without closing the panel*) open new window (with shortcut)
5. open panel in new window
(now it looks off already)
6. move to toolbar

Comment 8

[:Gijs (he/him)]

Marco, I recall that we talked about this issue and you said the class persistence had been added because of the styling of buttons. However, it seems that in the case of the bookmarks menu button it was introduced in bug 748894, but the bug has no mention of why the attribute was added.

What are the potential downsides of just removing the attribute? :-)

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #8)
> it was introduced in bug
> 748894, but the bug has no mention of why the attribute was added.

it's exactly for the reason I expressed, when you start the browser the class persistence allows the button to be styled correctly immediately, while without it we may load the wrong style and then change it to the right one once the BookmarkingUI code runs. That has a couple downside effects: 1. flicker, 2. startup work. If we are already doing startup work for the customization support, as I think we are, the latter issue is basically no more an issue. flicker may be bad or ignorable, it depends on the situation.

Comment 10

[:Gijs (he/him)]

Attachment: switch to attributes instead of classes for overflowedItem and for the Australis panel subview anchor,

Casual testing and our mochitest suite say that this patch is OK. It should fix the issue by switching to attributes instead of classes. It may require some docs work.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8385536 [details] [diff] [review]
switch to attributes instead of classes for overflowedItem and for the Australis panel subview anchor,

Review of attachment 8385536 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser_884402_customize_from_overflow.js
@@ +29,5 @@
>    let contextMenu = document.getElementById("toolbar-context-menu");
>    let shownContextPromise = contextMenuShown(contextMenu);
>    let homeButton = document.getElementById("home-button");
>    ok(homeButton, "home-button was found");
> +  ok(homeButton.getAttribute("overflowedItem") == "true", "Home button is overflowing");

Nit: switch to |is|

@@ +73,5 @@
>    let homeButtonPlacement = CustomizableUI.getPlacementOfWidget("home-button");
>    ok(homeButtonPlacement, "Home button should still have a placement");
>    is(homeButtonPlacement && homeButtonPlacement.area, "nav-bar", "Home button should be back in the navbar now");
>  
> +  ok(homeButton.getAttribute("overflowedItem") == "true", "Home button should still be overflowed");

ditto

::: browser/components/customizableui/test/browser_914138_widget_API_overflowable_toolbar.js
@@ +44,5 @@
>    window.resizeTo(originalWindowWidth, window.outerHeight);
>    yield waitForCondition(() => !navbar.hasAttribute("overflowing"));
>    ok(!navbar.hasAttribute("overflowing"), "Should not have an overflowing toolbar.");
>    ok(navbar.querySelector("#" + kHomeBtn), "Home button should be in the navbar");
> +  ok(homeBtnNode && !homeBtnNode.getAttribute("overflowedItem"), "Home button should no longer have overflowedItem attribute");

It seems like this should either check that it's not "true" for consistency or use hasAttribute. I also think it should really be split into two checks but since many others aren't it's okay to leave it.

@@ +49,4 @@
>    ok(!overflowList.querySelector("#" + kHomeBtn), "Home button should no longer be overflowing");
>    ok(navbar.querySelector("#" + kTestBtn1), "Test button should be in the navbar");
>    ok(!overflowList.querySelector("#" + kTestBtn1), "Test button should no longer be overflowing");
> +  ok(newButtonNode && !newButtonNode.hasAttribute("overflowedItem"), "New button should no longer have overflowedItem attribute");

Likewise, it seems like this should use getAttribute and compare to "true" for consistency and also be split.

Comment 12

[:Gijs (he/him)]

w/ nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/de78bf065877

Comment 13

[:Gijs (he/him)]

Attachment: fix negation issue in Australis panel detection CSS,

Comment 14

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/830a6dd88d1b

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de78bf065877
https://hg.mozilla.org/mozilla-central/rev/830a6dd88d1b

Comment 16

[:Gijs (he/him)]

Attachment: Patch for Aurora

Comment 17

[:Gijs (he/him)]

Comment on attachment 8388231 [details] [diff] [review]
Patch for Aurora

Coalesced these patches, carrying forward r+

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: class-persisting node end up in weird situations
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low, except for add-on risk, which is probably still low because (a) add-ons shouldn't be checking for these classes/attributes themselves, and even if they were, there's precious few add-ons that have been updated for Australis.
String or IDL/UUID changes made by this patch: none

Comment 18

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45f54346f9f8

Comment 19

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on old Nightly (2014-03-03), verified that the issue is fixed on Firefox 29RC and latest Aurora using Windows 7 64bit, Mac OS X 10.9.2 and Ubuntu 14.04 32bit.