Bug 91012 – [WK2] Send click events to WebCore when the user clicked on a non-special node with TOUCH_ADJUSTMENT enabled.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Hugo Parente Lima]

Created attachment 151764 [details]
Test page

Currently is impossible to remove the focus of a e.g. text field by clicking somewhere on the webpage, this is the cause of the following user interaction bug:

- Open the attached page on MiniBrowser.
- click on the text field, you will get the 2x zoom and the virtual keyboard (at least on Qt port).
- pan the webpage, zoom it out, etc...
- click again on the text field and you will not get the 2x zoom as expected[1].

[1] By "expected" I mean, this is the behavior of n9 and IE on WP7 browsers.

[Hugo Parente Lima]

Created attachment 152066 [details]
Patch

[Hugo Parente Lima]

Btw, there's no tests included. to write a test for this I need to have support to eventSender.gestureTap on WTR or find a way to get a gesture tap recognized sending touch events in the WTR.

[Allan Sandfeld Jensen]

The bug was actually introduced by a change that was made to avoid returning non-clickable targets, because they would be touch-highlighted, which would mean a short highlighting of odd blocks on the page everytime a pan is performed.

The highlighting code will need a different way to tell that the returned target should not be highlighted.

[Allan Sandfeld Jensen]

Never mind the last comment. Highlighting is of course unaffected.

The patch looks good to me. But can you explain to me in which case the adjustment for the viewport is necessary, and what it changes?

[Hugo Parente Lima]

(In reply to comment #4)
> Never mind the last comment. Highlighting is of course unaffected.
> 
> The patch looks good to me. But can you explain to me in which case the adjustment for the viewport is necessary, and what it changes?

Yep, suppose you have a lineedit with 600 pixels width, you click on it, get the 2x zoom, now suppose your viewport isn't too large so just 1/3 of this line widget will be show after the zoom, then when you tap on the line edit the touch adjustment will adjust to touch point to the center of the line edit widget, but the center of the line edit widget is out of the viewport because just the first 1/3 of the widget is show due to the zoom, so webcore ignores the click and you can't focus the line edit.

The change just check if the adjusted point go far the viewport width or viewport height and fixes it.

btw, by viewport I mean FrameView.

[Hugo Parente Lima]

You can reproduce the issue of the viewport with the attached page, running MiniBrowser with --window-size 400x800

[Kenneth Rohde Christiansen]

Comment on attachment 152066 [details]
Patch

This needs a best

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (From update of attachment 152066 [details])
> This needs a best

a test* even :-)

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (In reply to comment #4)
> > Never mind the last comment. Highlighting is of course unaffected.
> > 
> > The patch looks good to me. But can you explain to me in which case the adjustment for the viewport is necessary, and what it changes?
> 
> Yep, suppose you have a lineedit with 600 pixels width, you click on it, get the 2x zoom, now suppose your viewport isn't too large so just 1/3 of this line widget will be show after the zoom, then when you tap on the line edit the touch adjustment will adjust to touch point to the center of the line edit widget, but the center of the line edit widget is out of the viewport because just the first 1/3 of the widget is show due to the zoom, so webcore ignores the click and you can't focus the line edit.
> 
That makes sense, would it be possible to mention that better in the ChangeLog?

Also add a short comment where you remove the !targetNode short-cut, about why we can not take that short cut. Otherwise someone might add it again later.

[Allan Sandfeld Jensen]

An alternative way to do this could be:

-        if (!targetNode)
+        if (!targetNode) {
+            if (Page* page = m_frame->page()) {
+                return page->focusController()->setFocusedNode(0, m_frame);
+            }
             return false;
+        }

It would keep the optimization, and make it unnecessary to adjust for viewport, unless that is also needed in other cases?

[Hugo Parente Lima]

(In reply to comment #10)
> An alternative way to do this could be:
> 
> -        if (!targetNode)
> +        if (!targetNode) {
> +            if (Page* page = m_frame->page()) {
> +                return page->focusController()->setFocusedNode(0, m_frame);
> +            }
>              return false;
> +        }
> 
> It would keep the optimization, and make it unnecessary to adjust for viewport, unless that is also needed in other cases?

The adjust for viewport is needed to focus the element when it isn't entire visible on the viewport, not on the unfocus operation, so with this modification it still needed or you wont be able to focus a text field if it's only e.g. 30% visible on the viewport.

BTW, isn't better to let WK2 go on the same code path of WK1 when the user clicks/touches on an non-link/non-editable node?

[Hugo Parente Lima]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 152066 [details] [details])
> > This needs a best
> 
> a test* even :-)

Yep, I wrote a test, but it wasn't going on the same code path due to the use o synchronized events on WTR, I'll continue to work on it today, but tomorrow I'm going on vacations \o/.

BTW.. a little rant here, why WK2 uses synchronized events/RPC calls on WTR and there are specialized code paths inside WebProcess to deal with this, in other words the tests are testing the test code, not the real code, the ones using async events/RPC calls.

[Allan Sandfeld Jensen]

(In reply to comment #11)

> BTW, isn't better to let WK2 go on the same code path of WK1 when the user clicks/touches on an non-link/non-editable node?

It is definitely safer keep the code-path the same. I was just brain-storming.

[Hugo Parente Lima]

Comment on attachment 152066 [details]
Patch

Yep, so I'll try to somehow get a gesture tap on WTR then re-submit the patch.

[Kevin Ellis]

With patch r122970, the touch adjustment is confined to the touch area, which should help address the issue in comment #5.

[Hugo Parente Lima]

(In reply to comment #15)
> With patch r122970, the touch adjustment is confined to the touch area, which should help address the issue in comment #5.

For sure, now just the removal of the return does the job.

[Hugo Parente Lima]

Created attachment 155862 [details]
Patch

[Hugo Parente Lima]

Comment on attachment 155862 [details]
Patch

This can't be landed until bug 92895 get fixed.

[Allan Sandfeld Jensen]

(In reply to comment #18)
> (From update of attachment 155862 [details])
> This can't be landed until bug 92895 get fixed.

You could probably land it with a manual test-case if you must. Of course if you can find a workable way to make a real test-case, it would be the best solution.

[Hugo Parente Lima]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 155862 [details] [details])
> > This can't be landed until bug 92895 get fixed.
> 
> You could probably land it with a manual test-case if you must. Of course if you can find a workable way to make a real test-case, it would be the best solution.

I'm working on bug 92895 and already have it working, but the patch still crappy, a working in progress, so better to wait a bit while I finish the patch for bug 92895.

[Hugo Parente Lima]

Created attachment 155882 [details]
Patch

Oops, now with layouttests changelog.

[WebKit Review Bot]

Comment on attachment 155882 [details]
Patch

Attachment 155882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13404575

New failing tests:
touchadjustment/focusout-on-touch.html

[WebKit Review Bot]

Created attachment 155897 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Hugo Parente Lima]

Created attachment 156165 [details]
Patch

Test fixed + typos, we can't rely on focusout event because it's always fired when WTR exists.

[Allan Sandfeld Jensen]

Comment on attachment 156165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156165&action=review

I am not a reviewer, so can't even give you an r+, but it looks good to me.
But I did clean up in the touchadjustment tests two days ago, so excuse me for being a little pedantic:

> LayoutTests/touchadjustment/focusout-on-touch.html:1
> +<html>

Add a doctype, since this isn't testing quirky.

> LayoutTests/touchadjustment/focusout-on-touch.html:2
> +<head>

I have added a title to all of the tests in touchadjustment. The most important part is just that the title meantions the bug id.

> LayoutTests/touchadjustment/focusout-on-touch.html:7
> +* {
> +    margin: 0px;
> +    padding: 0px;
> +}

Is this really necessary? Perhaps just set it on body?

[WebKit Review Bot]

Comment on attachment 156165 [details]
Patch

Attachment 156165 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13415756

New failing tests:
touchadjustment/focusout-on-touch.html

[WebKit Review Bot]

Created attachment 156188 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Hugo Parente Lima]

(In reply to comment #25)
> (From update of attachment 156165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156165&action=review
> 
> I am not a reviewer, so can't even give you an r+, but it looks good to me.
> But I did clean up in the touchadjustment tests two days ago, so excuse me for being a little pedantic:
> 
> > LayoutTests/touchadjustment/focusout-on-touch.html:1
> > +<html>
> 
> Add a doctype, since this isn't testing quirky.

Not related with the bug being tested but ok, I can add a doctype for the sake of HTML correctness.

> > LayoutTests/touchadjustment/focusout-on-touch.html:2
> > +<head>
> 
> I have added a title to all of the tests in touchadjustment. The most important part is just that the title meantions the bug id.

Yep, this is a good thing to add.
 
> > LayoutTests/touchadjustment/focusout-on-touch.html:7
> > +* {
> > +    margin: 0px;
> > +    padding: 0px;
> > +}
> 
> Is this really necessary? Perhaps just set it on body?

Yep, it isn't needed, I can remove it for the sake of cleanness.

[Hugo Parente Lima]

Created attachment 156359 [details]
Patch

[Hugo Parente Lima]

Comment on attachment 156359 [details]
Patch

cq- until patch for bug 92895 get landed.

[WebKit Review Bot]

Comment on attachment 156359 [details]
Patch

Attachment 156359 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13423825

New failing tests:
touchadjustment/focusout-on-touch.html

[WebKit Review Bot]

Created attachment 156382 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 156359 [details]
Patch

Rejecting attachment 156359 [details] from commit-queue.

New failing tests:
touchadjustment/focusout-on-touch.html
Full output: http://queues.webkit.org/results/13430493

[WebKit Review Bot]

Created attachment 156417 [details]
Archive of layout-test-results from gce-cq-04

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Terry Anderson]

Just a heads up that https://bugs.webkit.org/show_bug.cgi?id=92914 landed.

[Allan Sandfeld Jensen]

(In reply to comment #33)
> (From update of attachment 156359 [details])
> Rejecting attachment 156359 [details] from commit-queue.
> 
> New failing tests:
> touchadjustment/focusout-on-touch.html
> Full output: http://queues.webkit.org/results/13430493

You need to set this one to expected fail on Chromium until they implement setTouchPointRadius.

[Hugo Parente Lima]

Created attachment 160789 [details]
Patch

Patch rebased, delay due to my vacations =]

[WebKit Review Bot]

Comment on attachment 160789 [details]
Patch

Clearing flags on attachment: 160789

Committed r126873: <http://trac.webkit.org/changeset/126873>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.