Share bookmark toolbar button styling between platforms

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(2 attachments)

Assignee

Description

2 years ago
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?

Comment 2

2 years ago
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]
Assignee

Updated

2 years ago
Summary: Share bookmark toolbar button styling between platforms (and update icons) → Share bookmark toolbar button styling between platforms
Assignee

Updated

2 years ago
Blocks: 1363028
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P3 → P1
Whiteboard: [reserve-photon-visual][p4] → [photon-visual][p4]

Comment 4

2 years ago
mozreview-review
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-
Comment hidden (mozreview-request)
Assignee

Comment 6

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

Comment 7

2 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Comment 9

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Assignee

Comment 12

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

Comment 13

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
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

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01ae37c9efc0
Share bookmark toolbar button styling between platforms. r=dao

Updated

2 years ago
Depends on: 1365026

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01ae37c9efc0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Blocks: 1289147
Depends on: 1365593

Comment 20

2 years ago
mozreview-review
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;

Updated

2 years ago
Depends on: 1366424

Updated

2 years ago
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.
Assignee

Comment 23

2 years ago
(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.
Assignee

Comment 24

2 years ago
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.