Opening up to other platforms as this is an extension theme

Fixed link to theme:

https://chrome.google.com/webstore/detail/chuck-anderson/gegkoiakifeoejnjkbnnojkkdoegeofp

The issue seems to be that hovering a background tab pushes it much closer to the active tab than I'd expect, and that in turn means our assumption that hovering doesn't affect the tab's color much (so we don't need to adjust its text color) is violated.

Ryan, have time to figure out why this is?

Nothing jumps out at me, but I'm not terribly familiar with how tab hovering is handled.  Orin, I'm led to understand that you did the contrast-based hover implementation so you might have a better idea what's going on - would you be able to take a look at this?

Sure, I will look into this today.

The problem appears to be that the colors reported by tab-strip controller's GetTabBackgroundColor are not as different as expected.  We get:

TAB_INACTIVE -> rgba(120,144,161,1)
TAB_ACTIVE -> rgba(128,143,156,1)

So trying to produce contrast between these nearly-identical color values results in a high (1.0) opacity.  So #c3 is right on.  If I force lower opacity here (which would naturally occur if the color values were sufficiently different as they should be) then we end up with a much more natural/expected hover effect.  So next I will look at why TAB_ACTIVE returns such a dark shade of gray while the rendering is much brighter.  I don't think we need to worry about changing text color at this point.

My suspicion: the theme explicitly specifies both a toolbar image (that is near-white) and a color (that is (128,143,156)).  In theory, if you specify both, your color is supposed to be a representative color for the image, which means this would be theme author error.

It's not clear how we can be robust in the face of this kind of thing.

Yep, theme manifest.json pasted below so you can see how "toolbar" color is set -- this color doesn't quite represent the "theme_toolbar" gradient image, so it is apparently theme author error.  But maybe we can make life easier for authors or reduce errors... personally I found it surprising that the active tab background color is coming from "toolbar", but according to the code this is correct.  And if a color is supposed to be representative of an image, maybe we can auto-derive the color from the image at theme-installation time... is it possible to show a warning or error out if there's some conflict with an author-specified color?

I agree it's hard to be robust in a cases of conflict like this, but I'll pass it to you for judgment on whether this is a WontFix or cause for action...

{
   "default_locale": "en",
   "key": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC87fz98fc92REB0SojrzPW0ysa6owVqEdb47bl+8yaOo2KByYCmfrOEwINoXbId6aCLYxXaNGrGUvsmgwHhEeHx/VghdjkqHbmOeTpd0WHQVlAKb6GXJH5M5/RY1Om/NZ9varZZ2w1rflDR2My93Rv/xYQBxq8imnAiD3jalSMVQIDAQAB",
   "name": "__MSG_themeName__",
   "theme": {
      "colors": {
         "bookmark_text": [ 23, 34, 55 ],
         "frame": [ 43, 104, 102 ],
         "ntp_background": [ 255, 255, 255 ],
         "ntp_header": [ 165, 51, 102, 1 ],
         "ntp_link": [ 28, 119, 125 ],
         "ntp_section": [ 232, 241, 246, 1 ],
         "ntp_section_link": [ 28, 119, 125 ],
         "ntp_section_text": [ 91, 55, 88 ],
         "ntp_text": [ 28, 119, 125 ],
         "tab_background_text": [ 255, 255, 255 ],
         "tab_text": [ 23, 34, 55 ],
         "toolbar": [ 128, 143, 156 ]
      },
      "images": {
         "theme_button_background": "i/agxjaHJvbWV0aGVtZXNyDAsSBEZpbGUYmPkCDA",
         "theme_frame": "i/agxjaHJvbWV0aGVtZXNyDAsSBEZpbGUYoYEDDA",
         "theme_ntp_attribution": "i/agxjaHJvbWV0aGVtZXNyDAsSBEZpbGUYk8oEDA",
         "theme_tab_background": "i/agxjaHJvbWV0aGVtZXNyDAsSBEZpbGUYv_ECDA",
         "theme_toolbar": "i/agxjaHJvbWV0aGVtZXNyDAsSBEZpbGUY-cMGDA"
      },
      "properties": {
         "ntp_background_alignment": "bottom",
         "ntp_background_repeat": "no-repeat"
      },
      "tints": {
         "buttons": [ 0.671, 0.329, 0.5 ]
      }
   },
   "update_url": "http://clients2.google.com/service/update2/crx",
   "version": "3"
}

Some things we can do here:

* If the hover opacity is >50%, use the active tab text color for hovered tabs (and as the base for close button glyphs, etc.) instead of the inactive tab text color.  We could try to do something else like push the text color one way or another to get a different contrast ratio but ugh.
* Find what team nominally owns this extension and get them to remove the "toolbar" color value, or maybe change it to [ 198, 206, 214 ], which is at least more representative.

I don't think we can show a warning about this kind of thing.  If we had a theme validation tool, it could look for issues like this, but we don't have one.

Sure, will do -- using the text color designed to be shown atop the background color that is nearest to hover color actually being shown ... seems sensible and should be an improvement.

I do like the idea of a theme validation tool, and improving the process for creating themes could nip some of these kinds of issues in the bud.  Would be great to have an online tool to create themes live!  Color is such an important part of user experience, I think it'd be worth the effort to make theme creation and tuning high class.

Okay, I sent an email to who I think is the theme author to help them update their theme.  Then I wrote this CL to improve color handling:

crrev.com/c/1227215 : Select tab foreground colors using expected opacity

One possibly-unexpected consequence of this logic is that selected tabs will now change their foreground color discretely instead of pushing for contrast as they did before.  This seems consistent with the intention of ensuring contrast against the actual expected background color (derived from expected opacity, which is 0.75 in selected tab case).

+kylixrd@ here for context because my changes overlap with some of his code that addresses similar contrast concerns.  This bug applies not only to hovered tab but also selected tabs: they were rendered white on white with Chuck Anderson theme as well, but switching title to active-state text color gives good contrast.

Would pkasting@ or kylixrd@ want to review the CL?

I don't think this should be fixed in the theme. If one theme looked fine this way before and no longer does, there can be any number of themes for which the same is true. I think a color scheme that worked before should continue to work.

Active tab renders black on white for me. Not white on white.

And if we land the fix in comment 11, the hovered tabs will also be black on white.

It's not possible in general to make every theme work, because many behaviors in Chrome aren't directly specified by themes, and when a theme implicitly relies on an unspecified behavior and that behavior changes, things can break.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1adfd6531bf33724c15b31e77a406e8e850c55d8

commit 1adfd6531bf33724c15b31e77a406e8e850c55d8
Author: Orin Jaworski <orinj@chromium.org>
Date: Mon Sep 17 23:30:06 2018

Select tab foreground colors using expected opacity

Some themes were showing tab text with insufficient contrast
because the tab background was rendered more like the active
state than inactive state (e.g. when selected or hovered).
To ensure sufficient contrast, the expected render opacity
and tab background color are used to decide on title text
color, as well as to generate button and icon colors.

Bug:  883257 
Change-Id: I67398132d13fd9e401e4165347bffe35ac6e506d
Reviewed-on: https://chromium-review.googlesource.com/1227215
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591873}
[modify] https://crrev.com/1adfd6531bf33724c15b31e77a406e8e850c55d8/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/1adfd6531bf33724c15b31e77a406e8e850c55d8/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/1adfd6531bf33724c15b31e77a406e8e850c55d8/chrome/browser/ui/views/tabs/tab_unittest.cc

This is working in today's Canary.

Will this also fix the hover effect on the tab close button for the active tab? That's also almost not visible.

This fix makes the close button visible on inactive hovered tab (the 'X' was white on white; now it's black on white).  It does not change the hover effect.  That might be a separate bug, if we really want to go down the rabbit hole of handling every odd interaction with user-made themes.  I have a better solution in mind, though, mentioned briefly in this comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=870549#c14

I really need to write up this idea in detail sometime.  The fundamental problem here is not that we give users/theme authors too much control over colors, it's that we don't expose the full set.  We have a hodgepodge of colors coming from the OS, from the theme, from the code - and we have a bunch of logic to blend them together and push them around for contrast... but what we really need to do is introduce an intelligent color management system that Chrome uses *exclusively* for UI colors.  Turn everything into data, define the relations between colors (for things like readable contrast), and let the system manage the flow from there.  Pull from legacy themes, pull from OS colors, pull from wherever...but then check all relations to warn or fail on commit.

@16, 17: I filed bug 887642 about the hover effects.

The hover style has indeed been fixed but now the problem happens on the selected tab when the window loses focus.

Can you file that separately?  (On the run or I'd just file)

https://crbug.com/914860