1377165 - Bookmark star icon in location bar should be blue (not black) when filled in

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Bookmark star icon.png

Bookmark star icon in doorhanger/panelUI menu should be black to be consistent with other icons.

Comment 1

[:Gijs (he/him)]

Are there specs of what the colour of the star should be when it's filled in, in the location bar? Maybe that shouldn't be black? I couldn't find any specs (nothing on https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229940647 )...

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

at the table today shorlander said it should be filled in blue as it was before.

Comment 3

[Fahima Zulfath]

Could able to track the bug in Firefox 56.
Affected version : Firefox Nightly 56.0a1 [Build ID: 20170630030203]
Affected OS: Windows 10.0 X 64

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1377165 - use correct fill colours and opacity for in-urlbar icons,

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

Comment 5

[:Gijs (he/him)]

Gah, I thought this was the bug that had discussion, but that was bug 1377227. Too many bugs. Anyway, from what I gathered there, and in bug 1377202 and here, I think this patch is correct... in theory.

This uses 0.6 opacity because that's what the spec says, but IMO for dark themes (or at least the compact dark theme) this makes the icon too grey/light, because the current color isn't white but off-white already. I don't know that we should depend on that being the case for all dark themes, though... bug 1377227 comment 3 suggests it's not always the case that we'll use a dark location bar for dark themes (maybe non-compact dark lwthemes do something else)?

I took the liberty of assuming the blue highlight colour for starred bookmarks shouldn't be opacity-d into greyness, so I'm forcing opacity: 1 there.

Given the above, I'd like a UI review for this, especially as concerns behaviour on compact dark, other dark lwthemes, and (of course) the default theme. Trypush with builds (should be up 'soon' given they're artifact builds):

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

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to :Gijs from comment #1)
> Are there specs of what the colour of the star should be when it's filled
> in, in the location bar? Maybe that shouldn't be black? I couldn't find any
> specs (nothing on
> https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229940647 )...

Yeah that has the specs for what the buttons / icons should look like. I can add the active state. Should be Blue 50: #0a84ff

http://design.firefox.com/photon/visual/color.html#blue

Comment 7

[Stephen Horlander [:shorlander]]

(In reply to :Gijs from comment #5)
> I took the liberty of assuming the blue highlight colour for starred
> bookmarks shouldn't be opacity-d into greyness, so I'm forcing opacity: 1
> there.

Yeah, should be full opacity.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8882605 [details]
Bug 1377165 - use correct fill colours and opacity for in-urlbar icons,

https://reviewboard.mozilla.org/r/153694/#review158866

::: browser/themes/shared/browser.inc.css:112
(Diff revision 1)
>    display: none;
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +.urlbar-icon,
> +#urlbar toolbarbutton {

What's the point of this selector? The bookmark and page action buttons seem to have the urlbar-icon class, as they should.

::: browser/themes/shared/browser.inc.css:118
(Diff revision 1)
> +  -moz-context-properties: fill, fill-opacity;
> +  fill: currentColor;
> +  fill-opacity: 0.6;
> +  color: inherit;
> +}
> +%endif

Please move this to urlbar-searchbar.inc.css.

::: browser/themes/shared/toolbarbutton-icons.inc.css:65
(Diff revision 1)
>  }
>  
> +#star-button[starred] {
> +  fill-opacity: 1;
> +  fill: var(--toolbarbutton-icon-fill-attention);
> +}

#star-button should be in urlbar-searchbar.inc.css too. toolbarbutton-icons.inc.css is for full-fledged buttons on the toolbar, not other UI elements that happen to be <toolbarbutton>.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8882605 [details]
Bug 1377165 - use correct fill colours and opacity for in-urlbar icons,

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8882605 [details]
Bug 1377165 - use correct fill colours and opacity for in-urlbar icons,

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

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8882605 [details]
Bug 1377165 - use correct fill colours and opacity for in-urlbar icons,

https://reviewboard.mozilla.org/r/153694/#review158896

::: browser/themes/shared/urlbar-searchbar.inc.css:101
(Diff revision 3)
> +  fill: var(--toolbarbutton-icon-fill-attention);
> +}
> +
> +/* Page action popup */
> +#page-action-bookmark-button {
> +  list-style-image: url("chrome://browser/skin/bookmark-hollow.svg");

This and related icons need to set a fill color. Can you file a new bug on this?

Comment 12

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #11)
> This and related icons need to set a fill color. Can you file a new bug on
> this?

Filed bug 1377535.

Comment 13

[:Gijs (he/him)]

Also filed bug 1377537 to move the zoom button CSS.

Comment 14

[:Gijs (he/him)]

I've just realized that Stephen is out this week. Rather than keeping it in the current state (which has 3 separate issues filed against it - bug 1377227, bug 1377202 and this one) for another week, I think I'll just go ahead and get this landed (which I believe addresses all 3 bugs), and if we need follow-up changes we can do those in separate bugs.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d089acf3ff1a
use correct fill colours and opacity for in-urlbar icons, r=dao

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/d089acf3ff1a

Comment 17

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm marking this bug as VERIFIED, as issue is fixed, starting from Mozilla Firefox Nightly 56.0a1 (2017-07-04).
Thanks.

Comment 18

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a3b192dc8344679ce208af42b6246c3c0d42cab3&newProject=mozilla-central&newRev=53a5192b436827f667db9df036b07d245e35d97a