Fix touch accessibility events in WebViews and iframes

Fix touch accessibility events in WebViews and iframes

This only affects Chrome OS devices with touch screens with spoken feedback
enabled. In that mode, touching the screen sends a mouse move event instead
of a touch down event, and it sets a special touch accessibility flag, and
inside Blink that gets translated into a HOVER event on the accessibility
object the event targets.

This change fixes two specific cases:
* Tapping in a WebView was sending a HOVER event on both the embedder frame
  and the guest frame. Fix this by ignoring touch accessibility events on a
  LayoutPart.
* Tapping in an iframe wasn't translating the coordinates properly, so the
  hit test was sometimes returning the wrong object.

It also adds tests and consolidates some test helper functions into a
common file.

BUG=613694
Committed: https://crrev.com/f9e11986fc63ddfd1f20c88b72530fd6b5af9dba
Cr-Commit-Position: refs/heads/master@{#397325}

[dmazzoni, 2016-05-25 17:13:55]

dmazzoni@chromium.org changed reviewers:
+ dcheng@chromium.org, fsamuel@chromium.org

[Fady Samuel, 2016-05-25 17:22:27]

I haven't worked on <webview> in a while. Could you please pass this along to
wjmaclean@? Thanks!

[dcheng, 2016-05-25 17:44:38]

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2159: if (result.innerNodeFrame())
{
I like this because it looks simpler, but I don't think I understand why this
new version is more correct. Do you mind enlightening me?

[dmazzoni, 2016-05-25 17:50:34]

dmazzoni@chromium.org changed reviewers:
+ wjmaclean@chromium.org
- fsamuel@chromium.org

[dmazzoni, 2016-05-25 17:50:35]

+wjmaclean instead of fsamuel

[dmazzoni, 2016-05-25 17:53:51]

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2159: if (result.innerNodeFrame())
{
On 2016/05/25 17:44:38, dcheng wrote:
> I like this because it looks simpler, but I don't think I understand why this
> new version is more correct. Do you mind enlightening me?

I don't think result.innerNodeOrImageMapImage()->document().frame() was
incorrect - it seemed to return the same result for the cases I tried.
result.innerNodeFrame() does seem cleaner.

The critical bug was below, passing pme.position() rather than
result.roundedPointInInnerNodeFrame().

[dcheng, 2016-05-25 18:02:59]

dcheng@chromium.org changed reviewers:
+ pdr@chromium.org

[dcheng, 2016-05-25 18:03:00]

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2159: if (result.innerNodeFrame())
{
On 2016/05/25 at 17:53:51, dmazzoni wrote:
> On 2016/05/25 17:44:38, dcheng wrote:
> > I like this because it looks simpler, but I don't think I understand why
this
> > new version is more correct. Do you mind enlightening me?
> 
> I don't think result.innerNodeOrImageMapImage()->document().frame() was
incorrect - it seemed to return the same result for the cases I tried.
result.innerNodeFrame() does seem cleaner.
> 
> The critical bug was below, passing pme.position() rather than
result.roundedPointInInnerNodeFrame().

Hmm. It looks like the original code is from the initial upstreaming of
WebViewImpl... +pdr who touched some hittesting stuff recently.

Intuitively this makes sense: I don't think you should be able to hittest a
detached document (which is what a null frame would imply), but I'm not sure if
there's an edge case I'm missing here.

[wjmaclean, 2016-05-25 18:04:46]

On 2016/05/25 17:53:51, dmazzoni wrote:
>
https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:2159: if
(result.innerNodeFrame())
> {
> On 2016/05/25 17:44:38, dcheng wrote:
> > I like this because it looks simpler, but I don't think I understand why
this
> > new version is more correct. Do you mind enlightening me?
> 
> I don't think result.innerNodeOrImageMapImage()->document().frame() was
> incorrect - it seemed to return the same result for the cases I tried.
> result.innerNodeFrame() does seem cleaner.
> 
> The critical bug was below, passing pme.position() rather than
> result.roundedPointInInnerNodeFrame().

lgtm for web_view_browsertest + related files

[pdr., 2016-05-26 21:32:48]

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2009283002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2159: if (result.innerNodeFrame())
{
On 2016/05/25 at 18:03:00, dcheng wrote:
> On 2016/05/25 at 17:53:51, dmazzoni wrote:
> > On 2016/05/25 17:44:38, dcheng wrote:
> > > I like this because it looks simpler, but I don't think I understand why
this
> > > new version is more correct. Do you mind enlightening me?
> > 
> > I don't think result.innerNodeOrImageMapImage()->document().frame() was
incorrect - it seemed to return the same result for the cases I tried.
result.innerNodeFrame() does seem cleaner.
> > 
> > The critical bug was below, passing pme.position() rather than
result.roundedPointInInnerNodeFrame().
> 
> Hmm. It looks like the original code is from the initial upstreaming of
WebViewImpl... +pdr who touched some hittesting stuff recently.
> 
> Intuitively this makes sense: I don't think you should be able to hittest a
detached document (which is what a null frame would imply), but I'm not sure if
there's an edge case I'm missing here.

This change seems reasonable to me. LGTM

[dmazzoni, 2016-05-31 21:27:01]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-31 21:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wjmaclean@chromium.org
Link to the patchset: https://codereview.chromium.org/2009283002/#ps40001
(title: "Fix WebInputEventConversion")

[commit-bot: I haz the power, 2016-05-31 21:27:33]

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

[commit-bot: I haz the power, 2016-05-31 23:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 23:16:31]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-06-01 16:38:16]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-06-01 16:38:39]

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

[commit-bot: I haz the power, 2016-06-01 19:00:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 19:00:46]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-06-01 22:16:33]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-01 22:16:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wjmaclean@chromium.org
Link to the patchset: https://codereview.chromium.org/2009283002/#ps60001
(title: "Disable cross-site test temporarily")

[commit-bot: I haz the power, 2016-06-01 22:17:52]

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

[commit-bot: I haz the power, 2016-06-02 02:04:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 02:04:24]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-06-02 04:33:37]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-06-02 04:33:58]

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

[commit-bot: I haz the power, 2016-06-02 05:14:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 05:14:01]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-06-02 05:55:39]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-06-02 05:56:06]

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

[commit-bot: I haz the power, 2016-06-02 07:03:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-02 07:06:05]

Description was changed from

==========
Fix touch accessibility events in WebViews and iframes

This only affects Chrome OS devices with touch screens with spoken feedback
enabled. In that mode, touching the screen sends a mouse move event instead
of a touch down event, and it sets a special touch accessibility flag, and
inside Blink that gets translated into a HOVER event on the accessibility
object the event targets.

This change fixes two specific cases:
* Tapping in a WebView was sending a HOVER event on both the embedder frame
  and the guest frame. Fix this by ignoring touch accessibility events on a
  LayoutPart.
* Tapping in an iframe wasn't translating the coordinates properly, so the
  hit test was sometimes returning the wrong object.

It also adds tests and consolidates some test helper functions into a
common file.

BUG=613694
==========

to

==========
Fix touch accessibility events in WebViews and iframes

This only affects Chrome OS devices with touch screens with spoken feedback
enabled. In that mode, touching the screen sends a mouse move event instead
of a touch down event, and it sets a special touch accessibility flag, and
inside Blink that gets translated into a HOVER event on the accessibility
object the event targets.

This change fixes two specific cases:
* Tapping in a WebView was sending a HOVER event on both the embedder frame
  and the guest frame. Fix this by ignoring touch accessibility events on a
  LayoutPart.
* Tapping in an iframe wasn't translating the coordinates properly, so the
  hit test was sometimes returning the wrong object.

It also adds tests and consolidates some test helper functions into a
common file.

BUG=613694

Committed: https://crrev.com/f9e11986fc63ddfd1f20c88b72530fd6b5af9dba
Cr-Commit-Position: refs/heads/master@{#397325}
==========

[commit-bot: I haz the power, 2016-06-02 07:06:07]

Patchset 4 (id:??) landed as
https://crrev.com/f9e11986fc63ddfd1f20c88b72530fd6b5af9dba
Cr-Commit-Position: refs/heads/master@{#397325}