Closed Bug 972286 Opened 6 years ago Closed 6 years ago

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

Categories

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

30 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: alice0775, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(3 files)

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
Reproducible: sometimes
(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.
Severity: minor → trivial
Whiteboard: [Australis:P3]
I can reliably reproduce this with the steps in bug 972267, at least on OS X.
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee: nobody → gijskruitbosch+bugs
Stephen saw this again today. Sigh. :-(
Assignee: gijskruitbosch+bugs → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Severity: trivial → major
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: Firefox 30 → ---
I think we can/should just workaround this.
Assignee: nobody → gijskruitbosch+bugs
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
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? :-)
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
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.
Attachment #8385536 - Flags: review?(MattN+bmo)
Blocks: 971956
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.
Attachment #8385536 - Flags: review?(MattN+bmo) → review+
w/ nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/de78bf065877
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Attachment #8386211 - Flags: review?(MattN+bmo)
Attachment #8386211 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/de78bf065877
https://hg.mozilla.org/mozilla-central/rev/830a6dd88d1b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Attached patch Patch for AuroraSplinter Review
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
Attachment #8388231 - Flags: review+
Attachment #8388231 - Flags: approval-mozilla-aurora?
Attachment #8388231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.