Force theme background tab text color to be readable |
|||||||||||
Issue descriptionThe theme background tab tint changed in M69 to allow tabs to match the frame. Some themes were relying on the old lightening to make their dark text look readable and as a result have unreadable colors now, especially in incognito. For each of the four background tab text color values, unless the theme author explicitly sets them (and maybe even then?) we should use Bret/Orin's new functions to force them to have some minimum level of contrast over the background tab.
,
Aug 7
@2: Honestly I'm not sure how much we can do in that case; that frame is so noisy that the theme author is likely going to have to modify the theme to explicitly tint the background tabs. Can you give a link to the theme? @3: This suggests to me that the theme author has set a bad frame color or needs to add an IDR_WINDOW_CAPTION.
,
Aug 7
,
Aug 8
Theme used in screenshots: https://chrome.google.com/webstore/detail/colors/lhbgjlhhonbdjfdoiklbbkejcipkbnac { "description": "A colorful 'paint splashes' chrome theme.", "key": "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEApSiW8uG7rG9qAe0BOSDHuSjS4oFVKgmIPi++Os8aCPEiv9arPr5CdsXtwzfEOpsMpbLVg+KUtEcBckok4364FvfImVshYpLbI8Knmyha2lGn1+cV1397GFVLETwU+O8AzwHl7ax8tssTBTlSDJEIZjGBYCy3KTpbUOfVjTAL6CRc2BJG2wYnSBJr4D+wAiFuRouLBofTe9fp1zgSqcWDGG8KA81oHxViELSm+Z3Pn+NZTOY4Zp8ZCeuwDatUZp7heSSclK5vQlEHOtfCU/Pqe9tlBnbW3xWwnZBFLDTIFTJWrUi5p0UB/Eop+maIlhZ2BRLPZ+y2YWkAYnI42f2ZswIDAQAB", "manifest_version": 2, "name": "Colors", "theme": { "colors": { "bookmark_text": [ 55, 94, 147 ], "button_background": [ 0, 0, 0, 0 ], "frame": [ 255, 255, 255 ], "ntp_background": [ 255, 255, 255 ], "ntp_text": [ 45, 45, 45 ], "tab_background_text": [ 0, 0, 0 ], "tab_text": [ 65, 65, 65 ], "toolbar": [ 185, 226, 232 ] }, "images": { "theme_frame": "images/theme_frame.png", "theme_ntp_background": "images/theme_ntp_background.png", "theme_toolbar": "images/theme_toolbar.png" }, "properties": { "ntp_background_alignment": "left top", "ntp_background_repeat": "no-repeat", "ntp_logo_alternate": 0 }, "tints": { "buttons": [ 0.56, 0.98, 0.44 ] } }, "update_url": "https://clients2.google.com/service/update2/crx", "version": "2.1" }
,
Aug 9
For the record I've notified the Colors theme author about the up coming UI changes and sent out a CL to fix our theme documentation: https://chromium-review.googlesource.com/c/chromium/src/+/1169082
,
Aug 9
Thanks Alan. It looks to me like the issue in comment 3 is indeed that the author has set the frame color to #ffffff; if they set it to some color that's a closer representative for their frame image, or (in M69+) simply leave it blank so we autocompute it, the caption button glyphs will be colored better.
,
Aug 9
,
Aug 9
,
Aug 18
#4 I disagree with blaming the author for having a "noisy tab", this really sound like a bug (see attached image called bug.png). #7 My theme is also impacted, could you share here what should we change? In order to fix this ourselves?
,
Aug 18
I found two alternatives, none of which are good. I could change my background tab text color to white (in my theme) and it would now look like bug2 .png (which is unacceptable IMO) The other one is by forcing a background tab background color, which is even worse. (see bug3 .png) However, I realized I can play with "frame_inactive" or "background_tab" tints. I'm trying to find the best combination but it's honestly very hard.
,
Aug 18
And finally, at least I was able to fix my theme with a "better" tint, but I did have to change my background tab text color to white. I just had to add this tint - "frame_inactive": [-1, -1, 0.63] I'm happy but I think it's kind of.. disruptive. Like.. you seriously need to email that this can happen.
,
Aug 18
carlosiog90: Is this a theme on the web store? If so, if you can provide a link, we can test to see what the current code will transform to. Chrome 69+ themes now allow setting different background tab text colors for active and inactive frames, so you don't need to pick just one text color that works in all scenarios.
,
Aug 18
pkasting - Thanks, the link to the theme is https://chrome.google.com/webstore/detail/yin/ildepmemekfmmbkhhlhfdammihfhghea?hl=en
,
Aug 20
carlosiog90: I've got a change that should be landing soon which will do some automatic processing to ensure that the contrast between tab text color and the tab background is above a minimum threshold. This should help ensure that most themes don't end up in a broken/unreadable state as a result of the changes in M69. I'm attaching a screenshot of your theme with the new processing active in each of the 4 states to give you an idea of what this might look like. If you have any thoughts or feedback on how this works/looks, etc - I'd love to hear them!
,
Aug 21
Ah, that looks really nice, my only feedback is that the contrast could probably be a little bit higher, but other than that I think this is indeed what I would expect.
,
Aug 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1db88d208dd765df0187df916d6ce67e20fa110e commit 1db88d208dd765df0187df916d6ce67e20fa110e Author: Ryan Meier <rameier@chromium.org> Date: Tue Aug 21 17:19:26 2018 Make theme background tab text readable While importing a theme, browser_theme_pack now runs through a pass to ensure that tab title text for background tabs maintains a minimum contrast ratio with the tab background. Bug: 871026 Change-Id: Ic1d76c9d50ac0255de671916138933dd96af49ef Reviewed-on: https://chromium-review.googlesource.com/1173491 Commit-Queue: Ryan Meier <rameier@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#584806} [modify] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/browser/themes/theme_properties.h [add] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/test/data/extensions/theme_tabcontrast/manifest.json [add] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/test/data/extensions/theme_testinherittextcolor/manifest.json [add] https://crrev.com/1db88d208dd765df0187df916d6ce67e20fa110e/chrome/test/data/extensions/theme_testinherittextcolor_withincog/manifest.json
,
Aug 21
As themes are being imported, we now do some processing to ensure that tab text contrasts appropriately with the tab background, taking into account tab colors and custom tab background images. This works for a majority of themes and ensures that they work and are readable automatically with the new M69 changes. Unfortunately, the 'Colors' theme provided as an example in the initial bug falls in the minority of themes that this fix doesn't work terribly well for, as the background image across the tab bar varies greatly in color, so calculating an average color (which is how this change fixes most cases) doesn't work. The correct fix for that theme is likely to set a background image/color for the tab backgrounds, which isn't really something we can easily (or probably even should) do automatically.
,
Aug 21
Requesting M69 merge. Rationale is that we changed the background tab tinting in M69, and a number of custom themes that worked fine before will be unreadable/broken now; this mitigates that to the greatest degree possible.
,
Aug 21
Pls update bug with canary result tomorrow for cl listed at #19.
,
Aug 21
This bug requires manual review: We are only 13 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 22
,
Aug 22
The NextAction date has arrived: 2018-08-22
,
Aug 22
@22: Verified working in Canary (install the theme listed in comment 16 to test).
,
Aug 22
Approving merge to M69 branch 3497 based on comments #21 and #26. Pls merge ASAP so we can pick it up for tomorrow's Beta release. Thank you.
,
Aug 22
In order to merge this cleanly, I need to merge the patch on bug 854675, as well as the followup fix to that patch on bug 872045 . It's possible to merge without doing this, but it would mean the theme pack versions get out of sync between releases, which can cause compat problems when moving profiles between versions. I'd rather avoid that if I can. As an added bonus, merging bug 854675 should reduce browser process memory usage, potentially significantly, for the fraction of users that have themes with large images. I'm going to request a merge on those bugs.
,
Aug 22
Thank you pkasting@. Merges listed at #28 are approved.
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b1c80acc0655a280d4e0df40a4f0a668362f5a2 commit 6b1c80acc0655a280d4e0df40a4f0a668362f5a2 Author: Peter Kasting <pkasting@chromium.org> Date: Wed Aug 22 19:23:29 2018 Make theme background tab text readable While importing a theme, browser_theme_pack now runs through a pass to ensure that tab title text for background tabs maintains a minimum contrast ratio with the tab background. (cherry picked from commit 1db88d208dd765df0187df916d6ce67e20fa110e) Bug: 871026 Change-Id: Ic1d76c9d50ac0255de671916138933dd96af49ef Reviewed-on: https://chromium-review.googlesource.com/1173491 Commit-Queue: Ryan Meier <rameier@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#584806} Reviewed-on: https://chromium-review.googlesource.com/1185634 Cr-Commit-Position: refs/branch-heads/3497@{#777} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/browser/themes/theme_properties.h [add] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/test/data/extensions/theme_tabcontrast/manifest.json [add] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/test/data/extensions/theme_testinherittextcolor/manifest.json [add] https://crrev.com/6b1c80acc0655a280d4e0df40a4f0a668362f5a2/chrome/test/data/extensions/theme_testinherittextcolor_withincog/manifest.json |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by pkasting@chromium.org
, Aug 4Labels: M-69