New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 856893 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 865819
issue 822061



Sign in to add a comment

Hovered tab is too close to active state

Project Member Reported by bettes@chromium.org, Jun 27 2018

Issue description

With  issue 848415 , we tried a linear function that scales between 0.44 at standard width up to 0.85 at minimum width. 

With 10 tabs, hovering now looks just like a flash of color. See movie.

Expected: 
Let's keep the 0.44 opacity, for any size tab, and remove the linear function

 
hover.mov
3.0 MB View Download

Comment 1 by bettes@chromium.org, Jun 27 2018

Labels: Hotlist-Helper Proj-MdRefresh
Labels: -Restrict-View-Google -Pri-2 -Hotlist-Teamfood-Feedback Pri-3
That's not what we landed; we landed something that ranges between 0.5 and 0.65.  We also landed a change on another bug that changes the hover animation duration from 400 to 200 ms.  And it's important to test these changes across a range of different themes, colors, and tab widths, since the effects are very different in different cases.  Finally, your comparison is against pre-refresh, not against refresh without the change(s) in question.

Orin and I both spent some time looking at this, so while I'm happy to make further changes, I'd like to do it in a carefully-considered context.  Maybe you can spend some time going over all the above with us.  If not, let's punt changing anything here to post-69 when we have more time.
Labels: Hotlist-Polish

Comment 4 by bettes@chromium.org, Jun 28 2018

Owner: bettes@chromium.org
Status: Assigned (was: Available)
Blockedon: 854738
Blockedon: -854738
Blocking: 822061
Cc: bettes@chromium.org
Owner: ----
Status: Available (was: Assigned)
I compared against pre-refresh because that is what we should aim for and to show that 0.5 is worse off. 

I've explored options across custom title bars, default, and our most popular theme. After doing so, I've come to believe that we can go even lower than 0.44 and still maintain proper tab distinction. (given that x buttons are back) 

However, for the sake of time, my request still remains to go back to 0.44. 
0.30 is better.
0.20 is the preferred visual treatment but probably upsetting to this audience, so that can wait till we have more time to experiment. 

Without seeing an alternative, I can't comment on the effectiveness of the linear coloring at this time. 

https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZW9TjXpqhOa9/files/MCFBUdYrwbJ9FPJZ7d0aHYQg
Owner: orinj@chromium.org
Status: Assigned (was: Available)
Okay, I think this is a simple change but has high impact, so I'm ready to come down on some numbers for anyone who wants to look at the results.  I don't think going from 0.5 to 0.44 will make a huge difference, but dropping further to 0.3 will be noticeable.  So how about widening the range from today's [0.5, 0.65] to proposed [0.3, 0.65]?  This would mellow the effect on wide tabs while remaining stark on narrow tabs - and maybe strike a balance for both sides?
Components: UI>Browser>TabStrip
Honestly, I really like the 0.2 w/much softer radial gradient "preferred" proposal in the link in comment 7.

I worry about two things the mocks don't cover:
1. Super-narrow tabs (i.e. below 24-40 px), which is where the opacity boost starts to become more helpful for targeting
2. Themes which use images, which is some of what drove the 0.5-0.65 interpolation

That said, I'd like to at least achieve the design vision for the main case, and I'd LOVE a Bettes-approved change to lower the hover radial gradient opacity, since that's the really off-putting thing for me today.

So what about this: we try to implement the "proposed" changes from comment 7, but also file a bug where Alan will examine the two cases above, probably for M70.  Would that be an OK path forward?
Labels: Group-Tab
Labels: -Pri-3 Pri-2
Bumped pri
Okay, I will start a CL to change the range from [0.5, 0.65] to [0.3, 0.65] and change the interpolation parameter from t to t^2.  This will keep more tabs on the lower end of opacity while still ensuring that extremely narrow tabs get the full opacity boost.  With this change we can test and begin to see if this is sufficiently different from active state or if we also want to affect hover radial gradient opacity.
Can we start by just implementing Alan's proposed changes directly, and then experiment atop that?  That way we'll have a design-approved baseline for the default theme.  I'm also making a bunch of other changes that will affect how custom themes and small tabs work, so those might affect what further tweaking we want to do.
Status: Started (was: Assigned)
Blocking: 865819
Just had a very productive conversation with Alan about the spec and a plan to proceed.  I think patch-set 6 on the CR is ready to go so he can experiment.  And then we can change radius later - or maybe back-fit the spec to the code's calculation?  Would be fun to see how that happens! :D
Alan and I just played with a bunch of stuff locally.

We decided that a key issue here is the variance of active and inactive tab colors.  No one opacity value or curve works well across all themes and colors.  What we need is something based on contrast ratios.

We also found that in practice, the 0.2 in his spec felt a bit too subtle for full-width tabs, and definitely too subtle for narrow tabs.  So Alan agreed that some kind of a curve based on tab width was justified.

Here's where we landed:

* With the default theme colors -- 0xFFFFFF active tabs against 0xDEE1E6 background tabs -- Alan likes what we have today except he would lower the full-width base opacity from 0.5 to 0.4.  So we'd have an 0.4 - 0.65 lerp for base opacity, and a radial gradient that uses exactly the current params.

* Given the above, compute the contrast ratios of the following:
** Hovered tab base against unhovered tab base
** Radial gradient center against hovered tab base
Then change the code so that instead of using particular opacities, it uses opacities sufficient to hit those contrast ratios.  For a theme where the active tab is much different than the inactive tab, we'll end up using much lower opacities for both the base hover and the radial gradient.  For a theme where the two are nearly identical, we'll potentially go to 100% opacity.

* To implement the above, we'll probably need to decompose the code Bret added recently that tries to guarantee a minimum contrast ratio, to factor out a function that's something like:

// Alpha blends |trg| atop |src| until the resulting color has a contrast
// ratio of at least |contrast_ratio| with |base|.  Returns the alpha value
// for that blend
SkAlpha GetBlendValueForContrastRatio(
    SkColor src, SkColor trg, SkColor base, float contrast_ratio);

Then you can make calls like:

constexpr float kDefaultThemeHoverBaseAlphaWide = 1.11f;
const SkAlpha hover_base_alpha_wide = GetBlendValueForContrastRatio(
    inactive_tab_bg_color, active_tab_bg_color, inactive_tab_bg_color,
    kDefaultThemeHoverBaseAlphaWide);
constexpr float kDefaultThemeHoverBaseAlphaNarrow = 1.19f;
const SkAlpha hover_base_alpha_narrow = GetBlendValueForContrastRatio(
    inactive_tab_bg_color, active_tab_bg_color, inactive_tab_bg_color,
    kDefaultThemeHoverBaseAlphaNarrow);

...and similarly for the radial gradient opacity; then make sure these get recalculated on a theme change.
Okay, I see Bret's new GetColorWithMinimumContrast function and something like it might be a starting point to develop GetBlendValueForContrastRatio -- or a different approach might also be possible.  I can experiment with it.  Just to be clear: you want the *minimum* alpha such that the blended color has a contrast ratio against |base| of at least |contrast_ratio|, right?
Basically, we have three values to tune:

* The base alpha for full-width hovered tabs
* The base alpha for min-width hovered tabs
* The alpha for the center of the radial gradient

What I want to do is tune all of them to be based on contrast ratios, so that when you use the default theme of #ffffff active tabs on #dee1e6 inactive tabs, our equations end up computing 0.4, 0.65, and whatever the gradient center is today (0.45 I think); and then when you have themes that have very different colors, we wind up with different opacities that produce the same contrast ratios as the default theme case (and thus should produce a similar "level of subtlety" in practical use).

Did I manage to make it clear?
Labels: M-69 Target-69
Labels: -Pri-2 Pri-1
Yes, see if patch-set 7 on crrev.com/c/1147605 expresses the right idea. I see how you found those 1.11 and 1.19 contrast ratios: they give opacities close to 0.4 and 0.65. I did likewise to find the 0.45 radial highlight opacity.

This patch-set is not for submission, it still has logging, fails lint, etc. I'm just aiming to make sure this is the right idea, and if so then I will:
* Remove LOGs
* Optimize GetBlendValueForContrastRatio (right now it's dirt simple so the meaning is clear as glass)
* Save the opacities on Tab instance and calculate them in Tab::OnThemeChanged so that hovering doesn't calculate them on mouse moves (unless you say a binary search within 8-bit range is fast enough)
* Ensure git cl format passes

One more question: How does crbug.com/865819 play into this?  We came up with these target contrast ratios by back-fitting for the existing opacity values, but should we be targeting specified contrast ratios instead - and then let the blend values fall where they may?
I've moved this off the "To be merged list", as I don't think we can merge this reasonably to M69 anymore.
Project Member

Comment 25 by bugdroid1@chromium.org, Aug 6

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

commit 659c9e34e14cfcecf33f2b2e52aa133db5806fa2
Author: Peter Kasting <pkasting@chromium.org>
Date: Mon Aug 06 22:45:42 2018

Force frame and tab colors to be opaque.

We can't compute contrast ratios on transparent colors.  This prevents DCHECKs
with Orin's soon-to-be-landed code, and prevents theme authors from setting
colors that lead to drawing artifacts.

Bug:  856893 
Change-Id: I2994cd44b04ea363b20f087295f6b085325f8ec5
Reviewed-on: https://chromium-review.googlesource.com/1164170
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581022}
[modify] https://crrev.com/659c9e34e14cfcecf33f2b2e52aa133db5806fa2/chrome/browser/themes/browser_theme_pack.cc
[modify] https://crrev.com/659c9e34e14cfcecf33f2b2e52aa133db5806fa2/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
[modify] https://crrev.com/659c9e34e14cfcecf33f2b2e52aa133db5806fa2/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/659c9e34e14cfcecf33f2b2e52aa133db5806fa2/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Aug 7

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

commit 5420a4f2471b3223735947dab88d3c0474de89be
Author: Orin Jaworski <orinj@chromium.org>
Date: Tue Aug 07 20:21:47 2018

Use contrast ratios for tab hover opacities

The hovered tab state was visually too close to active state
so this CL reduces the opacity. The method for calculating
interpolation range and hover radial gradient opacity is
changed to use target contrast ratios. This approach ideally
will adapt better to various themes and color schemes,
ensuring the right level of contrast by adjusting opacity
automatically.

Bug:  856893 
Change-Id: I2799b722c1d84d7c323e4f587fa8f5e58106bf58
Reviewed-on: https://chromium-review.googlesource.com/1147605
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581330}
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/chrome/browser/ui/views/tabs/glow_hover_controller.cc
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/chrome/browser/ui/views/tabs/glow_hover_controller.h
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/ui/gfx/color_utils.cc
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/ui/gfx/color_utils.h
[modify] https://crrev.com/5420a4f2471b3223735947dab88d3c0474de89be/ui/gfx/color_utils_unittest.cc

Cc: kylixrd@chromium.org
Just spoke with tommycli@ and he suggested that I open the conversation about whether we still want to merge these changes for M-69.  Between the two in preceding comments, we not only changed contrast but also fixed an error: translucent colors on themes (e.g. "bookmark_text") were triggering an existent DCHECK.

Allen also has crrev.com/c/1162664 which was related to the new contrast ratios code so I'm adding him to CC.  If we do want to merge, will that be done on the Views side or do you want me to give it a go?
Yes, we want to merge both the CLs here, assuming they work well.  Need to have some validation on Canary and oversight from design before that.

Comment 29 Deleted

Just want to note that if the above CLs get merged then this should also be included: crrev.com/c/1168140
Status: Fixed (was: Started)
Labels: Merge-Request-69
Project Member

Comment 33 by sheriffbot@chromium.org, Aug 13

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
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
Are CLs listed at #25, #26 and crrev.com/c/1168140 need merge to M69? 
If yes, are they all three verified in canary and fully sage to merge to M69?
Also could you pls justify the merge to M69 so it is easy for approval?
Yes, all three.  Verified these are working as expected in Canary.  They should be reasonably safe to merge (safer than a lot of my themes work was).  Justification is that these prevent unreadable contrast in cases where the frame and active tab colors are opposite (e.g. default theme with dark colored titlebars on Win 10).
Labels: -Merge-Review-69 Merge-Approved-69
Thank you pkasting@.

Approving merge to M69 branch 3497 for all three cls listed at #34 based on comment #35.
Project Member

Comment 37 by bugdroid1@chromium.org, Aug 14

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83803734ddcf256df30ca9735f4ddcf5dc76a3ea

commit 83803734ddcf256df30ca9735f4ddcf5dc76a3ea
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Aug 14 00:08:25 2018

(Merge to M69 branch) Force frame and tab colors to be opaque.

We can't compute contrast ratios on transparent colors.  This prevents DCHECKs
with Orin's soon-to-be-landed code, and prevents theme authors from setting
colors that lead to drawing artifacts.

(cherry picked from commit 659c9e34e14cfcecf33f2b2e52aa133db5806fa2)

Bug:  856893 
Change-Id: Ia031faad75304343ad3459c5118df892500e34f5
Reviewed-on: https://chromium-review.googlesource.com/1164170
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581022}
Reviewed-on: https://chromium-review.googlesource.com/1173531
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#599}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/83803734ddcf256df30ca9735f4ddcf5dc76a3ea/chrome/browser/themes/browser_theme_pack.cc
[modify] https://crrev.com/83803734ddcf256df30ca9735f4ddcf5dc76a3ea/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
[modify] https://crrev.com/83803734ddcf256df30ca9735f4ddcf5dc76a3ea/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/83803734ddcf256df30ca9735f4ddcf5dc76a3ea/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Aug 14

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

commit 263ac79dd5e899a7e0a9f752e63606f6cd24300b
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Aug 14 00:42:01 2018

(Merge to M69 branch) Use contrast ratios for tab hover opacities

The hovered tab state was visually too close to active state
so this CL reduces the opacity. The method for calculating
interpolation range and hover radial gradient opacity is
changed to use target contrast ratios. This approach ideally
will adapt better to various themes and color schemes,
ensuring the right level of contrast by adjusting opacity
automatically.

(cherry picked from commit 5420a4f2471b3223735947dab88d3c0474de89be)

Bug:  856893 
Change-Id: Iefe76ab1596adda313a3f5c7014261873e9d82a2
Reviewed-on: https://chromium-review.googlesource.com/1147605
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581330}
Reviewed-on: https://chromium-review.googlesource.com/1173656
Cr-Commit-Position: refs/branch-heads/3497@{#605}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/chrome/browser/ui/views/tabs/glow_hover_controller.cc
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/chrome/browser/ui/views/tabs/glow_hover_controller.h
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/ui/gfx/color_utils.cc
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/ui/gfx/color_utils.h
[modify] https://crrev.com/263ac79dd5e899a7e0a9f752e63606f6cd24300b/ui/gfx/color_utils_unittest.cc

All merged.
Cc: abdulsyed@chromium.org
+abdulsyed@ fyi, M69 merges taken for Proj-MdRefresh .

Sign in to add a comment