See the image to see what happened.

Deleted: bug.png 14.1 KB

	bug.png 14.1 KB View Download

Bisected to r559213 "Add finch testing config for VizHitTestDrawQuad."
Confirmed by disabling the feature via command line "chrome --disable-features=VizHitTestDrawQuad"
and observing the bug is gone in snapshot builds until r583548 that enabled Viz by default and the switch stopped working.
The bug requires site-per-process which was enabled since r552589 by default.

Bisected with both conditions force-enabled:
chrome --enable-features=VizHitTestDrawQuad --site-per-process --use-viz-hit-test-draw-quad
https://chromium.googlesource.com/chromium/src/+log/05582e88..7e152cf5?pretty=fuller
Suspecting r536389 = 15ae8f16cbad299b2a98a051b621a9d15c42201b = https://crrev.com/c/916402 by riajiang@chromium.org
"Set event handling flags in browser hit-test data."
Landed in 66.0.3347.0 (disabled by default)

Able to reproduce the issue on chrome reported version# 70.0.3538.110 using Windows-10, Mac 10.14.1 and Ubuntu 14.04, hence providing bisect info

Note: With default chrome settings unable to reproduce the issue but "resetting the flags to default" in chrome://flags then we are able to reproduce the issue. 
As per comment# 2 assigning it to the author of the CL

Change Log: https://chromium.googlesource.com/chromium/src/+log/05582e88..7e152cf5?pretty=fuller
Suspect: https://chromium.googlesource.com/chromium/src/+/15ae8f16cbad299b2a98a051b621a9d15c42201b
Change-Id: I3cc2a564400b690c1ae7f1cb9c079cfdf4b2b43c
Reviewed-on: https://chromium-review.googlesource.com/916402

@Ria Jiang: Please confirm the issue and help in re-assigning if it is not related to your change.

Thanks!

See also https://bugs.chromium.org/p/chromium/issues/detail?id=909702 and also the linked fiddle for reproduction https://jsfiddle.net/w0s4xmc0/54570/ for an easy reproduction

It is a known issue that drag-and-drop doesn't work with OOPIFs (see https://bugs.chromium.org/p/chromium/issues/detail?id=804633), regardless of using Viz hit-testing or not. +kenrb@ for insights into plans to get dnd working with OOPIFs.

Drag and drop does work with OOPIFs, it just can't use asynchronous hit testing because of the nested event loop. Is there anything in this test case that would require async hit testing for the targeting to work correctly?

Yea when there's a transparent div on top of the OOPIF, we need the async hit testing to tell us that div should be the real target.

Do we not get a HitTestRegion for a transparent div?

No we don't have hit-test regions for divs, we need blink targeting to find those

 Issue 909702  has been merged into this issue.

I don't think anybody has been investigating how to fix this properly. It is a difficult problem. Viz hit testing v3 will fix this for most cases, correct? Then it will be limited to the difficult situations like rounded corners, IIRC.

 Issue 908815  has been merged into this issue.

Viz hit-testing v3 is very unclear at this moment and we are not actively looking at that. What info would Viz need for v3 to fix this?

I'm experiencing the same issue (https://bugs.chromium.org/p/chromium/issues/detail?id=907272). Is there a known workaround or ETA for the fix? 

There are two known cases that Viz hit-testing v1 behaves differently for drag-n-drop:

1. When the OOPIF underneath the div has pointer-events:none set, v1 currently relies on async targeting while drag-n-drop expects sync results. I'm looking at adding these information to hit-test data.

2. When the div on top of the OOPIF has content (i.e. generates draw quad), v1 also relies on async targeting but non-viz-hit-testing checks all draw quads. I'm investigating the best way to fix this.

+creis@, nzolghadr@, wjmaclean@, sunxd@

For the second case mentioned in c#17 (details in https://bugs.chromium.org/p/chromium/issues/detail?id=907272), there's a possible solution that does a second pass of all the draw-quads and check if there's any other draw-quad (e.g. PICTURE_CONTENT, SOLID_COLOR, STREAM_VIDEO_CONTENT, TEXTURE_CONTENT, TILED_CONTENT, YUV_VIDEO_CONTENT quads) that's on top of the OOPIFs we've added to the hit-test data list. However, this might have a performance penalty because of the increased pre-processing time to generate hit-test data; it is also a short term fix for v1 (this fix doesn't apply to v2). 

A better fix would be to change drag-n-drop to accept asynchronous targeting result so we can guarantee that it always get the correct target when OOPIF is present (there might be other unknown bugs for drag-n-drop with OOPIFs). 

Is there any plans to get drag-n-drop working with OOPIFs?

It's been a while since we looked at this, but I don't think we had a clear path to making drag and drop compatible with asynchronous hit testing. The problem is that when a drag is in progress, the platform runs a nested event loop, so we can't process asynchronous hit test results.

Sounds like this is blocked on issue 804633.

Any updates on this problem?

The fix for pointer-events:none is in review https://chromium-review.googlesource.com/c/chromium/src/+/1372322 which can fix some cases. There's no one actively looking at fixing drag-n-drop for OOPIFs in general.

Ria, if there are different cases underlying this issue, perhaps you can create a tracking bug, and then case-specific bugs underneath it?

Yea drag-n-drop for OOPIFs is being tracked here https://bugs.chromium.org/p/chromium/issues/detail?id=804633, drag-n-drop cases that require async targeting to find the correct view don't work

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

commit 25fb6af16067c0cb4364cbc60eaff161bf5b5253
Author: Ria Jiang <riajiang@chromium.org>
Date: Wed Jan 09 22:01:58 2019

Ignore OOPIFs with pointer-events:none set during targeting.

Currently we rely on async targeting to handle pointer-events:none;
however, drag-n-drop expects sync result so that is not possible. This
CL adds pointer-events:none info to SurfaceDrawQuad and don't send
hit-test data for that region. This flag should be removed when v2 is
enabled by default.

Bug:  908750 
Test: SitePerProcessHitTestDataGenerationBrowserTest.PointerEventsNoneOOPIF/1
Change-Id: I23e0185e86af6ca405aa5ef960ae4fd78525dcbd
Reviewed-on: https://chromium-review.googlesource.com/c/1372322
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621300}
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/cc/layers/surface_layer_impl.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/cc/mojo_embedder/async_layer_tree_frame_sink_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/client/hit_test_data_provider_draw_quad.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/client/hit_test_data_provider_draw_quad_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/common/quads/draw_quad_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/common/quads/surface_draw_quad.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/common/quads/surface_draw_quad.h
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/service/display/surface_aggregator_perftest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/service/display/surface_aggregator_pixeltest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/service/frame_sinks/direct_layer_tree_frame_sink_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/service/frame_sinks/video_detector_unittest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/components/viz/test/surface_hittest_test_helpers.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/content/renderer/android/synchronous_layer_tree_frame_sink.cc
[modify] https://crrev.com/25fb6af16067c0cb4364cbc60eaff161bf5b5253/services/viz/public/cpp/compositing/struct_traits_unittest.cc

CL in c#25 should fix the pointer-events:none case. The more general one is being tracked in https://chromium-review.googlesource.com/c/chromium/src/+/1372322 so I'm going to close this one.

Requesting a merge for c#25 to M72 as it's affecting Google Site embed code case https://bugs.chromium.org/p/chromium/issues/detail?id=907272

This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Seems like this this bug has been around for quite a while, and the change seems quite large. Can we target this for M73? Or are there other requirements that make this more critical? We're only 2 weeks from M72 stable, and we'd like to minimize risk. 

Most changes are just function signature changes in tests because we added one more field (pointer-events:none) and would drop the hit-test region if that's set. This CL could help fix Google Site's (tianm@) embed code use case (https://bugs.chromium.org/p/chromium/issues/detail?id=907272) so they'd like it to ship to stable sooner.

This bug impacts both embedded code and third party iframe embeds in Google Sites, which make up more than half of embed usage in Sites. We've received many customer reports and help forum posts about this issue in the past two months. 

Thanks all for the feedback. I'm still a bit hesitant to take this merge. Looking at the CL, there are 5 or so non-test files being modified. 

Riajiang@ - are you 100% confident this won't introduce any new regressions in M72? Since we're only 2 weeks away from stable, this change is also not behind a flag, I'd like to ensure we only take critical and safe merges. 

The real change is for cc::SurfaceLayerImpl to pass its |has_pointer_events_none_| information so it should be safe. Although VizHitTestDrawQuad has been turned on by default on non-cros platforms, it is still controlled by a finch experiment. 

Ok sounds good - approving merge to M72. Branch:3626

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

Commit: 3262600289f41303c83f6e86ec1824f5e73bddd7
Author: riajiang@chromium.org
Commiter: riajiang@chromium.org
Date: 2019-01-16 23:10:32 +0000 UTC

Merge M72: Ignore OOPIFs with pointer-events:none set during targeting.

Currently we rely on async targeting to handle pointer-events:none;
however, drag-n-drop expects sync result so that is not possible. This
CL adds pointer-events:none info to SurfaceDrawQuad and don't send
hit-test data for that region. This flag should be removed when v2 is
enabled by default.

Bug:  908750 
Test: SitePerProcessHitTestDataGenerationBrowserTest.PointerEventsNoneOOPIF/1
Change-Id: I23e0185e86af6ca405aa5ef960ae4fd78525dcbd
Reviewed-on: https://chromium-review.googlesource.com/c/1372322
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#621300}
Reviewed-on: https://chromium-review.googlesource.com/c/1415840
Reviewed-by: Ria Jiang <riajiang@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#716}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

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

commit 3262600289f41303c83f6e86ec1824f5e73bddd7
Author: Ria Jiang <riajiang@chromium.org>
Date: Wed Jan 16 23:10:32 2019

Merge M72: Ignore OOPIFs with pointer-events:none set during targeting.

Currently we rely on async targeting to handle pointer-events:none;
however, drag-n-drop expects sync result so that is not possible. This
CL adds pointer-events:none info to SurfaceDrawQuad and don't send
hit-test data for that region. This flag should be removed when v2 is
enabled by default.

Bug:  908750 
Test: SitePerProcessHitTestDataGenerationBrowserTest.PointerEventsNoneOOPIF/1
Change-Id: I23e0185e86af6ca405aa5ef960ae4fd78525dcbd
Reviewed-on: https://chromium-review.googlesource.com/c/1372322
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#621300}
Reviewed-on: https://chromium-review.googlesource.com/c/1415840
Reviewed-by: Ria Jiang <riajiang@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#716}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/cc/layers/surface_layer_impl.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/client/hit_test_data_provider_draw_quad.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/client/hit_test_data_provider_draw_quad_unittest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/common/quads/draw_quad_unittest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/common/quads/surface_draw_quad.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/common/quads/surface_draw_quad.h
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/service/display/display_unittest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/service/display/surface_aggregator_perftest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/service/display/surface_aggregator_pixeltest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/service/display/surface_aggregator_unittest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/service/frame_sinks/direct_layer_tree_frame_sink_unittest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/service/frame_sinks/video_detector_unittest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/components/viz/test/surface_hittest_test_helpers.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/content/renderer/android/synchronous_layer_tree_frame_sink.cc
[modify] https://crrev.com/3262600289f41303c83f6e86ec1824f5e73bddd7/services/viz/public/cpp/compositing/struct_traits_unittest.cc