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.

The Bookmarks Toolbar Items doesn't appear as a normal menu panel button in new windows

VERIFIED FIXED in Firefox 29

Status

()

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

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P3-])

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 8371022 [details]
Screenshot of bug

STR:
Enter customization mode
Show the bookmarks toolbar 
Drag the Bookmarks Toolbar Item to the menu panel
Exit customization
Open the menu panel
Notice that the button is a normal column wide and matches the display of other buttons
Open a new window
Open the menu panel on the new window

ER:
The toolbar item looks like it did in the previous window

AR:

The toolbar item is wide and takes up all three columns.

Comment 1

5 years ago
This is now broken even in the first window. :-\

I thought we had a bug for this already but I can't find it.
Whiteboard: [Australis:P4] → [Australis:P3-]
It still works for me in the first window (that is, if I make the customization and then open the panel). Restarting probably reproduces the bug the same way that opening a new window will.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Created attachment 8376458 [details] [diff] [review]
Patch
Attachment #8376458 - Flags: review?(gijskruitbosch+bugs)

Comment 4

5 years ago
Comment on attachment 8376458 [details] [diff] [review]
Patch

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

This isn't right. There's code in the places code that is meant to deal with this correctly. Inasmuch as it doesn't, that's the code that needs fixing. browser-places.js, I think.
Attachment #8376458 - Flags: review?(gijskruitbosch+bugs) → review-
Created attachment 8377751 [details] [diff] [review]
Patch v2
Attachment #8376458 - Attachment is obsolete: true
Attachment #8377751 - Flags: review?(gijskruitbosch+bugs)

Comment 6

5 years ago
Comment on attachment 8377751 [details] [diff] [review]
Patch v2

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

I guess I get the idea... what I worry about is how we're adding code specifically for the builtin widgets. I guess add-on-provided widgets could use their onCreated/onBuild handlers to add a popupshowing handler to the PanelUI if they're in there... but that seems hacky. Wonder if we could improve that...

Separately, we shouldn't initialize the places view for the toolbar if we're in the panel. Just calling .customizeChange() will do, I think. And that's why this gets an f+ rather than r+
Attachment #8377751 - Flags: review?(gijskruitbosch+bugs) → feedback+
Created attachment 8378634 [details] [diff] [review]
Patch v3
Attachment #8377751 - Attachment is obsolete: true
Attachment #8378634 - Flags: review?(gijskruitbosch+bugs)

Comment 8

5 years ago
Comment on attachment 8378634 [details] [diff] [review]
Patch v3

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

r=me without the changes to browser-places.js, which seem unnecessary now, AFAICT. Both the automated test and a manual test seem to indicate that stuff works without it.

If we need those changes, please explain why and re-request review.
Attachment #8378634 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 9

4 years ago
Shouldn't this land ? ;)
(In reply to Guillaume C. [:ge3k0s] from comment #9)
> Shouldn't this land ? ;)

Yes, it should! Thanks for the reminder :)

https://hg.mozilla.org/integration/fx-team/rev/c10115ce8c0d
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
status-firefox29: --- → affected
status-firefox30: --- → affected
Depends on: 977804
Bug 977804 is happening too frequently to leave this in. Backed out.
https://hg.mozilla.org/mozilla-central/rev/d5bd7031fb3e
Flags: in-testsuite+
Created attachment 8383956 [details] [diff] [review]
Patch v2

This fixes the test failure on Linux. The problem was happening because Panel.show() was being called before window initialization had finished, meaning that the getter for PanelUI.panel didn't exist yet.
Attachment #8378634 - Attachment is obsolete: true
Attachment #8383956 - Flags: review?(mconley)
Attachment #8383956 - Flags: review?(mconley) → review?(gijskruitbosch+bugs)

Comment 13

4 years ago
Comment on attachment 8383956 [details] [diff] [review]
Patch v2

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

LGTM. I recently noticed that if you move the item to the palette, it also looks weirdly stretched (at least on OSX retina). Not sure if this patch would fix that too and/or if that's another issue...
Attachment #8383956 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 14

4 years ago
(In reply to :Gijs Kruitbosch from comment #13)
> I recently noticed that if you move the item to the palette, it also
> looks weirdly stretched (at least on OSX retina). Not sure if this patch
> would fix that too and/or if that's another issue...

I don't know if this patch fixes it but it's bug 971948
(In reply to Guillaume C. [:ge3k0s] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > I recently noticed that if you move the item to the palette, it also
> > looks weirdly stretched (at least on OSX retina). Not sure if this patch
> > would fix that too and/or if that's another issue...
> 
> I don't know if this patch fixes it but it's bug 971948

Yeah, that's bug 971948. This patch isn't intended to fix that issue.
https://hg.mozilla.org/mozilla-central/rev/13e905c91317
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment on attachment 8383956 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): issue that has been present since beginning of australis
User impact if declined: bookmarks toolbar item looks bad if customized to panel
Testing completed (on m-c, etc.): locally and on m-c now
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #8383956 - Flags: approval-mozilla-aurora?
status-firefox30: affected → fixed
Attachment #8383956 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
You need to log in before you can comment on or make changes to this bug.