971956 - Bookmark toolbar items uses the old hover styling in overflow panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u428464]

Several issues here :
First the styling of the bookmark toolbar items is not good in the overflow panel.

But the main problem is that after it has overflown even if it's still customizable the bookmark toolbar items put back in the bookmarks toolbar doesn't work anymore.

STR : 1. put the item in the navbar
2. Exit customization
3. Make it overflow
4. Resize the browser so it quits overflow
5. Customize it to put it in the bookmarks toolbar
6. Exit customize mode
7. Click on a folder of your bookmarks toolbar

CR : Nothing happens

ER : Bookmarks folder normally opens.

Comment 1

[u428464]

Note the classic bookmarks still works. Only the folder doesn't open.

Comment 2

[u428464]

But strangely enough right clicking on a classic bookmark after the STR makes the context menu opened have all the places actions in it.

Comment 3

[:Gijs (he/him)]

Them completely not working anymore sounds worse than P4. Do you know if this is a regression?

Marco, any ideas here?

Comment 4

[u428464]

I never tried to make my bookmarks toolbar items overflow before so I have no idea but I think that's not new compared to the overflow panel implementation. 

FWIW The folder reopens after a browser restart.

I marked it P4 because very few people put their bookmarks in the navbar (the styling is broken anyway) and the fact they overflow is even more uncommon.

Comment 5

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #3)
> Them completely not working anymore sounds worse than P4. Do you know if
> this is a regression?
> 
> Marco, any ideas here?

I just think we are probably not properly rebuilding the binding in PlacesToolbarHelper, it's very likely it needs some code to cope with overflow.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Guillaume C. [:ge3k0s] from comment #2)
> But strangely enough right clicking on a classic bookmark after the STR
> makes the context menu opened have all the places actions in it.

this is because the binding is broken, so the context menu shows all of the entries

Comment 7

[Marco Bonardo [:mak]]

it may also be broken in the Remove From Toolbar, Move to Toolbar case, if we don't fire CustomizeStart/CustomizeDone for that case.

Comment 8

[u428464]

The main issue is resolved here, but I think this bug can stay open for the hover styling part.

Comment 9

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

Attachment: Badness in OS X.

Yeah, it looks pretty bad on OS X - even without hovering it.

Comment 10

[:Gijs (he/him)]

Attachment: fix styling of items in widget overflow list,

Tested on OS X, this seems to do the job...

Comment 11

[:Gijs (he/him)]

Attachment: 971956-widget-overflow-list

Needed only two small tweaks on top of what I did to make it work on Windows (and probably Linux).

Comment 12

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

Comment on attachment 8386786 [details] [diff] [review]
971956-widget-overflow-list

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

Tested on Ubuntu, and it looks good. r=me with my nit fixed.

::: browser/themes/osx/browser.css
@@ +168,5 @@
>    padding: 1px 8px;
>    margin: 0 0 1px;
>  }
> +#personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {
> +  -moz-box-orient: horizontal;

Since this has the equivalent selector with the above rule, let's merge them.

Comment 13

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/69f04d5d6700

(In reply to Mike Conley (:mconley) from comment #12)
> Comment on attachment 8386786 [details] [diff] [review]
> 971956-widget-overflow-list
> 
> Review of attachment 8386786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tested on Ubuntu, and it looks good. r=me with my nit fixed.
> 
> ::: browser/themes/osx/browser.css
> @@ +168,5 @@
> >    padding: 1px 8px;
> >    margin: 0 0 1px;
> >  }
> > +#personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {
> > +  -moz-box-orient: horizontal;
> 
> Since this has the equivalent selector with the above rule, let's merge them.

We talked about this, but the previous rule has another selector, so left them separate.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69f04d5d6700

Comment 15

[:Gijs (he/him)]

Comment on attachment 8386786 [details] [diff] [review]
971956-widget-overflow-list

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: When overflown, the bookmarks toolbar items look bad
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d08a9e56086