Closed Bug 1377165 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect, P1)

56 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- verified

People

(Reporter: Virtual, Assigned: Gijs)

References

Details

(Keywords: nightly-community, ux-consistency, Whiteboard: [photon-structure])

Attachments

(2 files)

Attached image 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]
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]
at the table today shorlander said it should be filled in blue as it was before.
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]
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
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
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Flags: needinfo?(shorlander)
Priority: P2 → P1
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 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 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+
(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.
Also filed bug 1377537 to move the zoom button CSS.
Blocks: 1377202
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.
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
https://hg.mozilla.org/mozilla-central/rev/d089acf3ff1a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.