[Modern] Scrim doesn't cover the status bar for Contextual Search |
||||
Issue descriptionWhen 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.
,
Jun 2 2018
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.
,
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!
,
Jun 12 2018
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?
,
Jun 12 2018
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.
,
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.
,
Jun 25 2018
Thanks for looking into this Donn! Will you please grab a quick video of the in-flight CL for our records?
,
Jun 25 2018
Here's a video of the CL in flight: https://drive.google.com/open?id=1clyS95_I0drOr0TNbh9EBuABS-KKUlle
,
Jun 25 2018
I think this looks pretty good for a v1! Hannah, what do you think?
,
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
,
Jun 26 2018
lgtm! Thanks for the quick turnaround!
,
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
,
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
,
Jun 27 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by twelling...@chromium.org
, Jun 1 2018Labels: -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)