Background tab text forced to too high of contrast |
|||||||||
Issue descriptionChrome Version : 70.0.3533.0 Install the theme at https://github.com/Fiddle-N/material-deep-black-theme/tree/md2-alt . In Dev background tab text is (150, 150, 150) like it's supposed to be. On Canary it's about (224, 224, 224). I suspect https://chromium-review.googlesource.com/c/chromium/src/+/1173491 . The specified text color was high-enough contrast with the tab color already, it didn't need to be forced higher, so this is a bug. Considering high-priority since the change that likely caused this was merged to the M69 branch.
,
Aug 28
I've been unable to reproduce this in today's (70.0.3535.0) or yesterday's (70.0.3534.0) Canary build. Is anyone else able to reproduce this? I suspect you may be right that it was related to another issue, which seems to have been resolved.
,
Aug 28
Can you repro on 3533? If so we can verify that Chrome actually got fixed and it wasn't just an issue of your machine and mine differing. If we can verify that, then we could try to bisect/back out a few things locally to confirm which change caused/fixed this and whether they're in M69.
,
Aug 28
Ah - I was using the version from the web store, which I did not realize is different from the GitHub version. I'm now able to repro this locally - I'll investigate and see if I can figure out what's going on.
,
Aug 29
Figured out what's going on with this - it was snagging on an edge case of the background tab text contrast CL (as pkasting initially suspected). CL 1195818 is in progress to fix this.
,
Aug 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e6328219520d9e2f3a96aa22236e50cc8008b11 commit 2e6328219520d9e2f3a96aa22236e50cc8008b11 Author: Ryan Meier <rameier@chromium.org> Date: Thu Aug 30 01:42:18 2018 Apply background tab text contrast adjustment to themes that don't specify a background tab color or image. Fixes a corner case where the background tab text contrast adjustments (CL 1173491) would not be applied if the theme didn't specify a background tab color or image. The processing now falls back to the frame color to blend against in this case. Bug: 877853 Change-Id: Ibad260a17369342067ff8088eb0215ab54af2b10 Reviewed-on: https://chromium-review.googlesource.com/1195818 Commit-Queue: Ryan Meier <rameier@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#587397} [modify] https://crrev.com/2e6328219520d9e2f3a96aa22236e50cc8008b11/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/2e6328219520d9e2f3a96aa22236e50cc8008b11/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/2e6328219520d9e2f3a96aa22236e50cc8008b11/chrome/browser/themes/browser_theme_pack_unittest.cc [add] https://crrev.com/2e6328219520d9e2f3a96aa22236e50cc8008b11/chrome/test/data/extensions/theme_test_bgtabtext_notabcolor_singletextcolor/manifest.json [add] https://crrev.com/2e6328219520d9e2f3a96aa22236e50cc8008b11/chrome/test/data/extensions/theme_test_bgtabtext_notabcolor_singletextcolor_autoassign/manifest.json
,
Aug 30
,
Aug 30
Requesting a merge, though I don't know if the ship has sailed.
,
Aug 30
This bug requires manual review: We are only 4 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 30
We're cutting M69 stable RC soon, how critical and safe is this merge for M69 this late in release cycle?
,
Aug 30
rameier@, kylixrd@, estade@, could you ptal comment #10 as pkasting@ is OOO. Thank you.
,
Aug 30
Should be very safe. As for necessary, we could ship without it if we absolutely have to, but it's a regression in M69 if we do, so I'd like to avoid it if possible.
,
Aug 30
Approving merge to M69 branch 3497 based on comment #12. Please merge ASAP. Also is this change verified in canary?
,
Aug 30
Yes, I just checked it with a number of themes and it seems to be working properly in today's Canary (70.0.3537.0).
,
Aug 30
@14: How did you check against 3537 when that cut at 587302 and your CL didn't land until after that?
,
Aug 30
Per discussion with govind@, merged this. Ryan, can you verify against that github theme on tomorrow's canary (and confirm today's is indeed broken, since it doesn't have your fix)?
,
Aug 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15985b5a39705df096529f673ae2f6f0ce8c1fb9 commit 15985b5a39705df096529f673ae2f6f0ce8c1fb9 Author: Ryan Meier <rameier@chromium.org> Date: Thu Aug 30 19:56:29 2018 Apply background tab text contrast adjustment to themes that don't specify a background tab color or image. Fixes a corner case where the background tab text contrast adjustments (CL 1173491) would not be applied if the theme didn't specify a background tab color or image. The processing now falls back to the frame color to blend against in this case. Bug: 877853 Change-Id: Ibad260a17369342067ff8088eb0215ab54af2b10 Reviewed-on: https://chromium-review.googlesource.com/1195818 Commit-Queue: Ryan Meier <rameier@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#587397}(cherry picked from commit 2e6328219520d9e2f3a96aa22236e50cc8008b11) Reviewed-on: https://chromium-review.googlesource.com/1196772 Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#850} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/15985b5a39705df096529f673ae2f6f0ce8c1fb9/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/15985b5a39705df096529f673ae2f6f0ce8c1fb9/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/15985b5a39705df096529f673ae2f6f0ce8c1fb9/chrome/browser/themes/browser_theme_pack_unittest.cc [add] https://crrev.com/15985b5a39705df096529f673ae2f6f0ce8c1fb9/chrome/test/data/extensions/theme_test_bgtabtext_notabcolor_singletextcolor/manifest.json [add] https://crrev.com/15985b5a39705df096529f673ae2f6f0ce8c1fb9/chrome/test/data/extensions/theme_test_bgtabtext_notabcolor_singletextcolor_autoassign/manifest.json
,
Aug 30
Per chat with pkasting@, this should be a safe merge and pasking@ to verify this change on canary tomorrow.
,
Aug 30
@15/16: Whoops - yeah that's my bad. Cached theme data made it seem as though it was fixed, but the change has in fact not landed on Canary yet. Will confirm in tomorrow morning's build (and delete the cached data before I do).
,
Aug 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d5b999d429a3b8369f7f8e85d628245c8cea824 commit 2d5b999d429a3b8369f7f8e85d628245c8cea824 Author: Ryan Meier <rameier@chromium.org> Date: Thu Aug 30 21:28:12 2018 Apply background tab text contrast adjustment to themes that don't specify a background tab color or image. Fixes a corner case where the background tab text contrast adjustments (CL 1173491) would not be applied if the theme didn't specify a background tab color or image. The processing now falls back to the frame color to blend against in this case. Bug: 877853 Change-Id: Ibad260a17369342067ff8088eb0215ab54af2b10 Reviewed-on: https://chromium-review.googlesource.com/1195818 Commit-Queue: Ryan Meier <rameier@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#587397}(cherry picked from commit 2e6328219520d9e2f3a96aa22236e50cc8008b11) Reviewed-on: https://chromium-review.googlesource.com/1197783 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3537@{#4} Cr-Branched-From: 2517229df1c255626faa9309277b19bf79ad0b3d-refs/heads/master@{#587302} [modify] https://crrev.com/2d5b999d429a3b8369f7f8e85d628245c8cea824/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/2d5b999d429a3b8369f7f8e85d628245c8cea824/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/2d5b999d429a3b8369f7f8e85d628245c8cea824/chrome/browser/themes/browser_theme_pack_unittest.cc [add] https://crrev.com/2d5b999d429a3b8369f7f8e85d628245c8cea824/chrome/test/data/extensions/theme_test_bgtabtext_notabcolor_singletextcolor/manifest.json [add] https://crrev.com/2d5b999d429a3b8369f7f8e85d628245c8cea824/chrome/test/data/extensions/theme_test_bgtabtext_notabcolor_singletextcolor_autoassign/manifest.json
,
Aug 30
Pls verify this bug on canary version 70.0.3537.2 or higher.
,
Aug 31
The NextAction date has arrived: 2018-08-31
,
Aug 31
Verified on today's Canary (70.0.3538.0) using the GitHub version of the theme from the initial bug.
,
Aug 31
Thank you rameier@ for canary verification. Requesting to verify on M69 build #69.0.3497.75. Thank you.
,
Aug 31
Verified in build 69.0.3497.75
,
Aug 31
Thank you so much rameier@. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by pkasting@chromium.org
, Aug 27