895938 - Australis: Dragging the bookmarks toolbar item to the menupanel makes it disappear

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

STR:

0. Make the bookmarks toolbar visible
1. Open customize mode
2. Drag the bookmarks toolbar items to the menupanel


ER:
The bookmarks toolbar items show up in the menupanel in some shape or form

AR:
One placeholder in the menupanel disappears, and no bookmarks show up.


After then exiting customize mode, the items will appear (misstyled, but that's a separate bug). Reopening customize mode will make them disappear again, so the only way to get them back to the bookmarks toolbar is to reset customize mode.

Comment 1

[:Gijs (he/him)]

Attachment: bookmarks toolbar item invisible in the menupanel

This is only checked on OS X and possibly unfinished, but I think this is more or less what it takes. Going to be focusing on the perf work in the near future, so putting this up here in case anyone else wants to have a look.

Comment 2

[Mike de Boer [:mikedeboer]]

Gijs, are you able to reproduce this still? Following your STR leads to the ER in my case...

Comment 3

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Gijs, are you able to reproduce this still? Following your STR leads to the
> ER in my case...

On which platform? I can still reproduce on OS X.

Comment 4

[Willy_ Foo_Foo]

I have over 90 add-ons (other users more even) and I just cannot figure out how I will relate to those who have a toolbar button in what Mozilla is planning for FF25 I'm not an artist nor a techie, but I do appreciate having a browser interface set up for my convenience. Hereafter an image of my FF22 GUI ( http://hpics.li/3bc2987 ). From this, and considering what is planned, tell me how I will manage ...

Comment 5

[Mike de Boer [:mikedeboer]]

ElevenReds, even though I sympathize with your concern, I have to point out that Bugzilla is not a discussion forum. If you have information that might help developers fix this bug, feel free to leave a comment. Otherwise, please leave a message at the appropriate forums. Thank you.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: make bookmark toolbar items work properly in the menupanel

Comment 7

[Marco Bonardo [:mak]]

I'm looking into this by making a patch, since I don't understand some of the changes there, and I'm not sure which part is me not understand Australis and which part is Australis not understanding me.
I will let you know shortly.

Comment 8

[Marco Bonardo [:mak]]

Attachment: wip

this is Windows only so far, the approach should be a bit cleaner, even if it requires a bit of changes in the xul structure. Your patch has been very useful to find the painful points in the css.

Acutally, the PlacesToolbar contents could be simplified even more by removing a duplicated hbox, but that may break other functionality, so I didn't touch it for now, it's something good for a follow-up.
Fixing other platforms should not be hard, I will do that tomorrow.

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [:mak] from comment #8)
> Created attachment 784125 [details] [diff] [review]
> wip
> 
> this is Windows only so far, the approach should be a bit cleaner, even if
> it requires a bit of changes in the xul structure. Your patch has been very
> useful to find the painful points in the css.
> 
> Acutally, the PlacesToolbar contents could be simplified even more by
> removing a duplicated hbox, but that may break other functionality, so I
> didn't touch it for now, it's something good for a follow-up.
> Fixing other platforms should not be hard, I will do that tomorrow.

<3 how you graciously took over the work :)
Looking at the patch it does indeed look quite a bit cleaner! Linux & OSX changes should be trivial (very similar to Win), in my experience.
Looking forward to seeing the final patch!

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [:mak] from comment #8)
> Your patch has been very useful to find the painful points in the css.

It was my follow-up to Gijs' initial patch, so he deserves the credit too ;)

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v3

Two issues still open here, but I'd leave those to follow-ups:
1. PlacesToolbar contains 2 hboxes, one can probably be killed, though doing that may cause unexpected regressions, that is not wanted here
2. there's no good icon for this item in the palette or in the menu panel, can easily be addressed in a follow-up

I quickly tested this on Win7, Mint and Lion.
Mike, could you please verify this and let me know?

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 784377 [details] [diff] [review]
patch v3

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

Marco, when I applied the patch locally on OSX I noticed two things:

1) the patch already bitrotted :(
2) after fixing that, what I saw when I dragged the bookmarks item in cust mode to the menu panel was one single button called 'Bookmarks Toolbar Items'. When I clicked that one, it didn't do anything.

Do you know what's going on? Is my observation due to invalid un-bitrotting your patch?

::: browser/base/content/browser-places.js
@@ +991,5 @@
> +  onPlaceholderCommand: function () {
> +    let widget = CustomizableUI.getWidget("personal-bookmarks")
> +                               .forWindow(window);
> +    if (widget.overflowed || widget.areaType == CustomizableUI.TYPE_MENU_PANEL)
> +      PlacesCommandHook.showPlacesOrganizer('BookmarksToolbar');

nit: double quotes

Comment 14

[Marco Bonardo [:mak]]

The fact you saw one single button is what it's expected to happen.
The fact it didn't do nothing is due to mconley landing changes to the areaType getter, it's a trivial fix. I will unbitrot the patch.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v4

unbitrotted, fixed the command handler and addressed your comment

Comment 16

[Mike de Boer [:mikedeboer]]

Thanks! When I start working with your patch, I noticed - again :P - two things:

1) the button text overflows the button inside the menu panel. That is not an issue particular to this bug, so not a show-stopper here.
2) when I move the bookmarks from the bookmarks toolbar into the menu-panel, leave cust mode, enter it again and move the bookmarks back into the bookmarks toolbar: the panels for 'Most Visited' and bookmark folders don't work anymore.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> 1) the button text overflows the button inside the menu panel. That is not
> an issue particular to this bug, so not a show-stopper here.

yes, someone will have to decide how to handle long names, also for add-ons.

> 2) when I move the bookmarks from the bookmarks toolbar into the menu-panel,
> leave cust mode, enter it again and move the bookmarks back into the
> bookmarks toolbar: the panels for 'Most Visited' and bookmark folders don't
> work anymore.

I can reproduce, investigating.

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v5

The view was not expecting to be on something that is not a toolbar, so this skips view initialization if it's not on a toolbar.
That's also a perf win, since we don't need an attached view on the menu panel.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 784984 [details] [diff] [review]
patch v5

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

Looks good! I have one small question left, which I'm sure you can answer ;)

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +159,5 @@
>  toolbarpaletteitem[place="panel"] > toolbaritem > toolbarbutton > .toolbarbutton-icon {
>    min-width: calc(8em / 3);
>    min-height: calc(8em / 3);
>    margin: calc(5em / 12);
> +  -moz-margin-end: 0;

Why is this necessary for all button icons in the panel/ palette?

Comment 20

[Marco Bonardo [:mak]]

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> > +  -moz-margin-end: 0;
> 
> Why is this necessary for all button icons in the panel/ palette?

ah right, that should only be needed for toolbarbutton that have a label, if we hide the text to override .toolbarbutton-icon[label]:not([label=""])
Will fix it.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v6

With a more appropriate rule

Comment 22

[Marco Bonardo [:mak]]

https://hg.mozilla.org/projects/ux/rev/1fd99ab66989

Comment 23

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #20)
> (In reply to Mike de Boer [:mikedeboer] from comment #19)
> > > +  -moz-margin-end: 0;
> > 
> > Why is this necessary for all button icons in the panel/ palette?
> 
> ah right, that should only be needed for toolbarbutton that have a label, if
> we hide the text to override .toolbarbutton-icon[label]:not([label=""])
> Will fix it.

This still doesn't make sense to me, and it broke the centering of all the icons in the panel, because they now all have a start margin of 5em/8, and 0 right margin. See bug 918782. What is the point of this rule?

Comment 24

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #23)
> (In reply to Marco Bonardo [:mak] from comment #20)
> > (In reply to Mike de Boer [:mikedeboer] from comment #19)
> > > > +  -moz-margin-end: 0;
> > > 
> > > Why is this necessary for all button icons in the panel/ palette?
> > 
> > ah right, that should only be needed for toolbarbutton that have a label, if
> > we hide the text to override .toolbarbutton-icon[label]:not([label=""])
> > Will fix it.
> 
> This still doesn't make sense to me, and it broke the centering of all the
> icons in the panel, because they now all have a start margin of 5em/8, and 0
> right margin. See bug 918782. What is the point of this rule?

Egh, that was meant to be bug 918551.

Comment 25

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #23)
> This still doesn't make sense to me, and it broke the centering of all the
> icons in the panel, because they now all have a start margin of 5em/8, and 0
> right margin. See bug 918782. What is the point of this rule?

Is this on all platforms?
on linux there's this rule
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/toolbarbutton.css#26
while on Windows
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbarbutton.css#24
is it possible the rule is just wrong on Mac?
The fact is we are adding an additional end margin to separate the icon from the label, and in both the palette and the panel that space should not be there. Though, it sounds like Mac doesn't have such margin. bug 918551 is reported against Mac so far.

Comment 26

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #25)
> (In reply to :Gijs Kruitbosch from comment #23)
> > This still doesn't make sense to me, and it broke the centering of all the
> > icons in the panel, because they now all have a start margin of 5em/8, and 0
> > right margin. See bug 918782. What is the point of this rule?
> 
> Is this on all platforms?
> on linux there's this rule
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> toolbarbutton.css#26
> while on Windows
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> toolbarbutton.css#24
> is it possible the rule is just wrong on Mac?
> The fact is we are adding an additional end margin to separate the icon from
> the label, and in both the palette and the panel that space should not be
> there. Though, it sounds like Mac doesn't have such margin. bug 918551 is
> reported against Mac so far.

This is confusing. On all platforms, we override the general toolbarbutton rules you cited with this one:

https://mxr.mozilla.org/projects-central/source/ux/browser/themes/shared/customizableui/panelUIOverlay.inc.css#179

This says that panel and palette icons *should* have margin, on both sides, in order to make the surrounding toolbar button bigger, and exactly fitting the grid layout in the panel. The labels come underneath the icon, not (horizontally) after it, so the end margin isn't about the space between the icon and the text at all.

You then added a rule that overrides that one and sets the end margin to 0 in all the panels/palettes. It doesn't look right to me, but maybe I'm misunderstanding your comment?

Comment 27

[Marco Bonardo [:mak]]

ok, that makes sense. I think you can remove the margin-end rule.
I'm sure I added it cause some icons were not centered. but off-hand I can only tell it seems to make more damage than benefix. rs=me on that.

Comment 28

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/1fd99ab66989