Text in multiselected tabs hard to read on dark frame |
|||||||||||||||||||
Issue description
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/633a77ea42c1d12705d06c28c321865110645c31 commit 633a77ea42c1d12705d06c28c321865110645c31 Author: Allen Bauer <kylixrd@chromium.org> Date: Fri Aug 24 21:28:05 2018 Take the selected state of the tab into account for the text color. Apply the tab selected state color into account in order to calculate the proper contrast for the tab's text and icons. Bug: 876794 Change-Id: Ie0db7e6eb5cd93c7b7d34098c88e9f06ff355d4d Reviewed-on: https://chromium-review.googlesource.com/1188434 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#585996} [modify] https://crrev.com/633a77ea42c1d12705d06c28c321865110645c31/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/633a77ea42c1d12705d06c28c321865110645c31/chrome/browser/ui/views/tabs/tab.h [modify] https://crrev.com/633a77ea42c1d12705d06c28c321865110645c31/chrome/browser/ui/views/tabs/tab_strip.cc
,
Aug 24
This is, I hope, the last 69 merge request from my subteam :) Justification for this is that tab text is unreadable in selected tabs for some frame colors (see comment 0 screenshot). Obviously we'll want to wait until the Canary build with this to verify it before merging.
,
Aug 24
This bug requires manual review: We are only 10 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 24
Pls update bug with canary result on Monday morning.
,
Aug 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9dbbdadf3bb9cf09b1aea8544e06007e43751143 commit 9dbbdadf3bb9cf09b1aea8544e06007e43751143 Author: Allen Bauer <kylixrd@chromium.org> Date: Sat Aug 25 16:39:13 2018 Take the selected state of the tab into account for the text color. Apply the tab selected state color into account in order to calculate the proper contrast for the tab's text and icons. Bug: 876794 Change-Id: Ie0db7e6eb5cd93c7b7d34098c88e9f06ff355d4d Reviewed-on: https://chromium-review.googlesource.com/1188434 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#585996}(cherry picked from commit 633a77ea42c1d12705d06c28c321865110645c31) Reviewed-on: https://chromium-review.googlesource.com/1188833 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3532@{#7} Cr-Branched-From: cae5f8710a9652a6f1716812fbedfdd59fced679-refs/heads/master@{#585632} [modify] https://crrev.com/9dbbdadf3bb9cf09b1aea8544e06007e43751143/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/9dbbdadf3bb9cf09b1aea8544e06007e43751143/chrome/browser/ui/views/tabs/tab.h [modify] https://crrev.com/9dbbdadf3bb9cf09b1aea8544e06007e43751143/chrome/browser/ui/views/tabs/tab_strip.cc
,
Aug 25
Last night canary failed to trigger, pls see bug 876964 for details. I merged the change to canary branch 3532 at #6 and triggering new canary so we can have canary coverage over the weekend.
,
Aug 25
Canary version 70.0.3532.5 currently building.
,
Aug 27
The NextAction date has arrived: 2018-08-27
,
Aug 27
Tried to reproduce the issue on Windows 10 and Mac OS 10.13.3 on the build without fix 70.0.3521.0 and unable to reproduce the issue by following the below steps. 1. Launched Chrome and navigated to chrome webstore. 2. Added few dark themes, selected multiple tabs and could read the text on the tab strip without any issues. kylixrd@ Request you to provide the theme where this issue can be reproduced and help us in verifying the fix on the latest M-70 build. Thanks..
,
Aug 27
This isn't fixed. Testing at home gives the screenshot attached, where the text is white on #d8ded8.
Looking at the patch, this line seems wrong:
GetBlendValueWithMinimumContrast(
tab_bg_color, title_color, tab_inactive_bg_color,
kMinimumInactiveContrastRatio);
Here |tab_bg_color| is the selected background, #d8ded8. |title_color| is the nominal foreground color, #ffffff. |tab_inactive_bg_color| is the unselected tab background color, #647c64. So we're asking to blend #d8ded8 towards #ffffff until it contrasts sufficiently with #647c64. That isn't right. Clearly I didn't think about this code while reviewing it, just glanced at it. :P
Instead we want to be blending #ffffff towards the opposite luma of #d8ded8 until it contrasts sufficiently with #d8ded8. I think "sufficiently" can be different for title and close X -- for titles we could just use GetColorWithMinimumContrast() to get to 4.5, for close X we should use the custom ratios in the function. So the calculation of |tab_bg_color| is correct, but then the tab title color should be GetColorWithMinimumContrast(title_color, tab_bg_color), and the close button color would be based on that.
This code needs tests :P
,
Aug 27
Rejecting merge to M69 based on comment #11. Pls re-request a merge when change is safe to merge. Thank you.
,
Aug 27
I'd actually thought about this issue over the weekend. One of those, "Oh wait! I think that was wrong." moments. Will reopen and investigate.
,
Aug 27
Tentatively tagging as "RBS" as per kylixrd@, this is important for M69.
,
Aug 27
Ok, here's what it looks like with the suggested changes. First image is the current Canary. The second is from a local build with the patch. This looks much better... IMO
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5003a6a5a2f790a12d878e84b59e90fd8b8d7050 commit 5003a6a5a2f790a12d878e84b59e90fd8b8d7050 Author: Allen Bauer <kylixrd@chromium.org> Date: Mon Aug 27 19:09:03 2018 Use GetColorWithMinimumContrast instead of manually calculating a contrasting color. The previous patch attempting to fix this, https://crrev.com/c/1188434, didn't blend in the right direction. It got the background color calculation correct, but the title color calculation wasn't right. This change just uses GetColorWithMinimumContrast directly so that the text (and the close button icon) has sufficient contrast on a selected inactive tab. Bug: 876794 Change-Id: I8ee0a8f2a38aac60c14d104fbf75c3ef85304720 Reviewed-on: https://chromium-review.googlesource.com/1191189 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Allen Bauer <kylixrd@chromium.org> Cr-Commit-Position: refs/heads/master@{#586359} [modify] https://crrev.com/5003a6a5a2f790a12d878e84b59e90fd8b8d7050/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/5003a6a5a2f790a12d878e84b59e90fd8b8d7050/chrome/browser/ui/views/tabs/tab.h
,
Aug 27
Pls update bug with canary result tomorrow morning.
,
Aug 27
,
Aug 27
This bug requires manual review: We are only 7 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 27
Pls merge the change listed at #16 to canary branch #3534. So we can trigger new canary from same branch.
,
Aug 27
Merge to canary branch 3534 is here - https://chromium.googlesource.com/chromium/src.git/+/bbfaa73569a1a7491bcefbdc84749f2148830132
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bbfaa73569a1a7491bcefbdc84749f2148830132 commit bbfaa73569a1a7491bcefbdc84749f2148830132 Author: Allen Bauer <kylixrd@chromium.org> Date: Mon Aug 27 22:35:38 2018 Use GetColorWithMinimumContrast instead of manually calculating a contrasting color. The previous patch attempting to fix this, https://crrev.com/c/1188434, didn't blend in the right direction. It got the background color calculation correct, but the title color calculation wasn't right. This change just uses GetColorWithMinimumContrast directly so that the text (and the close button icon) has sufficient contrast on a selected inactive tab. Bug: 876794 Change-Id: I8ee0a8f2a38aac60c14d104fbf75c3ef85304720 Reviewed-on: https://chromium-review.googlesource.com/1191189 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Allen Bauer <kylixrd@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#586359}(cherry picked from commit 5003a6a5a2f790a12d878e84b59e90fd8b8d7050) Reviewed-on: https://chromium-review.googlesource.com/1192108 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3534@{#7} Cr-Branched-From: 5c1309f3276665c501e62f79a3bfb9244b3dca26-refs/heads/master@{#586175} [modify] https://crrev.com/bbfaa73569a1a7491bcefbdc84749f2148830132/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/bbfaa73569a1a7491bcefbdc84749f2148830132/chrome/browser/ui/views/tabs/tab.h
,
Aug 28
The NextAction date has arrived: 2018-08-28
,
Aug 28
Able to reproduce this issue on Windows 10, Mac OS 10.13.3 and Ubuntu 17.10 on the build without fix 70.0.3521.0 and the issue is fixed on the latest M-70 build 70.0.3534.4 by following the below steps. 1. Launched Chrome and on Windows changed the Windows color to dark frame in Settings -> Colors. 2. Selected multiple tabs and could read the text on the tab strip without any issues. On Mac OS and Ubuntu 17.10, added the theme "Isle of Skye Scotland", selected multiple tabs and could read the text on the tab strip without any issues. Attached are the screen shots for reference. Hence adding TE verified labels as the fix is working as intended. Thanks..
,
Aug 28
,
Aug 28
Approving merge to M69 branch 3497 based on comment #24. Please merge ASAP. Thank you.
,
Aug 28
,
Aug 28
M69 merge is approved at #26.
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f932b8a626da06fc538acf147675fc5c95124f9 commit 7f932b8a626da06fc538acf147675fc5c95124f9 Author: Allen Bauer <kylixrd@chromium.org> Date: Tue Aug 28 17:44:30 2018 Take the selected state of the tab into account for the text color. Apply the tab selected state color into account in order to calculate the proper contrast for the tab's text and icons. Bug: 876794 Change-Id: Ie0db7e6eb5cd93c7b7d34098c88e9f06ff355d4d Reviewed-on: https://chromium-review.googlesource.com/1188434 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#585996}(cherry picked from commit 633a77ea42c1d12705d06c28c321865110645c31) Reviewed-on: https://chromium-review.googlesource.com/1194290 Reviewed-by: Allen Bauer <kylixrd@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#828} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/7f932b8a626da06fc538acf147675fc5c95124f9/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/7f932b8a626da06fc538acf147675fc5c95124f9/chrome/browser/ui/views/tabs/tab.h [modify] https://crrev.com/7f932b8a626da06fc538acf147675fc5c95124f9/chrome/browser/ui/views/tabs/tab_strip.cc
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b7b3b3b508f714f1af75ee8f563a6c90a565e867 commit b7b3b3b508f714f1af75ee8f563a6c90a565e867 Author: Allen Bauer <kylixrd@chromium.org> Date: Tue Aug 28 17:45:10 2018 Use GetColorWithMinimumContrast instead of manually calculating a contrasting color. The previous patch attempting to fix this, https://crrev.com/c/1188434, didn't blend in the right direction. It got the background color calculation correct, but the title color calculation wasn't right. This change just uses GetColorWithMinimumContrast directly so that the text (and the close button icon) has sufficient contrast on a selected inactive tab. Bug: 876794 Change-Id: I8ee0a8f2a38aac60c14d104fbf75c3ef85304720 Reviewed-on: https://chromium-review.googlesource.com/1191189 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Allen Bauer <kylixrd@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#586359}(cherry picked from commit 5003a6a5a2f790a12d878e84b59e90fd8b8d7050) Reviewed-on: https://chromium-review.googlesource.com/1194291 Reviewed-by: Allen Bauer <kylixrd@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#829} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/b7b3b3b508f714f1af75ee8f563a6c90a565e867/chrome/browser/ui/views/tabs/tab.cc [modify] https://crrev.com/b7b3b3b508f714f1af75ee8f563a6c90a565e867/chrome/browser/ui/views/tabs/tab.h
,
Aug 29
Able to reproduce this issue on Windows 10, Mac OS 10.13.3 and Ubuntu 17.10 on the build without fix 70.0.3521.0 and the issue is fixed on the latest M-69 build 69.0.3497.72 by following the below steps. 1. Launched Chrome and on Windows changed the Windows color to dark frame in Settings -> Colors. 2. Selected multiple tabs and could read the text on the tab strip without any issues. On Mac OS and Ubuntu 17.10, added the theme "Isle of Skye Scotland", selected multiple tabs and could read the text on the tab strip without any issues. Attached are the screen shots for reference. Hence adding TE verified labels as the fix is working as intended. Thanks.. |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by robliao@chromium.org
, Aug 22