Closed Bug 971956 Opened 6 years ago Closed 6 years ago

Bookmark toolbar items uses the old hover styling in overflow panel

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

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

People

(Reporter: ge3k0s, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 files, 1 obsolete file)

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.
Whiteboard: [Australis:P4]
Note the classic bookmarks still works. Only the folder doesn't open.
But strangely enough right clicking on a classic bookmark after the STR makes the context menu opened have all the places actions in it.
Them completely not working anymore sounds worse than P4. Do you know if this is a regression?

Marco, any ideas here?
Flags: needinfo?(ge3k0s)
Whiteboard: [Australis:P4] → [Australis:P3]
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.
Flags: needinfo?(ge3k0s)
(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.
(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
it may also be broken in the Remove From Toolbar, Move to Toolbar case, if we don't fire CustomizeStart/CustomizeDone for that case.
The main issue is resolved here, but I think this bug can stay open for the hover styling part.
Summary: Bookmark toolbar items in the navbar is broken in overflow panel → Bookmark toolbar items uses the old hover styling in overflow panel
Attached image Badness in OS X.
Yeah, it looks pretty bad on OS X - even without hovering it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 972286
Assignee: nobody → gijskruitbosch+bugs
Tested on OS X, this seems to do the job...
Needed only two small tweaks on top of what I did to make it work on Windows (and probably Linux).
Attachment #8386380 - Attachment is obsolete: true
Attachment #8386786 - Flags: review?(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.
Attachment #8386786 - Flags: review?(mconley) → review+
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.
Status: NEW → ASSIGNED
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Blocks: 964995
https://hg.mozilla.org/mozilla-central/rev/69f04d5d6700
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
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
Attachment #8386786 - Flags: approval-mozilla-aurora?
Attachment #8386786 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Depends on: 1197652
You need to log in before you can comment on or make changes to this bug.