Bug 91012 - [WK2] Send click events to WebCore when the user clicked on a non-special node with TOUCH_ADJUSTMENT enabled.
Summary: [WK2] Send click events to WebCore when the user clicked on a non-special nod...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hugo Parente Lima
URL:
Keywords:
Depends on: 92895 95205
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-11 13:50 PDT by Hugo Parente Lima
Modified: 2012-08-28 08:17 PDT (History)
9 users (show)

See Also:


Attachments
Test page (1.16 KB, text/html)
2012-07-11 13:50 PDT, Hugo Parente Lima
no flags Details
Patch (2.70 KB, patch)
2012-07-12 14:39 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2012-08-01 12:36 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (4.68 KB, patch)
2012-08-01 14:14 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (654.55 KB, application/zip)
2012-08-01 15:10 PDT, WebKit Review Bot
no flags Details
Patch (4.91 KB, patch)
2012-08-02 14:23 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (389.49 KB, application/zip)
2012-08-02 15:56 PDT, WebKit Review Bot
no flags Details
Patch (5.09 KB, patch)
2012-08-03 06:51 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (374.42 KB, application/zip)
2012-08-03 07:51 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cq-04 (566.12 KB, application/zip)
2012-08-03 10:56 PDT, WebKit Review Bot
no flags Details
Patch (6.02 KB, patch)
2012-08-27 13:14 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hugo Parente Lima 2012-07-11 13:50:55 PDT
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.
Comment 1 Hugo Parente Lima 2012-07-12 14:39:56 PDT
Created attachment 152066 [details]
Patch
Comment 2 Hugo Parente Lima 2012-07-12 14:41:24 PDT
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.
Comment 3 Allan Sandfeld Jensen 2012-07-12 14:57:26 PDT
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.
Comment 4 Allan Sandfeld Jensen 2012-07-12 15:18:40 PDT
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?
Comment 5 Hugo Parente Lima 2012-07-12 15:26:03 PDT
(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.
Comment 6 Hugo Parente Lima 2012-07-12 15:29:11 PDT
You can reproduce the issue of the viewport with the attached page, running MiniBrowser with --window-size 400x800
Comment 7 Kenneth Rohde Christiansen 2012-07-12 20:20:45 PDT
Comment on attachment 152066 [details]
Patch

This needs a best
Comment 8 Kenneth Rohde Christiansen 2012-07-12 20:21:11 PDT
(In reply to comment #7)
> (From update of attachment 152066 [details])
> This needs a best

a test* even :-)
Comment 9 Allan Sandfeld Jensen 2012-07-13 01:01:37 PDT
(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.
Comment 10 Allan Sandfeld Jensen 2012-07-13 01:30:26 PDT
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?
Comment 11 Hugo Parente Lima 2012-07-13 11:02:58 PDT
(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?
Comment 12 Hugo Parente Lima 2012-07-13 11:09:12 PDT
(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.
Comment 13 Allan Sandfeld Jensen 2012-07-13 11:13:21 PDT
(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.
Comment 14 Hugo Parente Lima 2012-07-13 11:20:08 PDT
Comment on attachment 152066 [details]
Patch

Yep, so I'll try to somehow get a gesture tap on WTR then re-submit the patch.
Comment 15 Kevin Ellis 2012-07-18 08:11:36 PDT
With patch r122970, the touch adjustment is confined to the touch area, which should help address the issue in comment #5.
Comment 16 Hugo Parente Lima 2012-07-31 11:56:49 PDT
(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.
Comment 17 Hugo Parente Lima 2012-08-01 12:36:13 PDT
Created attachment 155862 [details]
Patch
Comment 18 Hugo Parente Lima 2012-08-01 12:37:25 PDT
Comment on attachment 155862 [details]
Patch

This can't be landed until bug 92895 get fixed.
Comment 19 Allan Sandfeld Jensen 2012-08-01 13:07:30 PDT
(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.
Comment 20 Hugo Parente Lima 2012-08-01 13:35:02 PDT
(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.
Comment 21 Hugo Parente Lima 2012-08-01 14:14:18 PDT
Created attachment 155882 [details]
Patch

Oops, now with layouttests changelog.
Comment 22 WebKit Review Bot 2012-08-01 15:10:22 PDT
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
Comment 23 WebKit Review Bot 2012-08-01 15:10:28 PDT
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
Comment 24 Hugo Parente Lima 2012-08-02 14:23:53 PDT
Created attachment 156165 [details]
Patch

Test fixed + typos, we can't rely on focusout event because it's always fired when WTR exists.
Comment 25 Allan Sandfeld Jensen 2012-08-02 15:03:20 PDT
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?
Comment 26 WebKit Review Bot 2012-08-02 15:56:42 PDT
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
Comment 27 WebKit Review Bot 2012-08-02 15:56:47 PDT
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
Comment 28 Hugo Parente Lima 2012-08-03 06:38:47 PDT
(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.
Comment 29 Hugo Parente Lima 2012-08-03 06:51:00 PDT
Created attachment 156359 [details]
Patch
Comment 30 Hugo Parente Lima 2012-08-03 07:13:29 PDT
Comment on attachment 156359 [details]
Patch

cq- until patch for bug 92895 get landed.
Comment 31 WebKit Review Bot 2012-08-03 07:51:25 PDT
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
Comment 32 WebKit Review Bot 2012-08-03 07:51:30 PDT
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
Comment 33 WebKit Review Bot 2012-08-03 10:56:35 PDT
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
Comment 34 WebKit Review Bot 2012-08-03 10:56:40 PDT
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
Comment 35 Terry Anderson 2012-08-03 12:52:09 PDT
Just a heads up that https://bugs.webkit.org/show_bug.cgi?id=92914 landed.
Comment 36 Allan Sandfeld Jensen 2012-08-06 05:10:57 PDT
(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.
Comment 37 Hugo Parente Lima 2012-08-27 13:14:04 PDT
Created attachment 160789 [details]
Patch

Patch rebased, delay due to my vacations =]
Comment 38 WebKit Review Bot 2012-08-28 07:10:56 PDT
Comment on attachment 160789 [details]
Patch

Clearing flags on attachment: 160789

Committed r126873: <http://trac.webkit.org/changeset/126873>
Comment 39 WebKit Review Bot 2012-08-28 07:11:02 PDT
All reviewed patches have been landed.  Closing bug.