Closed Bug 1361686 Opened 4 years ago Closed 4 years ago

Share bookmark toolbar button styling between platforms

Categories

(Firefox :: Theme, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.5 - May 15
Tracking Status
firefox55 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-visual][p4])

Attachments

(2 files)

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.
FWIW, not a high priority in my view since we don't show this toolbar by default. No big deal if this misses 57.
Whiteboard: [photon-visual][p1] → [photon-visual][p4]
Iteration: --- → 55.5 - May 15
Flags: qe-verify?
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).
Priority: P1 → P3
Assignee: jhofmann → nobody
Status: ASSIGNED → NEW
Iteration: 55.5 - May 15 → ---
Whiteboard: [photon-visual][p4] → [reserve-photon-visual][p4]
Summary: Share bookmark toolbar button styling between platforms (and update icons) → Share bookmark toolbar button styling between platforms
Blocks: 1363028
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P3 → P1
Whiteboard: [reserve-photon-visual][p4] → [photon-visual][p4]
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
Attachment #8866099 - Flags: review?(dao+bmo) → review-
Now includes a new dropdown arrow designed by shorlander to use on all platforms until 57.
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.
Attachment #8866099 - Flags: review?(dao+bmo)
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
(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.
We discussed this and said that we'll not change the down arrow for Linux until 57, since it looks fine.
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/header@2x.png               (../shared/panic-panel/header@2x.png)
>    skin/classic/browser/panic-panel/header-small.png            (../shared/panic-panel/header-small.png)
>    skin/classic/browser/panic-panel/header-small@2x.png         (../shared/panic-panel/header-small@2x.png)
>    skin/classic/browser/panic-panel/icons.png                   (../shared/panic-panel/icons.png)
>    skin/classic/browser/panic-panel/icons@2x.png                (../shared/panic-panel/icons@2x.png)
> +  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.
Attachment #8866099 - Flags: review?(dao+bmo) → review+
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. :)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01ae37c9efc0
Share bookmark toolbar button styling between platforms. r=dao
Depends on: 1365026
https://hg.mozilla.org/mozilla-central/rev/01ae37c9efc0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Blocks: 1289147
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;
Depends on: 1366424
Depends on: 1367172
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.
(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.
Setting this to qe-verify- based on the previous comment.
Flags: qe-verify+ → qe-verify-
QA Contact: brindusa.tot
You need to log in before you can comment on or make changes to this bug.