Desktop PWAs: Title bar foreground color contrast too low |
|||||||
Issue descriptionChrome Version: 66.0.3344.0 OS: Chrome What steps will reproduce the problem? (1) Install an app with theme_color #f16c00 (i.e., the Google Play Music theme colour). (2) Open the app window. What is the expected result? The colour of the title bar buttons is high contrast. What happens instead? The colour of the title bar buttons is #5a5a5a. This doesn't contrast that well. See screenshot. Note: We carefully checked the implementation to match against the material design spec. But we can look again and see if there is a discrepancy with Chrome on Android. Links: - Android title bar color calculation: https://codereview.chromium.org/1956803002 - Dark foreground threshold: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/util/ColorUtils.java?l=19
,
Mar 20 2018
The colour used by the buttons and origin text comes from: https://cs.chromium.org/chromium/src/ash/frame/caption_buttons/frame_caption_button.cc?type=cs&q=FrameCaptionButton::GetButtonColor&sq=package:chromium&l=59 This is a simple light/dark check that doesn't have any enforcement of contrast ratios. The origin text uses views::Label which (given a background colour) ensures the most contrasting colour gets used. For a theme colour of #4285f4 it disagrees with the colour chosen by GetButtonColor(). See screenshot.
,
Mar 20 2018
Created test PWA for the GPM theme colour #f16c00: https://bead-glove.glitch.me/
,
Mar 26 2018
,
Apr 3 2018
,
Apr 5 2018
The previous test site disappeared somehow. Created another one with theme colour #F16C00: https://fast-lute.glitch.me/ Compare chromeOS and Clank screenshots. Android will use white if the contrast ratio is >= 3 otherwise black while chromeOS uses kGoogleGrey100 (#F1F3F4) if the theme colour's luma is less than 128 otherwise kGoogleGrey800 (#3C4043).
,
Apr 5 2018
FYI - +mdjones@ (Clank Eng, who worked on the theme color and contrast)
,
Apr 5 2018
I think we should make chromeOS consistent with Clank given that web developers have been writing PWAs for mobile so far, they would have been designing their colour palettes against Clank's algorithm.
,
Apr 6 2018
Does this also fix the case where the buttons' color doesn't match the origin's? For example, if the theme color is #4285f4, we get two different colors.
,
Apr 6 2018
alancutter@ - re:c10, in case useful, one example I saw was app.starbucks.com (it shows the same issue as you described in c2). Thanks much for working on this!
,
Apr 6 2018
WIP CL at: https://chromium-review.googlesource.com/c/chromium/src/+/999213 #10: Yes, this fixes the mismatched colours for the origin text, see screenshot of #4285f4. #11: Yep, added screenshot of app.starbucks.com.
,
Apr 6 2018
alancutter@, what do you think of matching the Clank's foreground color on a light theme: Light toolbar background colors: - Foreground: 64% black
,
Apr 6 2018
To add to c14, it helps make the dark foreground blend into the background color.
,
Apr 6 2018
#14: Looks like I missed part of the dark foreground colour algorithm on Clank. It should be the theme color blended with 64% black, thanks for catching that!
,
Apr 6 2018
Updated algorithm to use 64% black theme color for light theme colours. See updated Santatracker screenshot and comparison of #949595 vs #959595 where the contrast ratio of 3 is reached.
,
Apr 6 2018
😍 Looks great. Thanks alancutter@!
,
Apr 6 2018
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64a32232a5fbd7fce899abaa9c984ff1b1dd7583 commit 64a32232a5fbd7fce899abaa9c984ff1b1dd7583 Author: Alan Cutter <alancutter@chromium.org> Date: Thu Apr 12 06:23:27 2018 Use Android style title bar colors for theme colors on chromeOS This CL updates the way hosted app windows pick title bar colors while a theme color is set on chromeOS. This aligns the behaviour with Android, see: https://codereview.chromium.org/1956803002. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=329991&signed_aid=3q0FM93vRJrPJV7kMinOTw==&inline=1 https://bugs.chromium.org/p/chromium/issues/attachment?aid=333012&signed_aid=VNXQQhjxyzYYyg36S3nnOQ==&inline=1 https://bugs.chromium.org/p/chromium/issues/attachment?aid=330203&signed_aid=DoC1IRqCuP659D2dMoL1_Q==&inline=1 After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=333244&signed_aid=J5yNTnY4-7RqGeYu32N8OA==&inline=1 https://bugs.chromium.org/p/chromium/issues/attachment?aid=333245&signed_aid=VOf6afQMQJbCApYu21wmyw==&inline=1 https://bugs.chromium.org/p/chromium/issues/attachment?aid=333257&signed_aid=OJzGQyVnXzCfXF6bZ16TCg==&inline=1 https://bugs.chromium.org/p/chromium/issues/attachment?aid=333258&signed_aid=jygsmXqxXfoPsPZXb_gqfg==&inline=1 Bug: 814121 Change-Id: I10a2e837d0c8a938072171715119a04c28e4f36a Reviewed-on: https://chromium-review.googlesource.com/999213 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#550045} [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/BUILD.gn [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/caption_buttons/frame_caption_button.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/caption_buttons/frame_caption_button.h [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/caption_buttons/frame_caption_button_container_view.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/caption_buttons/frame_caption_button_container_view.h [add] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/caption_buttons/frame_caption_button_unittest.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/default_frame_header.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/default_frame_header.h [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/default_frame_header_unittest.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/frame_header_origin_text.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/ash/frame/frame_header_origin_text.h [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/chrome/browser/ui/views/frame/browser_frame_header_ash.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc [modify] https://crrev.com/64a32232a5fbd7fce899abaa9c984ff1b1dd7583/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
,
Apr 12 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by mgiuca@chromium.org
, Mar 19 2018Cc: calamity@chromium.org mgiuca@chromium.org
Owner: alancutter@chromium.org
2.9 KB
2.9 KB View Download