I'll take this since I'm more familiar with that part.

Let me know if you need help with the cross-frame propagation. There are a few other areas where we need to pass information about the iframe element to the renderer for its frame, and I have a bug that I assigned to myself to make it the propagation path more sensible (issue 750179).

Note that this also affects touch-action for elements inside the OOPIF. touch-action can prevent panning, but not pinching.

Example:
Visit http://csreis.github.io/tests/cross-site-iframe-simple.html
Set touch-action: pan-x pan-y; on the body of the iframe's document.
Pinching inside the iframe causes zooming.

I see this is marked as P3.  Have we confirmed that we're OK with the web compat implications of site isolation shipping without this?  I.e. do we have (can we get) UMA data on how many touches occur in an iframe in which behavior will change (be broken) without fixing this?

This does not seem like a P3 to me, but also it hasn't seen any activity in a while. Is anyone actively looking at this? If not, it should be made available so someone else can pick it up.

rbyers@, I'd be looking to you to give us guidance of whether this is a ship blocking bug or not.

Thanks.  I defer to dtapuska - priority is probably an input team question (though we likely can't answer that without some UMA data).

dtapuska@: Can you help us find an owner for this, since sunxd@ isn't responding?  We should aim to fix this in M67, and possibly merge to M66 if it's important enough for beta trials.

I'm OOO next week, flackr@ is going to coordinate with kenrb@ and sunxd@ on this to get it in motion.

We talked this through, we should generalize passing the inherited touch action to the child document. This should be similar to Frame::SetIsInert which RemoteFrame overrides with an IPC (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/RemoteFrame.cpp?sq=package:chromium&l=137) which ultimately calls into WebFrameWidgetImpl::SetIsInert https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/WebFrameWidgetImpl.cpp?type=cs&q=WebFrameWidgetImpl::SetIsInert&l=675.

Ken proposed combining the inherited touch action with the inert bool so that all inherited frame properties can be passed using the same message rather than needing to add a message for each individual property. Adding additional methods may be easier when this uses mojo.

The inherited touch action can then be set on the owner or the child frame such that StyleAdjuster can access it and propagate it appropriately.

Then, we need to ensure that touch action regions within the child frame are respected, which from the original bug report it sounds like it may already be the case.

Can we get an update on whether this is on track to land in M67?

I'm working on this patch right now.

I have a WIP patch: https://chromium-review.googlesource.com/c/chromium/src/+/982154

What is the exact user impact for this?

When running in site isolation mode, user would be able to pinch zoom an iframe's content even if the iframe's parent forbids pinch zooming.

Re c16 ... is that true? We don't pass GesturePinch events to subframes, only the mainframe. See

https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_input_event_router.cc?rcl=627a4d1cb2a7ab83378aeb4b97e3b215e6a90593&l=1055

I just tried the repro in C1, and it doesn't work with --site-per-process enabled.

Or am I missing something subtler here.

wjmaclean@: I cannot reproduce it on desktop (with device mode) or mac, but I can pinch zoom the iframe on a nexus 6. Does the hit testing work differently on android? On both desktop and android, the effective touch action of subframes are wrong though.

Ignore me, I think I misunderstood the intent of the repro.

I created a bin which demonstrates the two cases I'm aware of: http://output.jsbin.com/tozizur/quiet

The first frame has an element which disables pinching, and the second has pinch disabled on the iframe element.

Re c18: Pinch zoom may behave differently on Android ... I'd have to check.

Re c20: ok, both of those definitely repro (i.e. broken) with --site-per-process.

I can confirm that http://jsbin.com/nimurap incorrectly allows pinch zoom on a Chromebook Pixel running 67.0.3381.0 with Site Isolation (--site-per-process), and that prevents it when Site Isolation is disabled (using the Site Isolation trial opt-out flag on chrome://flags).

sunxd@: Is https://chromium-review.googlesource.com/c/chromium/src/+/982154 still on track?

Also, is  issue 827182  is just about the green div case in http://output.jsbin.com/tozizur/quiet?

I'm tentatively treating both of these as blockers, given that we're aiming to enable Site Isolation on desktop in M67.

sunxd@: Can you please post an update on your status?  Branch cut is less than 1 week away, and we'd like to get this fixed by then.  Thanks!

wjmaclean@ and I will talk about  issue 827182  next Monday. I believe the touch-action is propagated correctly but the effective touch action is not respected.

sunxd@: Friendly ping on this one as well.  Branch cut is tomorrow-- is the fix on track to land?  Thanks!

sunxd@: What's the status on the fix for this?  Branch cut was last week, so please try to land a fix ASAP so that it can be merged to M67.

M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

I currently have the fix but I think the fix involves too many files and it might not be safe to merge it into M67. Since this we are not enabling site isolation for android, this only influence desktop touch screen when there frame owner does not allow pinch/double tap, I think we can make it block M68.

Ok.  We'll keep it as RBS for M68 but I'll remove the launch blocking label for Site Isolation.  Let's continue to try to get the fix in soon.  Thanks for the update!

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

commit 3e0ee34fa47a3558be52ac7e85e3e38d4a5eced5
Author: sunxd <sunxd@chromium.org>
Date: Thu May 24 14:08:09 2018

Determine the existence of touch handler by effective touch action

LayoutObject used to determine whether there is a touch event handler by
checking if it has touch-action property set. However, after we enable
site isolation, a LayoutObject may still have touch event handler if it
inherits touch-action from the parent frame.

This CL makes LayoutObject determine the existence by looking at its
effective touch action.

Bug:  759042 
Change-Id: I60e868090a7725757a910bfca8766ec430cf72aa
Reviewed-on: https://chromium-review.googlesource.com/1040350
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Xianda Sun <sunxd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561492}
[modify] https://crrev.com/3e0ee34fa47a3558be52ac7e85e3e38d4a5eced5/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/3e0ee34fa47a3558be52ac7e85e3e38d4a5eced5/third_party/blink/renderer/core/layout/layout_object_test.cc

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

commit 540a9960a86fed502af0692e9987da8f690f66b6
Author: sunxd <sunxd@chromium.org>
Date: Thu May 24 22:51:06 2018

Make effective touch action flow across OOPIFs

Elements inside an out-of-process iframe should inherit the effective
touch action of the parent frame, this CL propagates the computed touch
action to all of the element's OOPIF children.

Bug:  759042 
Change-Id: I2ddfbd0a75ad7166a6ec94a44e039ea7384c7947
Reviewed-on: https://chromium-review.googlesource.com/982154
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Xianda Sun <sunxd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561668}
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/frame_host/cross_process_frame_connector.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/frame_host/cross_process_frame_connector.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/renderer_host/frame_connector_delegate.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/renderer_host/frame_connector_delegate.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/renderer_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/common/frame_messages.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/common/input/touch_action_optional_struct_traits.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/common/input_messages.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/common/view_messages.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/renderer/render_frame_proxy.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/renderer/render_widget.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/renderer/render_widget.h
[add] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/content/test/data/frame_tree/page_with_iframe_in_div.html
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/testing/buildbot/filters/viz.content_browsertests.filter
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/public/web/web_frame_widget.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/public/web/web_remote_frame_client.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/css/resolver/style_adjuster.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/frame.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/frame.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/local_frame.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/local_frame.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/remote_frame.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/remote_frame.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/frame/web_frame_widget_impl.h
[modify] https://crrev.com/540a9960a86fed502af0692e9987da8f690f66b6/third_party/blink/renderer/core/loader/empty_clients.h

[Auto-generated comment by a script] We noticed that this issue is targeted for M-68; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-68 label, otherwise remove Merge-TBD label. Thanks.

I thought branch point was May 24, but the cl landed a few hours after, requesting merging to m68.

Your change meets the bar and is auto-approved for M68. Please go ahead and merge the CL to branch 3440 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

Actually, I don't think a merge is required here-- the fix landed at r561668 (68.0.3440.0), and the branch happened at r561733.  Indeed, I can confirm this is working correctly on ChromeOS Dev channel 68.0.3440.4 on a Chromebook Pixel 1, using the repro at http://jsbin.com/nimurap.  (It was not working on version 68.0.3437.0, before I updated.)

Thanks for the fix!

[Auto-generated comment by a script] We noticed that this issue is targeted for M-68; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-68 label, otherwise remove Merge-TBD label. Thanks.

Updating again, based on comment 37.