WebViewImpl's hit tests should use setToShadowHostIfInUserAgentShadowRoot.

WebViewImpl's hit tests should use setToShadowHostIfInUserAgentShadowRoot.

This code in WebViewImpl never expects to end up inside a UA shadow root,
so call setToShadowHostIfInUserAgentShadowRoot() after doing each hit test.

Unfortunately there's no obvious way to test context menus right now.

I also added a bunch of FIXME's related to parentNode() traversals inside
WebViewImpl that are not ShadowDOM aware.

BUG=402026
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181071

[esprehn, 2014-08-14 01:38:50]

The alternative to this is to revert
https://src.chromium.org/viewvc/blink?revision=176900&view=revision

and then make the old "ConfusingAndOftenMisusedDisallowShadowContent" flag into
something like "DisallowUserAgentShadowContents" and change it's meaning to be
that of setToShadowHostIfInUserAgentShadowRoot() which would fix all these bugs
and the other ones that I possibly caused.

[abarth-chromium, 2014-08-14 03:56:48]

@rbyers: Thoughts?

[Rick Byers, 2014-08-15 18:10:58]

Sorry for the delay, still digging my way out of vacation e-mail.

This looks reasonable to me, but needs at least some test (I guess ideally each
scenario would be covered by a test, but that's probably not worth the cost).

I personally prefer explicit calls to this over reintroducing a hit-test flag
for this.  This seems pretty subtle though - I expect we'll need to continue to
iterate on the edge cases as we find them (as I've found with almost every
non-trivial hit-testing change I've tried to make).

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1181: // FIXME: This wants to walk composed tree with
NodeRenderingTraversal::parent().
Sorry about this - I just asked for it to be fixed in a review before I left on
vacation, but it looks like that fix got lost. 
https://codereview.chromium.org/402603005/

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:3877:
result.setToShadowHostIfInUserAgentShadowRoot();
Are you sure we want this for all callers of this method?  It certainly seems
like the more reasonable default behavior for code in WebViewImpl, but I don't
know enough about each of the cases to say there won't be some regression from
this change.

[esprehn, 2014-08-16 04:14:58]

I'm not sure how to write tests at this layer of the system. We should probably
do something though, I don't think we can leave M38 broken.

I almost wonder if hit tests should always come up out of UA shadow roots unless
you explicit ask them not to. Otherwise you could imagine badness where new
callers are added, assume things are fine, then a new element like <marquee>
gets a UA ShadowRoot and they break.

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:3877:
result.setToShadowHostIfInUserAgentShadowRoot();
Looking at the callers of this method, and their transitive callers they all
seem to either not care about the node type, or assume that you never end up
inside UA ShadowDOM.

I think this is right.

[Rick Byers, 2014-08-18 16:35:12]

On 2014/08/16 04:14:58, esprehn wrote:
> I'm not sure how to write tests at this layer of the system. We should
probably
> do something though, I don't think we can leave M38 broken.

Couldn't you just write a WebViewTest unit test that triggers one of these code
paths with a constructed scenario involving UA shadow DOM?

Solving this at a lower level (and testing it there) does perhaps seems better
though.  The impossible test matrix of every scenario that involves a hit test
with all special hit test scenarios (such as UA shadow DOM) suggests that
requiring each call site to be aware of this is - as you suggest - a likely bug
farm.

> I almost wonder if hit tests should always come up out of UA shadow roots
unless
> you explicit ask them not to. Otherwise you could imagine badness where new
> callers are added, assume things are fine, then a new element like <marquee>
> gets a UA ShadowRoot and they break.

To me this seems potentially similar to the problem of dealing with pseudo
nodes.  Perhaps innerNode should never be inside UA shadow DOM, but there's a
new innerPossiblyUAShadow method on HitTestResult?  Especially as we move to
share HitTestResult objects across more and more consumers, we should try to
minimize the input configurations to the hit test, but having more
details/choices in the output seems fine.

> https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
> Source/web/WebViewImpl.cpp:3877:
> result.setToShadowHostIfInUserAgentShadowRoot();
> Looking at the callers of this method, and their transitive callers they all
> seem to either not care about the node type, or assume that you never end up
> inside UA ShadowDOM.
> 
> I think this is right.

[esprehn, 2014-08-19 23:36:15]

Can someone give pointers on how to write tests? I think we need to do something
for M38 which just branched. Perhaps someone else can take over this fix if they
think there's a better way to solve it.

[Rick Byers, 2014-08-20 14:18:22]

On 2014/08/19 23:36:15, esprehn wrote:
> Can someone give pointers on how to write tests?

I don't know the specific scenarios you're changing here, but in general I think
you'd want a new WebViewTest with some suitably interesting shadow-dom test page
and then sent fake mouse events and observed the behavior.  Eg. same sort of
structure as WebViewTest::FirstUserGestureObservedMouseEvent
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
but tailored to the scenarios you're affecting.

>  I think we need to do something
> for M38 which just branched. Perhaps someone else can take over this fix if
they
> think there's a better way to solve it.

I'm swamped with my own M38 blockers right now (several of them) - sorry.  But
I'm fine with this CL as written for the purposes of M38.  The argument I made
at https://codereview.chromium.org/486713003/#msg7 could be applied to say we
shouldn't worry about exhaustively testing all of these scenarios.  But I think
you could take that further and say that HitTestResult::innerNode shouldn't
include UA shadow DOM by default, but there should be new shadow-DOM aware
accessors on HitTestResult.  I'd be fine with that being a FIXME that you worry
about for M39 instead of holding up this M38 fix any longer though.

[esprehn, 2014-08-27 20:17:45]

Can I get an lgtm then?

[Rick Byers, 2014-08-27 20:35:10]

LGTM (modulo merge with trunk nits)
I'm not an owner of Source/web though.

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1181: // FIXME: This wants to walk composed tree with
NodeRenderingTraversal::parent().
On 2014/08/15 18:10:57, Rick Byers wrote:
> Sorry about this - I just asked for it to be fixed in a review before I left
on
> vacation, but it looks like that fix got lost. 
> https://codereview.chromium.org/402603005/

You'll need to merge with trunk and remove this comment (already fixed).

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1212: // FIXME: This wants to walk composed tree with
NodeRenderingTraversal::parent().
ditto

https://codereview.chromium.org/470803002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1229: // FIXME: This wants to walk composed tree with
NodeRenderingTraversal::parent().
ditto

[scherkus (not reviewing), 2014-08-27 20:43:35]

scherkus@chromium.org changed reviewers:
+ scherkus@chromium.org

[scherkus (not reviewing), 2014-08-27 20:43:35]

where are we with this CL?

as it stands blink r176900 regressed media context menus in M38 and we need
either a revert or a fix+merge

[esprehn, 2014-08-29 02:42:59]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-08-29 02:43:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/esprehn@chromium.org/470803002/20001

[commit-bot: I haz the power, 2014-08-29 02:47:36]

Committed patchset #2 (id:20001) as 181071