Version 72.0.3583.0 (Official Build) canary (64-bit)

1. save a card 
2. The secondary text color should be GG700 instead of GG900 
Check the image for details 

Deleted: screen.png 523 KB

	screen.png 523 KB View Download

The dropdown was affected as well. 

Deleted: downstream.png 30.5 KB

	downstream.png 30.5 KB View Download

Hi Rui, I notice this is a P1 bug, are we targeting this for M71? And maybe also worth noting the platforms this piece is affecting?

Hi this is probably because there is no STYLE_SECONDARY in GetHarmonyTextColorForNonStandardNativeTheme() in chrome_typography_provider.

Assigned to owner. And added target M71. 

Hi Allen, can you take a look at this and confirm this is a bug? I believe the secondary text in harmony should be in Google Grey 700.

Able to reproduce the issue on chrome version 72.0.3583.0 and on the latest canary 72.0.3589.0 using Mac 10.13.1 and Ubuntu 14.04
Note: Issue isn't seen on Windows 10.

Bisect Information:
-------------------
Good Build: 72.0.3582.0
Bad Build:  72.0.3583.0

Providing the change log from Omahaproxy as we couldn't perform per-revision bisect due to Insufficient builds and Chromium bisect invoked all good builds.
https://chromium.googlesource.com/chromium/src/+log/72.0.3582.0..72.0.3583.0?pretty=fuller&n=10000
Suspecting: https://chromium-review.googlesource.com/c/chromium/src/+/1282037

Adding RBS as this seems to be a recent regression, please change/remove if not needed.

Thanks!

Friendly ping to get an update as it is marked as RBS.
Thanks..!

M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

bsep@: could you please take a look at this. It's affecting multiple surfaces (Autofill dropdown, save card bubble), and we think something changed on the Views side.

This is affecting M71 as well (found in 71.0.3578.20), not sure why bisect says 72.0.3582.0 is a good build.

Hmm, this doesn't seem bad enough to RBS, but I'll try to fix and merge anyway since Beta started recently.

Views isn't used on mobile platforms, unchecking those. If autofill is affected there too it's a separate bug.

I don't know how to summon the paradise dialogs, please add invocations steps for them so I can test a fix. Also, you really should have added DialogBrowserTest for them a long time ago.

Well just want to mention the suspecting CL was merged to 71.0.3578.20 on 10/22, but I don't see why it breaks the text color of all autofill UIs.

bsep@: Please take a look at the test plan for the invocations steps: go/ib-paradise-test-plan. 

Delegating to pbos@

Culprit is r598108 which made primary text color GG900 (correct) for the common theme. This breaks the theme.GetSystemColor(kTestColorId) != SK_ColorBLACK check inside ShouldIgnoreHarmonySpec.

This commit is in 71.0.3576.0.

I'll do a CL that updates this ugly check to "black or gg900".

Thanks for working on this issue!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8c0c549ca867aa69cdfa5865d2f45504256cee0

commit b8c0c549ca867aa69cdfa5865d2f45504256cee0
Author: Peter Boström <pbos@chromium.org>
Date: Wed Oct 31 02:00:01 2018

Make Harmony colors apply by default for all OSes

r598108 made GG900 primary text color in GetAuraColor / common_theme.cc
which made the ShouldIgnoreHarmonySpec check (kColorId_LabelEnabledColor
== SK_ColorBLACK) fail. This caused the Harmony colors to be ignored
for OSes outside Windows that use the default aura colors.

This change meant that GetHarmonyTextColorForNonStandardNativeTheme was
pretty much always used for colors, instead of using the Harmony colors.

Bug:  chromium:896891 
Change-Id: I394e25d2eda5da5c0bb0532056d6eebff2a855c2
Reviewed-on: https://chromium-review.googlesource.com/c/1308901
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604109}
[modify] https://crrev.com/b8c0c549ca867aa69cdfa5865d2f45504256cee0/chrome/browser/ui/views/chrome_typography_provider.cc

Hi pbos@ can you please merge this fix into M71? The issue would affect several UIs and is blocking UI launch review. Thanks!

Able to reproduce the issue on chrome version 72.0.3583.0 (build without fix) as per the comment #4.
Verified the fix on Mac 10.13.5  using Chrome version # 72.0.3598.0 
Attaching screenshot for reference.
Observed that  "The secondary text color GG700 " 
This is working as expected, adding Verified labels

Thanks...!

Deleted: 896891.png 22.9 KB

	896891.png 22.9 KB View Download

Deleted: 896891-1.png 22.0 KB

	896891-1.png 22.0 KB View Download

Updating the above comment #22, Verified the fix on Mac 10.13.6 and Ubuntu 17.10 using Chrome version # 72.0.3598.0. 

Due to issue ( https://bugs.chromium.org/p/monorail/issues/detail?id=4496 ), got deleted verified labels, Readding the verified labels.

Thanks, I wanted it verified in Canary before asking for a merge.

Gross, looks like that bug is hitting me too.

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Justification: The change is low risk, verified in Canary and shows a pretty serious visual regression on Mac.

Approving merge to M71 branch 3578 based on comment #28. 

Pls merge your change to M71 branch 3578 ASAP so we can pick it up for next beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/910f81b88b18310ff617885e4dd236710cfe2f97

commit 910f81b88b18310ff617885e4dd236710cfe2f97
Author: Peter Boström <pbos@chromium.org>
Date: Fri Nov 02 17:31:14 2018

Make Harmony colors apply by default for all OSes

r598108 made GG900 primary text color in GetAuraColor / common_theme.cc
which made the ShouldIgnoreHarmonySpec check (kColorId_LabelEnabledColor
== SK_ColorBLACK) fail. This caused the Harmony colors to be ignored
for OSes outside Windows that use the default aura colors.

This change meant that GetHarmonyTextColorForNonStandardNativeTheme was
pretty much always used for colors, instead of using the Harmony colors.

Bug:  chromium:896891 
Change-Id: I394e25d2eda5da5c0bb0532056d6eebff2a855c2
Reviewed-on: https://chromium-review.googlesource.com/c/1308901
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604109}(cherry picked from commit b8c0c549ca867aa69cdfa5865d2f45504256cee0)
Reviewed-on: https://chromium-review.googlesource.com/c/1315362
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#473}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/910f81b88b18310ff617885e4dd236710cfe2f97/chrome/browser/ui/views/chrome_typography_provider.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/910f81b88b18310ff617885e4dd236710cfe2f97

Commit: 910f81b88b18310ff617885e4dd236710cfe2f97
Author: pbos@chromium.org
Commiter: pbos@chromium.org
Date: 2018-11-02 17:31:14 +0000 UTC

Make Harmony colors apply by default for all OSes

r598108 made GG900 primary text color in GetAuraColor / common_theme.cc
which made the ShouldIgnoreHarmonySpec check (kColorId_LabelEnabledColor
== SK_ColorBLACK) fail. This caused the Harmony colors to be ignored
for OSes outside Windows that use the default aura colors.

This change meant that GetHarmonyTextColorForNonStandardNativeTheme was
pretty much always used for colors, instead of using the Harmony colors.

Bug:  chromium:896891 
Change-Id: I394e25d2eda5da5c0bb0532056d6eebff2a855c2
Reviewed-on: https://chromium-review.googlesource.com/c/1308901
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604109}(cherry picked from commit b8c0c549ca867aa69cdfa5865d2f45504256cee0)
Reviewed-on: https://chromium-review.googlesource.com/c/1315362
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#473}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Unexpectedly deleted above comment #35, Updating again...

Verified the fix on Mac 10.13.6 and Ubuntu 17.10 using Chrome version # 71.0.3578.44. 
Attaching screenshot for reference.
Observed that  "The secondary text color GG700 " 
This is working as expected, adding Verified labels

Thanks...!

Deleted: 896891 (1).png 13.8 KB

	896891 (1).png 13.8 KB View Download