Closed Bug 895938 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Gijs, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 6 obsolete files)

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.
Whiteboard: [Australis:P2]
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.
Assignee: nobody → gijskruitbosch+bugs
Assignee: gijskruitbosch+bugs → nobody
Gijs, are you able to reproduce this still? Following your STR leads to the ER in my case...
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
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 ...
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.
Attachment #779643 - Attachment is obsolete: true
Attachment #783840 - Flags: feedback?(mak77)
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.
Attached patch wip (obsolete) — Splinter Review
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.
(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!
Assignee: mdeboer → mak77
(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 ;)
Blocks: 890567
Duplicate of this bug: 890567
Attached patch patch v3 (obsolete) — Splinter Review
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?
Attachment #783840 - Attachment is obsolete: true
Attachment #784125 - Attachment is obsolete: true
Attachment #783840 - Flags: feedback?(mak77)
Attachment #784377 - Flags: review?(mdeboer)
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
Attachment #784377 - Flags: review?(mdeboer) → review-
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.
Attached patch patch v4 (obsolete) — Splinter Review
unbitrotted, fixed the command handler and addressed your comment
Attachment #784377 - Attachment is obsolete: true
Attachment #784957 - Flags: review?(mdeboer)
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.
(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.
Attached patch patch v5 (obsolete) — Splinter Review
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.
Attachment #784957 - Attachment is obsolete: true
Attachment #784957 - Flags: review?(mdeboer)
Attachment #784984 - Flags: review?(mdeboer)
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?
Attachment #784984 - Flags: review?(mdeboer) → review+
(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.
Attached patch patch v6Splinter Review
With a more appropriate rule
Attachment #784984 - Attachment is obsolete: true
https://hg.mozilla.org/projects/ux/rev/1fd99ab66989
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-ux]
No longer depends on: 901418
(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?
Flags: needinfo?(mak77)
(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.
(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.
Flags: needinfo?(mak77)
(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?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
https://hg.mozilla.org/mozilla-central/rev/1fd99ab66989
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-ux] → [Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.