1361686 - Share bookmark toolbar button styling between platforms

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

In bug 1352364 we shared the main toolbar button code to make it easier to implement a uniform look. We should also consolidate the bookmark buttons to enable them to share the toolbar button look.

We have a lot of OS specific icons in the bookmark toolbar right now, so I'll probably need to request replacements for some of those from you, Stephen

Because of the updated icons I propose deferring this until early 57 cycle. I'll upload a partial patch for the changes we removed from bug 1352364.

Comment 1

[Dão Gottwald [:dao]]

FWIW, not a high priority in my view since we don't show this toolbar by default. No big deal if this misses 57.

Comment 2

[u428464]

FWIW the bookmark toolbar can be enabled on this interactive mockup : https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html (new icons are also visible).

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Bug 1361686 - Share bookmark toolbar button styling between platforms.

Review commit: https://reviewboard.mozilla.org/r/137698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137698/

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

https://reviewboard.mozilla.org/r/137698/#review141114

::: browser/themes/shared/toolbarbuttons.inc.css:380
(Diff revision 1)
>  }
>  
> +/* ::::: bookmark buttons ::::: */
> +
> +toolbarbutton.bookmark-item:not(.subviewbutton),
> +#personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {

Not sure why #bookmarks-toolbar-placeholder is part of this...

::: browser/themes/shared/toolbarbuttons.inc.css:390
(Diff revision 1)
> +  border-radius: var(--toolbarbutton-border-radius);
> +  background-origin: padding-box !important;
> +  background-clip: padding-box !important;
> +  transition-property: background-color, border-color, box-shadow;
> +  transition-duration: 150ms;
> +}

Please merge with this rule:
http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/browser/themes/shared/toolbarbuttons.inc.css#138-152

Looks like only the horizontal padding should be smaller. You can adjust that in a separate rule.

::: browser/themes/shared/toolbarbuttons.inc.css:395
(Diff revision 1)
> +}
> +
> +toolbarbutton.bookmark-item:not(.subviewbutton):hover:not([disabled="true"]):not([open]) {
> +  border-color: var(--toolbarbutton-hover-bordercolor);
> +  background: var(--toolbarbutton-hover-background);
> +}

Please merge with this rule:
http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/browser/themes/shared/toolbarbuttons.inc.css#244-261

::: browser/themes/shared/toolbarbuttons.inc.css:402
(Diff revision 1)
> +toolbarbutton.bookmark-item:not(.subviewbutton):hover:active:not([disabled="true"]),
> +toolbarbutton.bookmark-item[open="true"] {
> +  border-color: var(--toolbarbutton-active-bordercolor);
> +  box-shadow: var(--toolbarbutton-active-boxshadow);
> +  background: var(--toolbarbutton-active-background);
> +}

Please merge with this rule:
http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/browser/themes/shared/toolbarbuttons.inc.css#263-273

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137698/diff/1-2/

Comment 6

[Johann Hofmann [:johannh]]

Now includes a new dropdown arrow designed by shorlander to use on all platforms until 57.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

https://reviewboard.mozilla.org/r/137698/#review141752

::: browser/themes/shared/toolbarbuttons.inc.css:407
(Diff revision 2)
>  }
>  
> +/* ::::: bookmark buttons ::::: */
> +
> +.bookmark-item > .toolbarbutton-menu-dropmarker {
> +  list-style-image: url("chrome://browser/skin/places/arrow-down.svg");

The icon uses context-fill but you don't set a fill color, so this will just use black. Please set fill: currentColor.

::: browser/themes/shared/toolbarbuttons.inc.css:410
(Diff revision 2)
> +
> +.bookmark-item > .toolbarbutton-menu-dropmarker {
> +  list-style-image: url("chrome://browser/skin/places/arrow-down.svg");
> +  margin-top: 1px;
> +  margin-inline-start: 3px;
> +  -moz-appearance: none !important;

Does this actually need !important?

Looks good otherwise.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137698/diff/2-3/

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

https://reviewboard.mozilla.org/r/137698/#review141752

> Does this actually need !important?
> 
> Looks good otherwise.

Yes, for overriding https://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/toolkit/themes/linux/global/toolbarbutton.css#73

Comment 10

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #9)
> Comment on attachment 8866099 [details]
> Bug 1361686 - Share bookmark toolbar button styling between platforms.
> 
> https://reviewboard.mozilla.org/r/137698/#review141752
> 
> > Does this actually need !important?
> > 
> > Looks good otherwise.
> 
> Yes, for overriding
> https://searchfox.org/mozilla-central/rev/
> d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/toolkit/themes/linux/global/
> toolbarbutton.css#73

We should just remove that !important too.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137698/diff/3-4/

Comment 12

[Johann Hofmann [:johannh]]

We discussed this and said that we'll not change the down arrow for Linux until 57, since it looks fine.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

https://reviewboard.mozilla.org/r/137698/#review141892

::: browser/themes/shared/jar.inc.mn:178
(Diff revision 4)
>    skin/classic/browser/panic-panel/[e-mail]               (../shared/panic-panel/[e-mail])
>    skin/classic/browser/panic-panel/header-small.png            (../shared/panic-panel/header-small.png)
>    skin/classic/browser/panic-panel/[e-mail]         (../shared/panic-panel/[e-mail])
>    skin/classic/browser/panic-panel/icons.png                   (../shared/panic-panel/icons.png)
>    skin/classic/browser/panic-panel/[e-mail]                (../shared/panic-panel/[e-mail])
> +  skin/classic/browser/places/arrow-down.svg                   (../shared/places/arrow-down.svg)

This will probably give you a failure in browser/base/content/test/static/browser_all_files_referenced.js on Linux. Could add an exception for now.

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137698/diff/4-5/

Comment 15

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21faa734f1ca253f527e0b11fc801bf9d394825b

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137698/diff/5-6/

Comment 17

[Johann Hofmann [:johannh]]

There were unrelated try failures but rebasing seems to have gotten rid of those, at least locally for me.

(In reply to Dão Gottwald [::dao] from comment #13)
> This will probably give you a failure in
> browser/base/content/test/static/browser_all_files_referenced.js on Linux.
> Could add an exception for now.

I added an ifndef for Linux in the jar.mn instead, adding an exception there should be the very last resort IMO. :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01ae37c9efc0
Share bookmark toolbar button styling between platforms. r=dao

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/01ae37c9efc0

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8866099 [details]
Bug 1361686 - Share bookmark toolbar button styling between platforms.

https://reviewboard.mozilla.org/r/137698/#review144338

::: browser/themes/osx/browser.css:318
(Diff revision 6)
> -  -moz-box-orient: horizontal;
> -}
> -
>  .bookmark-item > .toolbarbutton-menu-dropmarker {
> -  list-style-image: url("chrome://browser/skin/places/folderDropArrow.png");
> -  -moz-image-region: rect(0, 7px, 5px, 0);
> +  list-style-image: url("chrome://browser/skin/places/arrow-down.svg");
> +  fill: currentColor;

This is missing -moz-context-properties: fill;

Comment 21

[Johann Hofmann [:johannh]]

Mozscreenshots results:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=0f4df67c5f162e00d6f52825badf468aefbfba19&newProject=mozilla-central&newRev=a4235c4be96edaf90b5d6d7c20272a8761ca2339

Comment 22

[ovidiu boca[:Ovidiu]]

Attachment: Windows 10 (Nightly 55.0a1 2017-06-09)-1.png

I tested this issue on Windows 10 with FF Nightly 55.0a1(2017-06-09) and the actual results are different from the expected ones presented in the specifications (https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.html)

Please see the attached print screen with my actual results.

Comment 23

[Johann Hofmann [:johannh]]

(In reply to ovidiu boca[:Ovidiu] from comment #22)
> Created attachment 8876116 [details]
> Windows 10 (Nightly 55.0a1 2017-06-09)-1.png
> 
> I tested this issue on Windows 10 with FF Nightly 55.0a1(2017-06-09) and the
> actual results are different from the expected ones presented in the
> specifications
> (https://people-mozilla.org/~shorlander/projects/photon/Mockups/windows-10.
> html)
> 
> Please see the attached print screen with my actual results.

This bug isn't about implementing the exact design as per spec. It changes things in the background to make the ongoing engineering work easier. Because the patch is shipping with 55 and we've been testing Nightly in Photon mode for the most part, it is important to verify that Beta 55 (with the non-Photon style) does not show any major visual regressions compared to 54. I don't think you need to test this against Nightly at this point.

Comment 24

[Johann Hofmann [:johannh]]

Setting this to qe-verify- based on the previous comment.