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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
2 months ago
20 days ago

People

(Reporter: Virtual, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {nightly-community, ux-consistency})

56 Branch
Firefox 56
x86_64
Windows 7
nightly-community, ux-consistency
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Created attachment 8882236 [details]
Bookmark star icon.png

Bookmark star icon in doorhanger/panelUI menu should be black to be consistent with other icons.
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Whiteboard: [photon][triage] → [photon] [triage]
(Assignee)

Comment 1

2 months ago
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 )...
Flags: needinfo?(shorlander)
Whiteboard: [photon] [triage] → [photon-structure] [triage]

Comment 2

2 months ago
at the table today shorlander said it should be filled in blue as it was before.
(Assignee)

Updated

2 months ago
Blocks: 1352120
Summary: Bookmark star icon in doorhanger/panelUI menu should be black to be consistent with other icons → Bookmark star icon in location bar should be blue (not black) when filled in
Whiteboard: [photon-structure] [triage] → [photon-structure]

Updated

2 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly

Comment 3

2 months ago
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
status-firefox56: --- → affected
(Assignee)

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

2 months ago
Iteration: --- → 56.2 - Jul 10
Flags: needinfo?(shorlander)
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 months ago
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
(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
(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

2 months ago
mozreview-review
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>.
Attachment #8882605 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 months ago
mozreview-review
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?
Attachment #8882605 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 12

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

Comment 13

2 months ago
Also filed bug 1377537 to move the zoom button CSS.
(Assignee)

Updated

2 months ago
Blocks: 1377202
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 14

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

2 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d089acf3ff1a
use correct fill colours and opacity for in-urlbar icons, r=dao

Comment 16

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d089acf3ff1a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1377227
I'm marking this bug as VERIFIED, as issue is fixed, starting from Mozilla Firefox Nightly 56.0a1 (2017-07-04).
Thanks.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified

Updated

2 months ago
Flags: qe-verify+
Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a3b192dc8344679ce208af42b6246c3c0d42cab3&newProject=mozilla-central&newRev=53a5192b436827f667db9df036b07d245e35d97a
Attachment #8882605 - Flags: review?(shorlander)
Version: Trunk → 56 Branch
QA Contact: gwimberly → Virtual
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.