New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 877853 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: Aug 30
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-08-31
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Background tab text forced to too high of contrast

Project Member Reported by pkasting@chromium.org, Aug 26

Issue description

Chrome 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.
 
Cc: kylixrd@chromium.org
Wonder if this was caused by https://chromium-review.googlesource.com/c/chromium/src/+/1188434 , which seems to have done the wrong thing; further writeup coming soon on  bug 876794 .
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.
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.
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.
Status: Started (was: Assigned)
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: Merge-Request-69
Requesting a merge, though I don't know if the ship has sailed.
Project Member

Comment 9 by sheriffbot@chromium.org, Aug 30

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
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
We're cutting M69 stable RC soon, how critical and safe is this merge for M69 this late in release cycle?
rameier@, kylixrd@, estade@, could you ptal comment #10 as pkasting@ is OOO. Thank you.
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.
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #12. Please merge ASAP. 

Also is this change verified in canary?
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).
@14: How did you check against 3537 when that cut at 587302 and your CL didn't land until after that?
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)?
Project Member

Comment 17 by bugdroid1@chromium.org, Aug 30

Labels: -merge-approved-69 merge-merged-3497
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

NextAction: 2018-08-31
Per chat with pkasting@, this should be a safe merge and pasking@ to verify this change on canary tomorrow.
@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).
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 30

Labels: merge-merged-3537
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

Pls verify this bug on canary version 70.0.3537.2 or higher.
The NextAction date has arrived: 2018-08-31
Verified on today's Canary (70.0.3538.0) using the GitHub version of the theme from the initial bug.
Thank you rameier@ for canary verification.

Requesting to verify on M69 build #69.0.3497.75. Thank you.


Verified in build 69.0.3497.75
Thank you so much rameier@.

Sign in to add a comment