Elevation-based shadows do not work for native windows on Windows and Mac |
|||||||
Issue descriptionFor native windows on Mac and Windows, the elevation-based shadows specified by Widget::InitParams::shadow_elevation do not work. I've attached the Desired look, as well as the Actual look on both Mac and Windows. The root cause is that the native shadows drawn by the native window managers of Mac and Windows do not understand shadow_elevation, since it's a Material concept. We have tried the following approach to fix it: 1. Keep using a native window and draw the shadow in Views. This hasn't worked because hit testing on Windows is based on alpha, and we can't click-through the shadow to the content underneath. (CL and long discussion here: https://chromium-review.googlesource.com/c/chromium/src/+/974501) We have also proposed the following approach (on the same CL): 2. Use a second WS_EX_TRANSPARENT window to contain only the shadow, which is transparent to mouse clicks. This works for static windows, but sky@ on the CL suggests that it would not work for animated windows, since they would not stay in sync. pkasting@ also reported similar troubles in the early days of Chrome. There is one more solution which is promising, but we have not tried: 3. Repost the click events that land on the shadow onto the window underneath. Anyone who would take this bug on should probably try #3. In the meantime, Omnibox team is working around this whole issue by using a non-native Aura window for the Omnibox popup. The only downside to this is that the window cannot be larger than the root browser window, and will be clipped.
,
May 1 2018
,
May 1 2018
> 2. Use a second WS_EX_TRANSPARENT window to contain only the shadow, which is transparent to mouse clicks. This works for static windows, but sky@ on the CL suggests that it would not work for animated windows, since they would not stay in sync. pkasting@ also reported similar troubles in the early days of Chrome. The system compositor on Mac is transaction oriented, so it should be possible to atomically change the content of multiple windows. It sounds like the hairiest part is managing the windows involved and making sure they get parented correctly, etc. I know there's been a bunch of work on surface synchronization (for OOPIF, mainly) — would any of that help on Windows? > The root cause is that the native shadows drawn by the native window managers of Mac and Windows do not understand shadow_elevation, since it's a Material concept. So, I got a little bit carried away last night and hacked up a (standalone native, not Views) version of this that draws the shadows CALayer shadow APIs. It's not immediately helpful, just a PoC that this is possible and that the shadows can be animated efficiently. (The code is not pretty.)
,
May 1 2018
I think my comments about the early days of Chrome were taken a little far. I think the WS_EX_TRANSPARENT + WS_EX_LAYERED idea for Windows is workable and worth pursuing.
,
May 1 2018
(Specifically, I think it's a better route than reposting events as in comment 0 item (3), and since it's more parallel to the Mac implementation as well, that makes it a clear front-runner for me.)
,
May 1 2018
sdy: That's a really good-looking prototype you have there. :) Anyways - if any of you folks want to carry this through the finish line with platform-specific implementations, I am happy about it. Take a look at my CL for the Views-native approach to see if there's anything we could reuse from there. https://chromium-review.googlesource.com/c/chromium/src/+/974501
,
May 1 2018
I'd like to reiterate a concern about adding complexity here. If there's some simple, natural way to solve the problem on one or more platforms, great. But I'm skeptical about adding even a moderate amount of additional code and maintenance burden to solve a cosmetic problem that most users are unlikely to ever encounter.
,
May 1 2018
It would be nice to have native compositor support for elevation-based shadows, because then we could use them on all our dialogs of all shadow styles. As-is, because other dialogs can't afford to be clipped by the window, we have to reimplement shadows in the bubble code (which can't be clicked through).
,
May 2 2018
Hey ellyjones: Do we plan to use Aura for MacViews? I ask because our workaround for Windows relies on NativeWidgetAura and Aura WM to draw the shadows. (https://chromium-review.googlesource.com/c/chromium/src/+/1028833) It seems that MacViews currently does not link in Aura (use_aura = false). Tommy
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b67bd278e394b14f9941223111a058c258777e2d commit b67bd278e394b14f9941223111a058c258777e2d Author: Tommy C. Li <tommycli@chromium.org> Date: Wed May 02 00:59:08 2018 Views on Windows: Force NativeWidgetAura for shadow_elevation Widgets On Windows, Widgets with shadow_elevation specified are currently drawn incorrectly. This is because we spawn a native top-level window, which ignores shadow_elevation. This is to be expected, since the Windows window manager has no concept of Material shadow elevation. This CL forces those Widgets with shadow_elevation to use an Aura non-toplevel window. The caveat is that these widgets are now clipped to the root browser window. (It can't be bigger.) But it has the benefits of being: 1. Low maintainence 2. Click-through shadow 3. Proper hit testing Bug: 823535 , 838667 Change-Id: I008ab64d0f3b7c2811d11ac94f00d041c45bf730 Reviewed-on: https://chromium-review.googlesource.com/1028833 Commit-Queue: Tommy Li <tommycli@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#555267} [modify] https://crrev.com/b67bd278e394b14f9941223111a058c258777e2d/chrome/browser/ui/views/chrome_views_delegate_win.cc
,
May 2 2018
#9: not at the moment or in the foreseeable future - Aura doesn't compile on Mac at all. It's on our list of stuff to look at only after MacViews browser is done.
,
May 2 2018
ellyjones: Okay I see. We definitely DO plan on doing Aura after MacViews is done?
,
May 3 2018
#12: No. We haven't yet investigated what it would mean & what the performance/eng tradeoffs would be. The entire project, including establishing whether or not to do it, is deferred until after MacViews.
,
Jun 11 2018
,
Jun 26 2018
,
Jun 26 2018
This half of the bug is essentially fixed by https://chromium-review.googlesource.com/c/chromium/src/+/1100006 The shadows are now drawn correctly within Views. We still need to be able to click through the shadows to the underlying web content area - and click through the top part of the frame to the underlying Omnibox, but that's dealt with in this bug: 836829 .
,
Jun 26 2018
I left this open because being clicking through the shadow may need a different fix from clicking through the hole. I'll leave it up to you and avi@ whether to use this bug or another one to track it.
,
Jul 18
This seems to cause bug 864963 , where the invisible shadow at the *top* of the omnibox blocks interaction with the entire bottom half of the tab strip while the results pane is up. :(
,
Jul 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af7dcaaf7c0cb2f0f3c75626f7acd368bf98287c commit af7dcaaf7c0cb2f0f3c75626f7acd368bf98287c Author: Avi Drissman <avi@chromium.org> Date: Mon Jul 23 22:18:47 2018 Forward events to the tab strip. The shadow around the omnibox results popup eats mouse events. Manually forward events occurring in the shadow area to the underlying widget to fix the tab strip. BUG= 864963 , 838667 Change-Id: I5c6499f5d024863dad611f8670fb12e1a01ff073 Reviewed-on: https://chromium-review.googlesource.com/1144225 Commit-Queue: Avi Drissman <avi@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#577293} [modify] https://crrev.com/af7dcaaf7c0cb2f0f3c75626f7acd368bf98287c/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc [modify] https://crrev.com/af7dcaaf7c0cb2f0f3c75626f7acd368bf98287c/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.h
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f37fee31d526b2197d5b88590474391bc0e717f4 commit f37fee31d526b2197d5b88590474391bc0e717f4 Author: Avi Drissman <avi@chromium.org> Date: Tue Jul 24 19:57:56 2018 Forward events to the tab strip. The shadow around the omnibox results popup eats mouse events. Manually forward events occurring in the shadow area to the underlying widget to fix the tab strip. BUG= 864963 , 838667 TBR=avi@chromium.org (cherry picked from commit af7dcaaf7c0cb2f0f3c75626f7acd368bf98287c) Change-Id: I5c6499f5d024863dad611f8670fb12e1a01ff073 Reviewed-on: https://chromium-review.googlesource.com/1144225 Commit-Queue: Avi Drissman <avi@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577293} Reviewed-on: https://chromium-review.googlesource.com/1148792 Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#50} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/f37fee31d526b2197d5b88590474391bc0e717f4/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc [modify] https://crrev.com/f37fee31d526b2197d5b88590474391bc0e717f4/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.h
,
Sep 10
There is an open TODO citing this bug: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_views_delegate_win.cc?rcl=1929034cb6471d9cf0c37a72595454f5c3d605f9&l=129
,
Sep 10
Indeed, we don't strictly speaking need that patch of code anymore, although it's still slightly more correct WITH that patch of code than without. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by tommycli@chromium.org
, May 1 2018