1387609 - "Close tab" button icon in the tab bar are too dark after landing patch from bug #1385702

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

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

Similar to bug #1377227 & bug #1384686


(Tim Nguyen :ntim in bug #1384686 comment #3 wrote)
> The spec says icons shouldn't be full black:
> http://design.firefox.com/photon/visual/icons.html#color


"Speedy" Regression window (mozilla-central)
Good:
https://ftp.mozilla.org/pub/firefox/nightly/2017/08/2017-08-03-10-03-52-mozilla-central/

Bad:
https://ftp.mozilla.org/pub/firefox/nightly/2017/08/2017-08-04-10-03-54-mozilla-central/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=63e261ce8cb04c913d2e6b19ea451b7078d24dc1&tochange=32083f24a1bb2c33050b4c972783f066432194eb

Probably caused by:
Bug #1372689 - Update tab strip button icons

Comment 1

[Dão Gottwald [:dao]]

What button is this bug talking about?

Comment 2

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

This bug is about "close tab" button, as stated in this bug summary.



(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #0)
> Probably caused by:
> Bug #1372689 - Update tab strip button icons

It should be:
Probably caused by:
Bug 1385702 - Update close icon style for photon

Comment 3

[Tim Nguyen :ntim]

This should be the same fix as bug 1384686.

Comment 4

[Tim Nguyen :ntim]

Tabbar icons should use var(--toolbarbutton-icon-fill) like navigation bar icons.

Comment 5

[Dão Gottwald [:dao]]

> This bug is about "close tab" button, as stated in this bug summary.

The summary mentions the location bar, which doesn't seem to make any sense.

Tab close buttons use the same color as tab labels, as specified here: http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html

The default text color is however lighter than what Windows uses by default.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #4)
> Tabbar icons should use var(--toolbarbutton-icon-fill) like navigation bar
> icons.

No, they shouldn't.

Comment 7

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

(In reply to Dão Gottwald [::dao] from comment #5)
> > This bug is about "close tab" button, as stated in this bug summary.
> 
> The summary mentions the location bar, which doesn't seem to make any sense.

Valid point. Fixing summary.

Comment 8

[Dão Gottwald [:dao]]

Attachment: Bug 1387609 - Use --toolbarbutton-icon-fill-opacity for the tab close button.

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

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910732 [details]
Bug 1387609 - Use --toolbarbutton-icon-fill-opacity for the tab close button.

https://reviewboard.mozilla.org/r/182188/#review187868

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a0e5139dc99
Use --toolbarbutton-icon-fill-opacity for the tab close button. r=nhnt11

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Note for future:

I was a little confused why we were using stroke-opacity for the fill-opacity, but Dão clarified on IRC that it's because we're already using context-fill-opacity for the background behind the X. So as a workaround, we use stroke-opacity to be able to set a different value for the fill-opacity of the X.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/6a0e5139dc99

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8910732 [details]
Bug 1387609 - Use --toolbarbutton-icon-fill-opacity for the tab close button.

Approval Request Comment
[Feature/Bug causing the regression]: photon-visual polish
[User impact if declined]: tab close button icon color is inconsistent with other toolbar buttons
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: trivial fix
[String changes made/needed]:

Comment 14

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

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 58.0a1 (2017-09-23), so I'm marking this bug as VERIFIED.
Thank you very much! \o/

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910732 [details]
Bug 1387609 - Use --toolbarbutton-icon-fill-opacity for the tab close button.


Polish photon, taking it
should be in 57b3

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ae054da94c28