Closed
Bug 972286
Opened 11 years ago
Closed 11 years ago
Australis: "<" mark appears for StarUI in PanelUI without subview
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
VERIFIED
FIXED
Firefox 30
People
(Reporter: alice0775, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:P3])
Attachments
(3 files)
31.87 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
31.81 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Reproducible: sometimes
Assignee | ||
Comment 2•11 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•11 years ago
|
Severity: minor → trivial
Assignee | ||
Updated•11 years ago
|
Whiteboard: [Australis:P3]
Assignee | ||
Comment 3•11 years ago
|
||
I can reliably reproduce this with the steps in bug 972267, at least on OS X.
Assignee | ||
Comment 4•11 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
Closed: 11 years ago
status-firefox29:
--- → fixed
status-firefox30:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Assignee | ||
Comment 5•11 years ago
|
||
Stephen saw this again today. Sigh. :-(
Assignee: gijskruitbosch+bugs → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Severity: trivial → major
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: Firefox 30 → ---
Assignee | ||
Comment 6•11 years ago
|
||
I think we can/should just workaround this.
Assignee: nobody → gijskruitbosch+bugs
Assignee | ||
Comment 7•11 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•11 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)
Comment 9•11 years ago
|
||
(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•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
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•11 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•11 years ago
|
||
Attachment #8386211 -
Flags: review?(MattN+bmo)
Updated•11 years ago
|
Attachment #8386211 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de78bf065877
https://hg.mozilla.org/mozilla-central/rev/830a6dd88d1b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Comment 17•11 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?
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8388231 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
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.
Description
•