Close buttons wrong color on dark frames |
|||||||||
Issue descriptionhttps://chromium-review.googlesource.com/c/chromium/src/+/1145590 regressed tab close button colors. On a dark frame, the close button is a dark grey that's unreadable. The specific problem is https://chromium-review.googlesource.com/c/chromium/src/+/1145590/14/chrome/browser/ui/views/tabs/tab.cc#1706 , which broadened the use of GetCloseTabButtonColor() from "touch UI" to "all newer material", perhaps accidentally. GetCloseTabButtonColor() effectively returns fixed, hardcoded colors. We want to use |button_color_|, which is based off the text color. While returning this conditional to being based on MD::IsTouchOptimizedUiEnabled() would address the regression, I think GetCloseTabButtonColor() is probably wrong for touchable refresh too, so we should restrict it to just touchable pre-refresh. (More work needs to be done on the computed button color, hover/pressed color, etc., but this bug doesn't cover that.)
,
Jul 30
Tested this issue on Windows 10 on the build without fix 70.0.3503.0 and unable to reproduce the issue by following the below steps. Launched Chrome and added few dark themes from Chrome webstore and couldn't find any dark grey close button which is unreadable. Attached is the screen cast for reference. kylixrd@ Request you to check and confirm if anything is missed from our end in verifying the issue? Thanks..
,
Jul 30
,
Jul 30
,
Jul 31
Merge requested - This was a regression introduced right before the M69 branch.
,
Jul 31
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact 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
,
Jul 31
Merge should be abandoned. The regression happened post-M69 branch and was subsequently fixed on the trunk. The issue isn't present on the branch.
,
Jul 31
Rejecting merge to M69 per comment #7.
,
Aug 15
Actually we do need to merge this, since we now merged the regression CL. Merging based on prior auto approval in comment 6.
,
Aug 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f3a18f9491330d76058b2de0981cf97896286c30 commit f3a18f9491330d76058b2de0981cf97896286c30 Author: Allen Bauer <kylixrd@chromium.org> Date: Wed Aug 15 15:44:57 2018 Only use GetCloseTabButtonColor() for pre-refresh touchable. Bug: 868169 Change-Id: I9adeed2a4fd8d57d3ae26e7885f66eb3bcfcd85d Reviewed-on: https://chromium-review.googlesource.com/1153581 Commit-Queue: Allen Bauer <kylixrd@chromium.org> Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578769}(cherry picked from commit ddfb2b7ee75c24b9105c953375405a66d1b64215) Reviewed-on: https://chromium-review.googlesource.com/1174773 Cr-Commit-Position: refs/branch-heads/3497@{#639} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/f3a18f9491330d76058b2de0981cf97896286c30/chrome/browser/ui/views/tabs/tab.cc
,
Aug 16
Tested this issue on Windows 10 on the build without fix 69.0.3497.12 and unable to reproduce the issue by following the below steps. Launched Chrome and added few dark themes from Chrome webstore and couldn't find any dark grey close button which is unreadable. Attached is the screen shot for reference. kylixrd@ Request you to check and confirm if anything is missed from our end in verifying the issue? Thanks..
,
Aug 16
You're good -- that screenshot verifies that you're not suffering from the issue. There shouldn't be a beta build that _does_ suffer from it, as it was a brief regression from https://chromium-review.googlesource.com/c/chromium/src/+/1145590 , and we merged both that and the fix here at the same time.
,
Aug 16
Thank you Peter, marking the bug as TE-Verified based on Comments#11 and 12. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Jul 27