Correct touch rect hit-test against iframe with scrolled content

When touch rect is hit-tested against an iframe with scrolled
content, the touch rect is translated into the iframe document's
space using the document's scroll offset. It has
a subtle problem that if originally the touch rect is only
partially overlapping with the iframe, after it is translated
into the iframe document's space, the touch rect might be
considered to be fully contained in the document.

Reinforce the detection by asserting that only when the touch
hit-test rect is totally within the iframe's bound in the main
document's space then do we stop the rect-base hit-test search.

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

[Yufeng Shen (Slow to review), 2015-09-02 02:37:22]

miletus@chromium.org changed reviewers:
+ dtapuska@chromium.org, rbyers@chromium.org

[Rick Byers, 2015-09-08 15:12:34]

Sorry for the delay - missed this after getting back from vacation.

https://codereview.chromium.org/1308173007/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1308173007/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:194: &&
nodeAtPointOverWidget(pointOverWidgetResult, locationInContainer,
accumulatedOffset, action))
If this returns false, we'll run it again below.  We only ever need to run
nodeAtPointOverWidget once, right?

[Yufeng Shen (Slow to review), 2015-09-08 18:44:53]

https://codereview.chromium.org/1308173007/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1308173007/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:194: &&
nodeAtPointOverWidget(pointOverWidgetResult, locationInContainer,
accumulatedOffset, action))
On 2015/09/08 15:12:33, Rick Byers wrote:
> If this returns false, we'll run it again below.  We only ever need to run
> nodeAtPointOverWidget once, right?

right, optimized so that nodeAtPointOverWidget() will only be called once.

[Rick Byers, 2015-09-08 19:50:21]

LGTM

[dtapuska, 2015-09-08 20:07:48]

https://codereview.chromium.org/1308173007/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1308173007/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:192: if (isInsideChildFrame) {
I presume this code path isn't necessary for the non rect based hit test..
should it be avoided in that case?


Also is this also an issue for any element that is backed by a ScrollableArea?
ie; does it happen with a div with overflow scroll?

[Yufeng Shen (Slow to review), 2015-09-08 22:08:42]

https://codereview.chromium.org/1308173007/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1308173007/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.cpp:192: if (isInsideChildFrame) {
On 2015/09/08 20:07:48, dtapuska wrote:
> I presume this code path isn't necessary for the non rect based hit test..
> should it be avoided in that case?
> 

done.

> 
> Also is this also an issue for any element that is backed by a ScrollableArea?
> ie; does it happen with a div with overflow scroll?

Yes, the same underlying issue applies also for overflow scroll div,
but the symptom gets mitigated by the fact that we do a re-hit-test
for gesture event, see here

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The re-hit-test uses hitTestResultInFrame. So if the tap finds the wrong frame,
then the click is routed
to the wrong frame. But if the tap finds at least the correct frame but wrong
node, the click is
still routed to the right node. It is really complicated and subtle :(

[Yufeng Shen (Slow to review), 2015-09-08 23:14:57]

The CQ bit was checked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2015-09-08 23:14:58]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1308173007/#ps100001
(title: "fix comments")

[commit-bot: I haz the power, 2015-09-08 23:15:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308173007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308173007/100001

[commit-bot: I haz the power, 2015-09-08 23:23:03]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201935

[Rick Byers, 2015-09-09 14:26:34]

On 2015/09/08 22:08:42, Yufeng Shen wrote:
>
https://codereview.chromium.org/1308173007/diff/60001/Source/core/layout/Layo...
> File Source/core/layout/LayoutPart.cpp (right):
> 
>
https://codereview.chromium.org/1308173007/diff/60001/Source/core/layout/Layo...
> Source/core/layout/LayoutPart.cpp:192: if (isInsideChildFrame) {
> On 2015/09/08 20:07:48, dtapuska wrote:
> > I presume this code path isn't necessary for the non rect based hit test..
> > should it be avoided in that case?
> > 
> 
> done.
> 
> > 
> > Also is this also an issue for any element that is backed by a
ScrollableArea?
> > ie; does it happen with a div with overflow scroll?
> 
> Yes, the same underlying issue applies also for overflow scroll div,
> but the symptom gets mitigated by the fact that we do a re-hit-test
> for gesture event, see here
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> The re-hit-test uses hitTestResultInFrame. So if the tap finds the wrong
frame,
> then the click is routed
> to the wrong frame. But if the tap finds at least the correct frame but wrong
> node, the click is
> still routed to the right node. It is really complicated and subtle :(

Yeah, this sucks.  We're going to have to decide whether to really invest in
rect-based hit testing, or consider it feature for legacy content only.  If the
latter, then perhaps we should just change the re-hit-test work-around to hit
test across all frames to ensure the damage due to touch adjustment bugs is
contained even in iframe cases?