Closed Bug 969592 Opened 6 years ago Closed 6 years ago

Consolidate style of bookmarks panel and its sub-panels

Categories

(Firefox :: Theme, defect)

28 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: phlsa, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:P2])

Attachments

(4 files, 11 obsolete files)

158.56 KB, image/png
Details
17.47 KB, patch
mak
: review+
Details | Diff | Splinter Review
285.29 KB, image/png
Details
278.30 KB, image/png
Details
Attached image Current state
The sub-menus of the bookmarks panel are currently styled like native menus, which can be jarring, especially on OS X, but also on Windows.

We should style them the same way as the main panel. That means:
1) Use the same background color
2) Use the same drop shadow
3) Use the same separators
4) Use the same hover state
5) Same typography (especially line height)

Work on this bug should probably be deferred until some of the other dependencies of bug 963098 are fixed, as they influence the style of the bookmarks panel which is the reference.
Depends on: 969584
Was just talking to mconley and I think we need some clarification/discussion here: Are we talking about restyling only the bookmarks menu popups, or all popups? By changing the bookmarks popup we introduce some inconsistency as now all popups don't look/feel the same. If this was the only place we had a popup I'd be more inclined to customize it, but we have many more (some even bookmarks related, such as clicking on a folder of bookmarks in the Bookmarks Toolbar).

By restyling all the popups we introduce some OS level inconsistencies, which I think we all agree would not be a positive change.

Thoughts?
Flags: needinfo?(philipp)
(In reply to Darrin Henein [:darrin] from comment #1)
> Was just talking to mconley and I think we need some
> clarification/discussion here: Are we talking about restyling only the
> bookmarks menu popups, or all popups? By changing the bookmarks popup we
> introduce some inconsistency as now all popups don't look/feel the same. If
> this was the only place we had a popup I'd be more inclined to customize it,
> but we have many more (some even bookmarks related, such as clicking on a
> folder of bookmarks in the Bookmarks Toolbar).
> 
> By restyling all the popups we introduce some OS level inconsistencies,
> which I think we all agree would not be a positive change.
> 
> Thoughts?

I think we're talking about just the bookmarks toolbar widget's menu/panel popup crossover, and any "submenus" emanating from there.

I think the current state is worse than having all of it look panel-like, even at the cost of OS level consistency, especially on e.g. Linux where the panels are forced to be bright-colored, and menus can be dark on some themes, so the contrast between the panel and its submenus is extremely stark.
Assignee: nobody → dhenein
(In reply to Darrin Henein [:darrin] from comment #1)
> Was just talking to mconley and I think we need some
> clarification/discussion here: Are we talking about restyling only the
> bookmarks menu popups, or all popups? By changing the bookmarks popup we
> introduce some inconsistency as now all popups don't look/feel the same. If
> this was the only place we had a popup I'd be more inclined to customize it,
> but we have many more (some even bookmarks related, such as clicking on a
> folder of bookmarks in the Bookmarks Toolbar).
> 
> By restyling all the popups we introduce some OS level inconsistencies,
> which I think we all agree would not be a positive change.
> 
> Thoughts?

I agree with Gijs here. This is more a matter of where menus get triggered from than of their contents. Right now you have one menu-ish thing (the bookmarks panel) opening a different menu that looks quite different. That contrast is the problem.

So I think we should align anything that spawns from the bookmarks panel with the style of that panel.
Flags: needinfo?(philipp)
Duplicate of this bug: 940141
Attachment #8386818 - Attachment is obsolete: true
Comment on attachment 8386818 [details] [diff] [review]
Bug-969592-ConsolidateBookmarksStyling.patch v1

Still WIP, one more fix coming.
Attachment #8386861 - Attachment is obsolete: true
Attachment #8386862 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8386862 [details] [diff] [review]
Bug-969592-ConsolidateBookmarksStyling.patch v1.2

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

So the CSS changes look OK from brief looks and having tested the changes (some small nits noted below).

However, the JS changes need some tweaking, I think. Essentially, these views are used for a bunch of other places (e.g. bookmarks toolbar folders, the main menu) and we don't want all those items to have the subviewbutton class. We're passing in some options specifically for the bookmarks menu button in:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1102

and the submenus should also be using those options (which probably means you need to rename the option from "mainView" to like "allViews" or something).

::: browser/base/content/browser.css
@@ +902,5 @@
>    -moz-image-region: auto;
>  }
>  
> +#BMB_bookmarksPopup menupopup {
> +  padding-top: 2px;

This should be in panelUIOverlay.inc.css

@@ +905,5 @@
> +#BMB_bookmarksPopup menupopup {
> +  padding-top: 2px;
> +}
> +
> +#BMB_bookmarksPopup menupopup > .bookmarks-actions-menuseparator {

Ditto.

::: browser/components/places/content/browserPlacesViews.js
@@ +267,5 @@
>        let label = PlacesUIUtils.getString("bookmarksMenuEmptyFolder");
>        aPopup._emptyMenuitem = document.createElement("menuitem");
>        aPopup._emptyMenuitem.setAttribute("label", label);
>        aPopup._emptyMenuitem.setAttribute("disabled", true);
> +      aPopup._emptyMenuitem.className = "bookmarks-item subviewbutton";

This is kind of odd. Why bookmarks-item? It's not an actual bookmarks item...

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +644,5 @@
>    margin-bottom: 0;
>  }
>  
> +/* Remove padding on xul:arrowscrollbox to avoid extra padding on footer */
> +#BMB_bookmarksPopup arrowscrollbox {

... and removing it here.
Attachment #8386862 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #9)
> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +644,5 @@
> >    margin-bottom: 0;
> >  }
> >  
> > +/* Remove padding on xul:arrowscrollbox to avoid extra padding on footer */
> > +#BMB_bookmarksPopup arrowscrollbox {
> 
> ... and removing it here.

Eh, ignore this bit.
Attached patch Bug-969592-v1.3.patch (obsolete) — Splinter Review
This patch addresses every case except for the Bookmarks Toolbar and Unsorted Bookmarks sub-folders in the Bookmarks Panel. See attached screenshot for clarification. Marco, Mike de Boer suggested you may be able to take a look and give some indication as to why this is happening?
Attachment #8386862 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Attached patch Bug-969592-v1.4.patch (obsolete) — Splinter Review
Attachment #8387059 - Attachment is obsolete: true
Attachment #8387062 - Attachment is obsolete: true
Attached patch Bug-969592-v1.5.patch (obsolete) — Splinter Review
Comment on attachment 8387065 [details] [diff] [review]
Bug-969592-v1.5.patch

IIRC Darrin said Gijs should review this...
Attachment #8387065 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8387065 [details] [diff] [review]
Bug-969592-v1.5.patch

We needinfo from Marco before this is ready.
Attachment #8387065 - Flags: review?(gijskruitbosch+bugs)
off-hand, you are using this fancy .options thing (that I never saw honestly), but these menus are defined as new PlacesMenu(event, 'place:folder=UNFILED_BOOKMARKS');"/> that means .options is null.

This .options things is bogus though, the getter should not return null, it should rather return an object with default values for the options... It can't be optional if then we never null-check it...
Flags: needinfo?(mak77)
and probably the new PlacesMenu calls in browser.xul should pass into the options object of the parent view
Attached patch Bug-969592-v1.6.patch (obsolete) — Splinter Review
Thanks for the help Marco, I added some defaults to the options object in the PlacesMenu constructor. Gijs, this patch styles only the items within the Bookmarks Manu Bar panel, and does not affect any other bookmarks lists.
Attachment #8387065 - Attachment is obsolete: true
Attachment #8387651 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8387651 [details] [diff] [review]
Bug-969592-v1.6.patch

>+#BMB_bookmarksPopup menupopup > hbox {
>+  /* emulating chrome://browser/content/places/menu.xml#places-popup-arrow but without the arrow */
>+  box-shadow: 0 0 4px rgba(0,0,0,0.2);
>+  background: #FFF;
>+  border: 1px solid rgba(0,0,0,0.25);
>+  border-radius: 4px;
>+  margin-top: -3px;
>+}

Without knowing and checking what this hbox is: Did you somewhere specify the appropriate text color (presumably black) for this?
(In reply to Dão Gottwald [:dao] from comment #20)
> Comment on attachment 8387651 [details] [diff] [review]
> Bug-969592-v1.6.patch
> 
> Without knowing and checking what this hbox is: Did you somewhere specify
> the appropriate text color (presumably black) for this?

Being a containing box with no bare text within, we can rely on the child elements tags/styles to specify the text colors. This style block simply changes the appearance of the container on Windows (which defaults to a grey background vs. white).
Comment on attachment 8387651 [details] [diff] [review]
Bug-969592-v1.6.patch

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

So, this got bitrotted by the checkmark changes from Blake.

Some other notes and confusion here... there's some constructors for the submenus for the bookmarks toolbar and the unsorted bookmarks inside browser.xul which should be passing these options, and otherwise I believe the defaults should be empty, so as not to change other menus/views. I also thought that the PlacesViewBase options getter already defaulted to having a non-null object which is why we didn't need nullchecks, so I don't think we need to construct the defaults object again the way this patch does. I think. Hopefully. Please check after unbitrotting.

::: browser/components/places/content/browserPlacesViews.js
@@ +1738,5 @@
>    }
>  #endif
>  
> +  if (!aOptions)
> +    aOptions = {};

This shouldn't be necessary. :-(

@@ +1740,5 @@
>  
> +  if (!aOptions)
> +    aOptions = {};
> +
> +  if (!("extraClasses" in aOptions))

nit: !aOptions.extraClasses

nit: braces around multiline ifs

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +645,5 @@
>  }
>  
> +/* Remove padding on xul:arrowscrollbox to avoid extra padding on footer */
> +#BMB_bookmarksPopup arrowscrollbox {
> +  padding: 0px;

Nit: padding-bottom ?

::: browser/themes/windows/browser.css
@@ +288,5 @@
> +/* ::::: bookmark panel submenus ::::: */
> +
> +#BMB_bookmarksPopup menupopup {
> +  -moz-appearance: none;
> +  -moz-binding: url("chrome://browser/content/places/menu.xml#places-popup-base");

Nit: is this necessary? Please doublecheck :-)
Attachment #8387651 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to Darrin Henein [:darrin] from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #20)
> > Comment on attachment 8387651 [details] [diff] [review]
> > Bug-969592-v1.6.patch
> > 
> > Without knowing and checking what this hbox is: Did you somewhere specify
> > the appropriate text color (presumably black) for this?
> 
> Being a containing box with no bare text within, we can rely on the child
> elements tags/styles to specify the text colors. This style block simply
> changes the appearance of the container on Windows (which defaults to a grey
> background vs. white).

Can you point out where those child elements get their color specified?
(In reply to Dão Gottwald [:dao] from comment #23)
> Can you point out where those child elements get their color specified?

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/menu.css#11
MenuText isn't necessarily black on Windows and Linux, which means that it may be illegible on a hardcoded white background.
Attached patch Bug-969592-v1.7.patch (obsolete) — Splinter Review
Gijs to take this over the finish line while I'm away :)
Attachment #8387651 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch Patch v1.8 (obsolete) — Splinter Review
This hardcodes to #000, which seems to be what we're using in the panels on Windows. I've also made some minor adjustments to the margin-top to make the items line up with their parent menu correctly when hovered, and I updated some of the rules that Blake added to deal with checkmarks to work in the submenus. 

Altogether I believe this works everywhere, although I've not touched the item size or font (bug 969584), nor their background/foreground color (on OS X it's still based on it being a menu, AFAICT).
Attachment #8387060 - Attachment is obsolete: true
Attachment #8387850 - Attachment is obsolete: true
Attachment #8388016 - Flags: review?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)
Hello
I do not know if I'm in the right place to show this bug
I have this for one week : The panel is too narrow favorites

http://i.imgur.com/wW6NcMU.png
http://i.imgur.com/qF7ZIj6.png

Nightly, Windows 7 x64
(In reply to G_HZ from comment #28)
> Hello
> I do not know if I'm in the right place to show this bug
> I have this for one week : The panel is too narrow favorites
> 
> http://i.imgur.com/wW6NcMU.png
> http://i.imgur.com/qF7ZIj6.png
> 
> Nightly, Windows 7 x64

What font are you using on the Windows level, and what DPI size?

(This is the same issues as I saw when using the latest STR from bug 980534)
Flags: needinfo?(gandalf_hz)
Yes, this is the same bug 980534 except that the panel is at home even closer
Windows font Tahoma Size 8
Flags: needinfo?(gandalf_hz)
It's the same thing if I put verdana font size 8
It's the same thing if I put Segoe UI font size 9 (Default font)

http://i.imgur.com/z1s06aq.png
http://i.imgur.com/SQQzXOH.png


Nightly, Windows 7 French x64
Work properly with the Vivaldi font size 8, but it's unreadable

http://i.imgur.com/TirRe5w.png
http://i.imgur.com/4Z1BA8K.png
Work properly with the Vivaldi font size 8, but it's unreadable

http://i.imgur.com/TirRe5w.png
http://i.imgur.com/4Z1BA8K.png

font setting
http://i.imgur.com/HLxfvYi.png
Comment on attachment 8388016 [details] [diff] [review]
Patch v1.8

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

::: browser/components/places/content/browserPlacesViews.js
@@ +279,5 @@
>        aPopup._emptyMenuitem.setAttribute("label", label);
>        aPopup._emptyMenuitem.setAttribute("disabled", true);
> +      aPopup._emptyMenuitem.className = "bookmark-item";
> +      if (typeof this.options.extraClasses.secondaryLevel == "string")
> +        aPopup._emptyMenuitem.className += " " + this.options.extraClasses.secondaryLevel;

this should use classList.add instead of string concatenation. here and below as well.

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +96,5 @@
>    display: none;
>  }
>  
> +#BMB_bookmarksPopup  menu,
> +#BMB_bookmarksPopup  menuitem:not(.panel-subview-footer) {

nit, there is now 2 spaces between these selectors. one of them can be removed.

is it possible to use a CSS class on all of the menus and menuitems that are descendants of #BMB_bookmarksPopup so we don't have to introduce this descendent selector?
Attached patch Restyle Bookmarks Menu Popups, (obsolete) — Splinter Review
Marco, Jared suggested you might be able to review this?
Attachment #8388862 - Flags: review?(mak77)
Attachment #8388016 - Attachment is obsolete: true
Attachment #8388016 - Flags: review?(mconley)
Assignee: dhenein → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Duplicate of this bug: 982245
Comment on attachment 8388862 [details] [diff] [review]
Restyle Bookmarks Menu Popups,

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

r=me with the following comments addressed and drag&drop indicator correctness testing (with AND without the arrowscrollbox visible)

I don't see any Linux styling in the patch, is the existing + shared styling enough there, or did you forget to add it?

::: browser/base/content/browser-places.js
@@ +1107,5 @@
>  
>      new PlacesMenu(event, "place:folder=BOOKMARKS_MENU", {
>        extraClasses: {
> +        mainLevel: "subviewbutton",
> +        secondaryLevel: "subviewbutton",

is there a reason to add a secondaryLevel, if we always want these menus to look consistent? This bug exists cause it's clear we want them to always look coherent, so I don't see the point of distinguishing levels.
may we coalesce both into a single property like entryClass or even just entry? (I leave bikeshed naming to you)

@@ +1108,5 @@
>      new PlacesMenu(event, "place:folder=BOOKMARKS_MENU", {
>        extraClasses: {
> +        mainLevel: "subviewbutton",
> +        secondaryLevel: "subviewbutton",
> +        footerClass: "subviewbutton panel-subview-footer"

I think this should define only the additional class for the footer (may be just named footer, there is no need for a class suffix since this is "extraClasses"), since the footer is part of the content and should look coherent with anything else, it should get both entry and footer extra classes.

So I think something like:
extraClasses: {
  entry: "subviewbutton",
  footer: "panel-subview-footer"
}
would be simpler

::: browser/base/content/browser.xul
@@ +824,5 @@
>                <menupopup id="BMB_bookmarksToolbarPopup"
>                           placespopup="true"
>                           context="placesContext"
>                           onpopupshowing="if (!this.parentNode._placesView)
> +                                           new PlacesMenu(event, 'place:folder=TOOLBAR', PLACES_MENU_DEFAULT_OPTIONS);">

I don't like much hardcoded defaults, since if someone changes the code and forgets about these, it breaks. would be better to read the info from the parent view. I wonder if one of these would do:

this.parentNode.parentNode.parentNode._placesView.options.extraClasses
or
PlacesUIUtils.getViewForNode(this.parentNode.parentNode).options.extraClasses

@@ +843,5 @@
>                <menupopup id="BMB_unsortedBookmarksPopup"
>                           placespopup="true"
>                           context="placesContext"
>                           onpopupshowing="if (!this.parentNode._placesView)
> +                                           new PlacesMenu(event, 'place:folder=UNFILED_BOOKMARKS', PLACES_MENU_DEFAULT_OPTIONS);"/>

ditto

::: browser/components/places/content/browserPlacesViews.js
@@ +355,5 @@
>          }
>  
>          element.appendChild(popup);
>          element.className = "menu-iconic bookmark-item";
> +        if (element == this._rootElt && typeof this.options.extraClasses.mainLevel == "string") {

this sounds strange, there should be no representation of the root element of a view, so what is this case covering really? I think the fact mainLevel and secondaryLevel are the same value is covering a bug.

@@ +813,5 @@
>        aPopup._endOptOpenAllInTabs = document.createElement("menuitem");
>        aPopup._endOptOpenAllInTabs.className = "openintabs-menuitem";
> +
> +      let extraClasses = this.options.extraClasses;
> +      if(typeof extraClasses.footerClass == "string")

everywhere we do typeof this.options.extraClasses.something, I don't see why here we assign to a temp var...


Also, missing space after if

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +640,5 @@
>    margin-top: 0;
>    margin-bottom: 0;
>  }
>  
> +/* Remove padding on xul:arrowscrollbox to avoid extra padding on footer */

please verify this change doesn't confuse drag&drop indicator when dragging over the menu entries (we do some fancy calculations to take the arrowscrollbox space into account)

@@ +647,5 @@
> +}
> +
> +#BMB_bookmarksPopup menupopup {
> +  padding-top: 2px;
> +}

ditto for d&d check

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +29,5 @@
> +  background: #FFF;
> +  border: 1px solid rgba(0,0,0,0.25);
> +  border-radius: 3.5px;
> +  margin-top: -4px;
> +}

ditto for d&d check
Attachment #8388862 - Flags: review?(mak77) → review+
This should have all the nits addressed. I needed to change two things only on Linux: one was something that should have landed in bug 978309, which was setting a width + height for the menu's arrows, and one was overriding menuitem's appearance with important, because for checked menu items (like the nested 'view bookmarks toolbar' item) it is set to checkedmenuitem with 'important', so we have to use the same to override it... :-(
Attachment #8390080 - Flags: review?(mak77)
Attachment #8388862 - Attachment is obsolete: true
Blocks: 969584
No longer depends on: 969584
Whiteboard: [Australis:P-] → [Australis:P2]
Attachment #8390080 - Flags: review?(mak77) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/1a9fa8fde7e0
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1a9fa8fde7e0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
Comment on attachment 8390080 [details] [diff] [review]
Restyle Bookmarks Menu Popups,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: bad styling of submenus in the bookmarks panel
Testing completed (on m-c, etc.): on m-c, locally
Risk to taking this patch (and alternatives if risky): like bug 972405, 979378, etc., this isn't as low risk as I'd like. However, I think we should move forward, take all these patches, and fix the last rough edges ASAP. Definitely leads to a better state than what we had before.
String or IDL/UUID changes made by this patch: none
Attachment #8390080 - Flags: approval-mozilla-aurora?
Attachment #8390080 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached image Bookmarks_Panel.png
The BM menu line spacing is too wide and the font too big.  It's inconsistent with other menus.

Running Windows 8.1 x64.  DPI is 120.
(In reply to Gary [:streetwolf] from comment #42)
> Created attachment 8390536 [details]
> Bookmarks_Panel.png

Right now, we just made the two panels consistent. Both will have smaller line heights and fonts after bug 969584 finishes.
Attached image BM-Panel-Nice.png
The BM panel should look more like this IMO.  I did this with some css code.
(In reply to Gary [:streetwolf] from comment #45)
> Created attachment 8390547 [details]
> BM-Panel-Nice.png
> 
> The BM panel should look more like this IMO.  I did this with some css code.

Spacing/size is being adjusted in bug 969584
Depends on: 984156
Depends on: 985659
QA Whiteboard: [qa-]
Depends on: 1009367
Depends on: 1042268
You need to log in before you can comment on or make changes to this bug.