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

Issue 848922 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
(OOO slow)
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

[Modern] Scrim doesn't cover the status bar for Contextual Search

Project Member Reported by mdjones@chromium.org, Jun 1 2018

Issue description

When contextual search is expanded, the scrim covers the web page but not the status bar. In modern this looks particularly bad since the status bar is usually white.

The android scrim now does this as a result of:
https://chromium-review.googlesource.com/c/chromium/src/+/996383

Unfortunately, the Constextual Search scrim is drawn by the compositor and is unaffected by the above change.
 
Cc: donnd@chromium.org
Labels: -Pri-2 Hotlist-Chrome-Modern Pri-1
Summary: [Modern] Scrim doesn't cover status bar in for Contextual Search (was: Scrim doesn't cover status bar in for Contextual Search)

Comment 2 by donnd@chromium.org, Jun 2 2018

Summary: [Modern] Scrim doesn't cover the status bar for Contextual Search (was: [Modern] Scrim doesn't cover status bar in for Contextual Search)
Now that issue 818745 is fixed we're happy with the scrim for everything except Contextual Search, right?

It sounds like we need to update that status bar color to match the CS scrim while we're drawing that in the compositor.

Comment 3 by hannahs@google.com, Jun 2 2018

Yes, I believe the scrim should be to spec now, the only thing we need to update is the status bar color to either the CS one when there's a scrim. Thanks!

Comment 4 by donnd@chromium.org, Jun 12 2018

Owner: donnd@chromium.org
Status: Assigned (was: Available)
I'll take a look at this since it's Contextual-Search specific now.

Matt & Theresa, can you help me with the overall approach here?  I think I need to update the status bar color when the Overlay Panel is expanding and we're drawing the scrim for the Overlay.  One way would be to update the Status Bar in ContextualSearchPanel#getUpdatedSceneOverlayTree ( https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java?type=cs&q=getUpdatedSceneOverlayTree&sq=package:chromium&g=0&l=160) but it seems bad to add a side-effect to that.  I think a better approach would be to add an updateStatusBar method that we call at the end of OverlayPanelBase#updatePanelForHeight (https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java?type=cs&q=updatePanelForHeight&sq=package:chromium&g=0&l=797), and CS can override that to know what color scheme it's using?
The scrim introduced a StatusBarScrimDelegate (https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/widget/FadingBackgroundView.java?rcl=498325c5e770abd27f6798d6ddb8d34c94ca596f&l=79) that it uses to delegate the responsibility of updating the status bar color for a scrim percentage (without affecting the theme color). It is implemented in-line to prevent classes other than the scrim from changing the color (https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java?rcl=498325c5e770abd27f6798d6ddb8d34c94ca596f&l=1368).

One option is to use the android scrim (FadingBackgroundView) but add a param that causes it to only affect the status bar. This seems like we're trying to shoehorn the scrim to be something it isn't, but keeps the implementation details hidden like I'd want.

Another option is to expose the status bar delegate from ChromeActivity. Unfortunately this lets anyone control the status bar and steps away from using the FadingBackgroundView class, but is probably easy to do.

Both options assume that updating an android view along with the compositor scrim will look ok, which it may not.

Comment 6 by donnd@chromium.org, Jun 25 2018

I have a CL that creates an approximate match between the status bar and the native overlay scrim, and wanted to capture Matt's comment on how to make this a perfect match in a subsequent CL:

"The android scrim uses a specific color (with alpha already applied) and changes opacity (0 to 1) on top of that. The color is here: https://codesearch.chromium.org/chromium/src/chrome/android/java/res/values/colors.xml?rcl=547b8f4807278a9c6181ff73e650e77c81ea846b&l=35
We use the omnibox_focused_fading_background_color in the FadingBackgroundView though (it points to the transparent black).
It looks like the overlay scrim uses a decidedly different system. The native code appears to use a brightness filter (CreateBrightnessFilter). It might be worth creating a full-screen sized view and apply the target color to it. I'm not sure you'll want to do that in this patch though."

Since this is P1 for 69 I'll plan to land the approximate CL first.
Thanks for looking into this Donn!

Will you please grab a quick video of the in-flight CL for our records?

Comment 8 by donnd@chromium.org, Jun 25 2018

Here's a video of the CL in flight: https://drive.google.com/open?id=1clyS95_I0drOr0TNbh9EBuABS-KKUlle

I think this looks pretty good for a v1! Hannah, what do you think?
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 25 2018

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

commit 04d2cc2b35f4c6fa812eb6b8e6728f78d67ed863
Author: Donn Denman <donnd@chromium.org>
Date: Mon Jun 25 22:21:53 2018

[TTS] Update the Status Bar color when CS Scrim on

Updates the Status Bar to match the scrim color when Contextual
Search shows the Overlay Panel.

Also updates the FadingBackgroundView to allow no anchor view.

BUG= 848922 

Change-Id: I085c57f1ae9b991518c5fc2925243505d751e70a
Reviewed-on: https://chromium-review.googlesource.com/1098455
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570200}
[modify] https://crrev.com/04d2cc2b35f4c6fa812eb6b8e6728f78d67ed863/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
[modify] https://crrev.com/04d2cc2b35f4c6fa812eb6b8e6728f78d67ed863/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
[modify] https://crrev.com/04d2cc2b35f4c6fa812eb6b8e6728f78d67ed863/chrome/android/java/src/org/chromium/chrome/browser/widget/ScrimView.java

lgtm! Thanks for the quick turnaround!
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 26 2018

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

commit dd2d9ca57b47597afe64bae120e5a6f2dcc0155e
Author: Donn Denman <donnd@chromium.org>
Date: Tue Jun 26 14:14:08 2018

Revert "[TTS] Update the Status Bar color when CS Scrim on"

This reverts commit 04d2cc2b35f4c6fa812eb6b8e6728f78d67ed863.

Reason for revert: Status bar sometimes stays gray after CS.  See issue 856571.

Original change's description:
> [TTS] Update the Status Bar color when CS Scrim on
> 
> Updates the Status Bar to match the scrim color when Contextual
> Search shows the Overlay Panel.
> 
> Also updates the FadingBackgroundView to allow no anchor view.
> 
> BUG= 848922 
> 
> Change-Id: I085c57f1ae9b991518c5fc2925243505d751e70a
> Reviewed-on: https://chromium-review.googlesource.com/1098455
> Reviewed-by: Theresa <twellington@chromium.org>
> Reviewed-by: Matthew Jones <mdjones@chromium.org>
> Commit-Queue: Donn Denman <donnd@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#570200}

TBR=donnd@chromium.org,twellington@chromium.org,mdjones@chromium.org

Change-Id: Ifa88bff81b2ffc4a8216d8cfef33ced79f3c555f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  848922 
Reviewed-on: https://chromium-review.googlesource.com/1114978
Reviewed-by: Donn Denman <donnd@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570398}
[modify] https://crrev.com/dd2d9ca57b47597afe64bae120e5a6f2dcc0155e/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
[modify] https://crrev.com/dd2d9ca57b47597afe64bae120e5a6f2dcc0155e/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
[modify] https://crrev.com/dd2d9ca57b47597afe64bae120e5a6f2dcc0155e/chrome/android/java/src/org/chromium/chrome/browser/widget/ScrimView.java

Project Member

Comment 13 by bugdroid1@chromium.org, Jun 27 2018

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

commit c1d7ebd614d42d911836802022a050f80af8cad9
Author: Donn Denman <donnd@chromium.org>
Date: Wed Jun 27 23:17:06 2018

[TTS] Reland Status Bar color update for CS Scrim

Relands this previous CL that had a flaw, with the flaw fixed.

Updates the Status Bar to match the scrim color when Contextual
Search shows the Overlay Panel.

Also updates the FadingBackgroundView to allow no anchor view.

BUG= 848922 ,856571

Change-Id: I397ba09dce5aa7be87ecf98ba9faf14293306467
Reviewed-on: https://chromium-review.googlesource.com/1115207
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570929}
[modify] https://crrev.com/c1d7ebd614d42d911836802022a050f80af8cad9/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java
[modify] https://crrev.com/c1d7ebd614d42d911836802022a050f80af8cad9/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
[modify] https://crrev.com/c1d7ebd614d42d911836802022a050f80af8cad9/chrome/android/java/src/org/chromium/chrome/browser/widget/ScrimView.java

Comment 14 by donnd@chromium.org, Jun 27 2018

Status: Fixed (was: Assigned)

Sign in to add a comment