| Summary: | [WK2] Send click events to WebCore when the user clicked on a non-special node with TOUCH_ADJUSTMENT enabled. | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hugo Parente Lima <hugo.lima> | ||||||||||||||||||||||||
| Component: | WebKit2 | Assignee: | Hugo Parente Lima <hugo.lima> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | allan.jensen, dglazkov, kenneth, kevers, luciano.wolf, menard, tdanderson, tonikitoo, webkit.review.bot | ||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| Bug Depends on: | 92895, 95205 | ||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
Created attachment 152066 [details]
Patch
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. 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. 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? (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. You can reproduce the issue of the viewport with the attached page, running MiniBrowser with --window-size 400x800 Comment on attachment 152066 [details]
Patch
This needs a best
(In reply to comment #7) > (From update of attachment 152066 [details]) > This needs a best a test* even :-) (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. 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?
(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? (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. (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 on attachment 152066 [details]
Patch
Yep, so I'll try to somehow get a gesture tap on WTR then re-submit the patch.
With patch r122970, the touch adjustment is confined to the touch area, which should help address the issue in comment #5. (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. Created attachment 155862 [details]
Patch
Comment on attachment 155862 [details] Patch This can't be landed until bug 92895 get fixed. (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. (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. Created attachment 155882 [details]
Patch
Oops, now with layouttests changelog.
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 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
Created attachment 156165 [details]
Patch
Test fixed + typos, we can't rely on focusout event because it's always fired when WTR exists.
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 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 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
(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. Created attachment 156359 [details]
Patch
Comment on attachment 156359 [details] Patch cq- until patch for bug 92895 get landed. 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 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 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 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
Just a heads up that https://bugs.webkit.org/show_bug.cgi?id=92914 landed. (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. Created attachment 160789 [details]
Patch
Patch rebased, delay due to my vacations =]
Comment on attachment 160789 [details] Patch Clearing flags on attachment: 160789 Committed r126873: <http://trac.webkit.org/changeset/126873> All reviewed patches have been landed. Closing bug. |
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.