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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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

[:Gijs (he/him)]

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

I thought we had a bug for this already but I can't find it.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 4

[:Gijs (he/him)]

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.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 6

[:Gijs (he/him)]

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+

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Comment 8

[:Gijs (he/him)]

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.

Comment 9

[u428464]

Shouldn't this land ? ;)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Bug 977804 is happening too frequently to leave this in. Backed out.
https://hg.mozilla.org/mozilla-central/rev/d5bd7031fb3e

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 13

[:Gijs (he/him)]

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...

Comment 14

[u428464]

(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

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/13e905c91317

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13e905c91317

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 19

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2cc14451781