Closed Bug 968447 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P3-])

Attachments

(2 files, 3 obsolete files)

Attached image 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.
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
Attached patch Patch (obsolete) — Splinter Review
Attachment #8376458 - Flags: review?(gijskruitbosch+bugs)
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-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8376458 - Attachment is obsolete: true
Attachment #8377751 - Flags: review?(gijskruitbosch+bugs)
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+
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #8377751 - Attachment is obsolete: true
Attachment #8378634 - Flags: review?(gijskruitbosch+bugs)
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+
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
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+
Attached patch Patch v2Splinter Review
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 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+
(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
Closed: 10 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?
Attachment #8383956 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.