bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
--
major
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

30 Branch
Firefox 30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Reproducible: sometimes
(Assignee)

Comment 2

5 years ago
(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.
(Reporter)

Updated

5 years ago
Severity: minor → trivial
(Assignee)

Updated

5 years ago
Whiteboard: [Australis:P3]
(Assignee)

Comment 3

5 years ago
I can reliably reproduce this with the steps in bug 972267, at least on OS X.
(Assignee)

Comment 4

4 years ago
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
Last Resolved: 4 years ago
status-firefox29: --- → fixed
status-firefox30: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
(Assignee)

Comment 5

4 years ago
Stephen saw this again today. Sigh. :-(
Assignee: gijskruitbosch+bugs → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
status-firefox29: fixed → affected
status-firefox30: fixed → affected
(Assignee)

Updated

4 years ago
Severity: trivial → major
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: Firefox 30 → ---
(Assignee)

Comment 6

4 years ago
I think we can/should just workaround this.
Assignee: nobody → gijskruitbosch+bugs
(Assignee)

Comment 7

4 years ago
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
(Assignee)

Comment 8

4 years ago
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)
(Assignee)

Comment 10

4 years ago
Created attachment 8385536 [details] [diff] [review]
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.
Attachment #8385536 - Flags: review?(MattN+bmo)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 12

4 years ago
w/ nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/de78bf065877
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
(Assignee)

Comment 13

4 years ago
Created attachment 8386211 [details] [diff] [review]
fix negation issue in Australis panel detection CSS,
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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
(Assignee)

Comment 16

4 years ago
Created attachment 8388231 [details] [diff] [review]
Patch for Aurora
(Assignee)

Comment 17

4 years ago
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?
status-firefox30: affected → fixed
Attachment #8388231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
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
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.