RESOLVED FIXED 77105
Implement touch event emulation in the WebCore layer
https://bugs.webkit.org/show_bug.cgi?id=77105
Summary Implement touch event emulation in the WebCore layer
Alexander Pavlov (apavlov)
Reported 2012-01-26 09:04:36 PST
Web developers for the mobile want to be able to debug their applications using their desktop hardware. This necessitates the emulation of certain aspects of mobile devices not found in desktop computers. One example is the emulation of "touch" events using the mouse pointer. The proposed implementation is based on http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html. Specifically, http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html#mouse-events states that if preventDefault() has been called on a touch event, the associated mouse event(s) should not be dispatched.
Attachments
Patch (9.36 KB, patch)
2012-01-26 09:23 PST, Alexander Pavlov (apavlov)
no flags
Patch (20.86 KB, patch)
2012-01-27 03:03 PST, Alexander Pavlov (apavlov)
no flags
[PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) (20.58 KB, patch)
2012-01-31 02:14 PST, Alexander Pavlov (apavlov)
no flags
[PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) - rebased (20.60 KB, patch)
2012-01-31 02:30 PST, Alexander Pavlov (apavlov)
no flags
[PATCH] rniwa's comments addressed (20.13 KB, patch)
2012-02-01 02:54 PST, Alexander Pavlov (apavlov)
no flags
[PATCH] more comments addressed (19.57 KB, patch)
2012-02-01 04:24 PST, Alexander Pavlov (apavlov)
rniwa: review+
Alexander Pavlov (apavlov)
Comment 1 2012-01-26 09:23:51 PST
Alexey Proskuryakov
Comment 2 2012-01-26 09:32:07 PST
CC'ing some folks who will likely have comments.
Adam Barth
Comment 3 2012-01-26 14:18:26 PST
Comment on attachment 124128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124128&action=review > Source/WebCore/dom/Document.h:685 > +#if ENABLE(TOUCH_EVENTS) > + void setTouchEventEmulationEnabled(bool enabled) { m_touchEventEmulationEnabled = enabled; } > + bool isTouchEventEmulationEnabled() const { return m_touchEventEmulationEnabled; } > +#endif Should these be on WebCore::Settings rather than on Document itself?
Adam Barth
Comment 4 2012-01-26 14:18:56 PST
Comment on attachment 124128 [details] Patch Can we test this change? Perhaps using WebCore::Internals?
Ryosuke Niwa
Comment 5 2012-01-26 14:46:13 PST
Comment on attachment 124128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124128&action=review >> Source/WebCore/dom/Document.h:685 >> +#endif > > Should these be on WebCore::Settings rather than on Document itself? Yes.
Alexander Pavlov (apavlov)
Comment 6 2012-01-27 03:03:35 PST
Alexander Pavlov (apavlov)
Comment 7 2012-01-27 03:04:49 PST
Thanks for your comments, everyone. I've moved the property from Document into Settings and come up with a test using InternalSettings.
Ryosuke Niwa
Comment 8 2012-01-30 12:10:56 PST
Comment on attachment 124283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124283&action=review > Source/WebCore/page/EventHandler.cpp:146 > +#if ENABLE(TOUCH_EVENTS) && ENABLE(INSPECTOR) This condition doesn't seem right. Why is enabling inspector required for synthetic touch events?
Alexander Pavlov (apavlov)
Comment 9 2012-01-30 12:51:54 PST
(In reply to comment #8) > (From update of attachment 124283 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124283&action=review > > > Source/WebCore/page/EventHandler.cpp:146 > > +#if ENABLE(TOUCH_EVENTS) && ENABLE(INSPECTOR) > > This condition doesn't seem right. Why is enabling inspector required for synthetic touch events? Originally, this was not required, but then I noticed that Inspector was going to be the only client of this feature (at least for the time being) and added the second clause. If you think this will make perfect sense for non-inspector-enabled builds, I'll be happy to revert this to the original ENABLE(TOUCH_EVENTS) condition.
Ryosuke Niwa
Comment 10 2012-01-30 13:19:14 PST
(In reply to comment #9) > Originally, this was not required, but then I noticed that Inspector was going to be the only client of this feature (at least for the time being) and added the second clause. If you think this will make perfect sense for non-inspector-enabled builds, I'll be happy to revert this to the original ENABLE(TOUCH_EVENTS) condition. As long as symbols are not exported, linkers should strip this code away so I'd rather have it for all builds with TOUCH enabled and hard-coding INSPECTOR dependency. Alternatively, you can add SYNTHETIC_TOUCH_EVENTS flag but that seems like an overkill.
Alexander Pavlov (apavlov)
Comment 11 2012-01-30 22:09:33 PST
(In reply to comment #10) > (In reply to comment #9) > > Originally, this was not required, but then I noticed that Inspector was going to be the only client of this feature (at least for the time being) and added the second clause. If you think this will make perfect sense for non-inspector-enabled builds, I'll be happy to revert this to the original ENABLE(TOUCH_EVENTS) condition. > > As long as symbols are not exported, linkers should strip this code away so I'd rather have it for all builds with TOUCH enabled and hard-coding INSPECTOR dependency. Alternatively, you can add SYNTHETIC_TOUCH_EVENTS flag but that seems like an overkill. Fine with reverting to just ENABLE(TOUCH_EVENTS). And what do you mean by "hard-coding INSPECTOR dependency"?
Alexander Pavlov (apavlov)
Comment 12 2012-01-31 02:14:28 PST
Created attachment 124687 [details] [PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation)
WebKit Review Bot
Comment 13 2012-01-31 02:17:03 PST
Attachment 124687 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 9504f8b..3b4a9fa master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 106347 = 9504f8bc63da3ef4bc09c7a9fb94caaf217cb96a r106348 = 3b4a9fa133678770a77bc6d602361e3cd4cb2dd1 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Alexander Pavlov (apavlov)
Comment 14 2012-01-31 02:30:43 PST
Created attachment 124689 [details] [PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) - rebased
WebKit Review Bot
Comment 15 2012-01-31 02:32:12 PST
Attachment 124689 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Alexander Pavlov (apavlov)
Comment 16 2012-01-31 14:05:25 PST
@reviewers: reverted to using only ENABLE(TOUCH_EVENTS). Any other concerns about the patch?
Ryosuke Niwa
Comment 17 2012-01-31 15:30:13 PST
Comment on attachment 124689 [details] [PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) - rebased View in context: https://bugs.webkit.org/attachment.cgi?id=124689&action=review r- due to various nits but this patch looks much better than previous ones. > Source/WebCore/ChangeLog:7 > + Please provide an overview of your change. > Source/WebCore/page/EventHandler.cpp:159 > + m_radiusY = 1; // Default value. > + m_radiusX = 1; // Default value. > + m_rotationAngle = 0.0f; // Default value. > + m_force = 1.0f; // Default value. Instead of commenting that these are default values, please add const int with appropriate variable names e.g. radiusXDefaultValue. > Source/WebCore/page/EventHandler.cpp:175 > + case PlatformEvent::MouseScroll: > + default: Why do we need to enumerate PlatformEvent::MouseScroll if we have default? > Source/WebCore/page/EventHandler.cpp:199 > + switch (event.type()) { > + case PlatformEvent::MouseMoved: > + m_type = TouchMove; > + break; > + case PlatformEvent::MousePressed: > + m_type = TouchStart; > + break; > + case PlatformEvent::MouseReleased: > + m_type = TouchEnd; > + break; > + case PlatformEvent::MouseScroll: > + default: > + ASSERT_NOT_REACHED(); > + m_type = NoType; > + } This switch statement duplicates the code. Please share code with SyntheticTouchPoint::SyntheticTouchPoint. > Source/WebCore/page/EventHandler.cpp:3443 > +bool EventHandler::maybeDispatchSyntheticTouchEvent(const PlatformMouseEvent& event) "maybe" isn't descriptive adverb here. How about dispatchSyntheticTouchEventIfEnabled? > LayoutTests/fast/events/touch/emulate-touch-events.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use HTML5 style DOCTYPE: <!DOCTYPE html> > LayoutTests/fast/events/touch/emulate-touch-events.html:4 > +<head> > +<script src="../../js/resources/js-test-pre.js"></script> Why do we need head element at all? > LayoutTests/fast/events/touch/emulate-touch-events.html:9 > +<!-- > + Touch tests that involve the ontouchstart, ontouchmove, ontouchend or ontouchcancel callbacks > + should be written in an asynchronous fashion so they can be run on mobile platforms like Android. > + You will need to invoke isSuccessfullyParsed() in your test script when the test completes. > +--> Once you move the script here, you wouldn't need this description. If you think this comment is still useful, then please put in the description. > LayoutTests/fast/events/touch/emulate-touch-events.html:14 > +<script src="script-tests/emulate-touch-events.js"></script> New style is to include the script in the html file itself. > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:21 > + // If we've got here, we can safely say we were successfully parsed :) We need to > + // call the isSucccessfullyParsed function to output the correct TEST COMPLETE > + // footer message. This comment doesn't provide us of any new information. Please remove. > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22 > + isSuccessfullyParsed(); You should be declaring successfullyParsed variable at the end of the time instead. > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:53 > + switch (which) { > + case 0: Don't indent cases. > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:99 > + debug("This test requires DumpRenderTree. Tap on the blue rect to log."); Is there anyway to manually test this feature?
Alexander Pavlov (apavlov)
Comment 18 2012-02-01 02:48:11 PST
Attaching a fixed patch shortly. (In reply to comment #17) > (From update of attachment 124689 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124689&action=review > > r- due to various nits but this patch looks much better than previous ones. > > > Source/WebCore/ChangeLog:7 > > + > > Please provide an overview of your change. Done. > > Source/WebCore/page/EventHandler.cpp:159 > > + m_radiusY = 1; // Default value. > > + m_radiusX = 1; // Default value. > > + m_rotationAngle = 0.0f; // Default value. > > + m_force = 1.0f; // Default value. > > Instead of commenting that these are default values, please add const int with appropriate variable names e.g. radiusXDefaultValue. Done. > > Source/WebCore/page/EventHandler.cpp:175 > > + case PlatformEvent::MouseScroll: > > + default: > > Why do we need to enumerate PlatformEvent::MouseScroll if we have default? I felt like enumerating all mouse events in case we might want to make use of them for some kind of additional emulation (that is, we have some "spare" mouse event to use.) > > Source/WebCore/page/EventHandler.cpp:199 > > + switch (event.type()) { > > + case PlatformEvent::MouseMoved: > > + m_type = TouchMove; > > + break; > > + case PlatformEvent::MousePressed: > > + m_type = TouchStart; > > + break; > > + case PlatformEvent::MouseReleased: > > + m_type = TouchEnd; > > + break; > > + case PlatformEvent::MouseScroll: > > + default: > > + ASSERT_NOT_REACHED(); > > + m_type = NoType; > > + } > > This switch statement duplicates the code. Please share code with SyntheticTouchPoint::SyntheticTouchPoint. Not really. This switch chooses an unsigned value describing one of the three event types, the other one (in SyntheticTouchPoint ctor) chooses PlatformTouchPoint::State, which is quite different (describes a touch point state,) and in the general case the State-related mapping won't be as simple (e.g. it may have stationary touchpoints.) Consider this a V1 of the emulation protocol. > > Source/WebCore/page/EventHandler.cpp:3443 > > +bool EventHandler::maybeDispatchSyntheticTouchEvent(const PlatformMouseEvent& event) > > "maybe" isn't descriptive adverb here. How about dispatchSyntheticTouchEventIfEnabled? Good point, done. > > LayoutTests/fast/events/touch/emulate-touch-events.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Please use HTML5 style DOCTYPE: <!DOCTYPE html> I based my test off of basic-single-touch-events.html, guess it's a bit out-of-date. Fixed. > > LayoutTests/fast/events/touch/emulate-touch-events.html:4 > > +<head> > > +<script src="../../js/resources/js-test-pre.js"></script> > > Why do we need head element at all? Copied from basic-single-touch-events.html. Fixed. > > LayoutTests/fast/events/touch/emulate-touch-events.html:9 > > +<!-- > > + Touch tests that involve the ontouchstart, ontouchmove, ontouchend or ontouchcancel callbacks > > + should be written in an asynchronous fashion so they can be run on mobile platforms like Android. > > + You will need to invoke isSuccessfullyParsed() in your test script when the test completes. > > +--> > > Once you move the script here, you wouldn't need this description. If you think this comment is still useful, then please put in the description. Removed. > > LayoutTests/fast/events/touch/emulate-touch-events.html:14 > > +<script src="script-tests/emulate-touch-events.js"></script> > > New style is to include the script in the html file itself. Done. > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:21 > > + // If we've got here, we can safely say we were successfully parsed :) We need to > > + // call the isSucccessfullyParsed function to output the correct TEST COMPLETE > > + // footer message. > > This comment doesn't provide us of any new information. Please remove. Done. > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22 > > + isSuccessfullyParsed(); > > You should be declaring successfullyParsed variable at the end of the time instead. Removed this call, added debug output denoting the test completion. > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:53 > > + switch (which) { > > + case 0: > > Don't indent cases. Fixed. > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:99 > > + debug("This test requires DumpRenderTree. Tap on the blue rect to log."); > > Is there anyway to manually test this feature? This feature will be exposed to the users in the Web Inspector UI (a checkbox in the Settings) shortly after this core change has been landed (patch making use of this change in bug 77096). E.g. you will be able to go to http://paulirish.com/demo/multi, enable the emulation mode and draw in the canvas with your mouse just as you would draw there on your touch-enabled device (with a single touch point.) There are no other clients (barring this test) that toggle the emulation mode at the moment.
Alexander Pavlov (apavlov)
Comment 19 2012-02-01 02:54:34 PST
Created attachment 124919 [details] [PATCH] rniwa's comments addressed
Ryosuke Niwa
Comment 20 2012-02-01 02:59:16 PST
(In reply to comment #18) > > > Source/WebCore/page/EventHandler.cpp:175 > > > + case PlatformEvent::MouseScroll: > > > + default: > > > > Why do we need to enumerate PlatformEvent::MouseScroll if we have default? > > I felt like enumerating all mouse events in case we might want to make use of them for some kind of additional emulation (that is, we have some "spare" mouse event to use.) Please don't add unnecessary code on speculation. > > > Source/WebCore/page/EventHandler.cpp:199 ... > > This switch statement duplicates the code. Please share code with SyntheticTouchPoint::SyntheticTouchPoint. > > Not really. This switch chooses an unsigned value describing one of the three event types, the other one (in SyntheticTouchPoint ctor) chooses PlatformTouchPoint::State, which is quite different (describes a touch point state,) and in the general case the State-related mapping won't be as simple (e.g. it may have stationary touchpoints.) Consider this a V1 of the emulation protocol. Okay. > > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22 > > > + isSuccessfullyParsed(); > > > > You should be declaring successfullyParsed variable at the end of the time instead. > > Removed this call, added debug output denoting the test completion. All you need to do is: var successfullyParsed = true; at the end of script. > > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:99 > > > + debug("This test requires DumpRenderTree. Tap on the blue rect to log."); > > > > Is there anyway to manually test this feature? > > This feature will be exposed to the users in the Web Inspector UI (a checkbox in the Settings) shortly after this core change has been landed (patch making use of this change in bug 77096). E.g. you will be able to go to http://paulirish.com/demo/multi, enable the emulation mode and draw in the canvas with your mouse just as you would draw there on your touch-enabled device (with a single touch point.) There are no other clients (barring this test) that toggle the emulation mode at the moment. Okay.
Ryosuke Niwa
Comment 21 2012-02-01 03:06:14 PST
Comment on attachment 124919 [details] [PATCH] rniwa's comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=124919&action=review > Source/WebCore/page/EventHandler.cpp:181 > + ASSERT_NOT_REACHED(); I think you need a break after ASSERT_NOT_REACHED on some ports. > Source/WebCore/page/EventHandler.cpp:202 > + m_type = NoType; Ditto about break. > Source/WebCore/page/EventHandler.cpp:3446 > +// Returns whether the original mouse event should be handled. We don't normally add a comment like this; instead we follow the same convention. > Source/WebCore/page/EventHandler.cpp:3447 > +bool EventHandler::dispatchSyntheticTouchEventIfEnabled(const PlatformMouseEvent& event) We normally return defaultPrevented. > LayoutTests/fast/events/touch/emulate-touch-events.html:9 > + var div = document.createElement("div"); > + div.id = "touchtarget"; It seems unnecessary to indent the entire script content like this. > LayoutTests/fast/events/touch/emulate-touch-events.html:12 > + div.style.width = "100px"; > + div.style.height = "100px"; > + div.style.backgroundColor = "blue"; Why can't we just put this as a markup and obtain the div? i.e. <div id="touchtarget" style="width: 100px; height: 100px; background-color: blue;"></div> var touchTarget = document.getElementById('touchtarget'); > LayoutTests/fast/events/touch/emulate-touch-events.html:26 > + debug('<br /><span class="pass">TEST COMPLETE</span>'); I don't think you need this debug given that the time is going to timeout when notifyDone is never called. > LayoutTests/fast/events/touch/emulate-touch-events.html:105 > +</script> Do: var successfullyParsed = true; here.
Alexander Pavlov (apavlov)
Comment 22 2012-02-01 03:07:46 PST
(In reply to comment #20) > (In reply to comment #18) > > > > Source/WebCore/page/EventHandler.cpp:175 > > > > + case PlatformEvent::MouseScroll: > > > > + default: > > > > > > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22 > > > > + isSuccessfullyParsed(); > > > > > > You should be declaring successfullyParsed variable at the end of the time instead. > > > > Removed this call, added debug output denoting the test completion. > > All you need to do is: > var successfullyParsed = true; > at the end of script. My guess was that successfullyParsed denoted the correctness of all the JavaScript executed so far, right? (Because all the cases using successfullyParsed I have seen are synchronous, so parsing errors and execution errors appear at the same time. OTOH, placing "var a; a.foo();" in a touch event handler and placing "var successfullyParsed = true;" at the end of script would yield no errors, but the test would never work correctly.) Since our test is asynchronous, this makes no sense until the point when we notify testController about the test termination. Do you think "var successfullyParsed = true;" at the end of script is the right solution?
Ryosuke Niwa
Comment 23 2012-02-01 03:15:38 PST
(In reply to comment #22) > My guess was that successfullyParsed denoted the correctness of all the JavaScript executed so far, right? No, successfullyParsed is there to say that "the test script parsed without syntax errors". > Since our test is asynchronous, this makes no sense until the point when we notify testController about the test termination. That's not what successfullyParsed is used for.
Alexander Pavlov (apavlov)
Comment 24 2012-02-01 03:23:45 PST
(In reply to comment #23) > (In reply to comment #22) > > My guess was that successfullyParsed denoted the correctness of all the JavaScript executed so far, right? > > No, successfullyParsed is there to say that "the test script parsed without syntax errors". I got confused because in the suggested setup it was not used in any way. > > Since our test is asynchronous, this makes no sense until the point when we notify testController about the test termination. > > That's not what successfullyParsed is used for. successfullyParsed is used only in isSuccessfullyParsed() (called from finishJSTest()). That said, I guess I've figured it out. I need to call finishJSTest() and include js-test-post.js at the end of the file.
Alexander Pavlov (apavlov)
Comment 25 2012-02-01 04:18:06 PST
(In reply to comment #21) > (From update of attachment 124919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124919&action=review > > > Source/WebCore/page/EventHandler.cpp:181 > > + ASSERT_NOT_REACHED(); > > I think you need a break after ASSERT_NOT_REACHED on some ports. Fixed. > > Source/WebCore/page/EventHandler.cpp:202 > > + m_type = NoType; > > Ditto about break. Fixed. > > Source/WebCore/page/EventHandler.cpp:3446 > > +// Returns whether the original mouse event should be handled. > > We don't normally add a comment like this; instead we follow the same convention. Removed. > > Source/WebCore/page/EventHandler.cpp:3447 > > +bool EventHandler::dispatchSyntheticTouchEventIfEnabled(const PlatformMouseEvent& event) > > We normally return defaultPrevented. The point was that this does not correspond to defaultPrevented in the case when no touch event dispatching takes place, but looks like this can be included into the abstraction of "conditional event dispatching" :) > > LayoutTests/fast/events/touch/emulate-touch-events.html:9 > > + var div = document.createElement("div"); > > + div.id = "touchtarget"; > > It seems unnecessary to indent the entire script content like this. Fixed. > > LayoutTests/fast/events/touch/emulate-touch-events.html:12 > > + div.style.width = "100px"; > > + div.style.height = "100px"; > > + div.style.backgroundColor = "blue"; > > Why can't we just put this as a markup and obtain the div? > i.e. <div id="touchtarget" style="width: 100px; height: 100px; background-color: blue;"></div> > var touchTarget = document.getElementById('touchtarget'); Done. I'm not sure, but when fixing this, I had some issues with the mouse event handler invocations (things looked like the mouse events were not dispatched.) I solved this by random code motion. > > LayoutTests/fast/events/touch/emulate-touch-events.html:26 > > + debug('<br /><span class="pass">TEST COMPLETE</span>'); > > I don't think you need this debug given that the time is going to timeout when notifyDone is never called. Right, I ended up invoking finishJSTest() in this line. > > LayoutTests/fast/events/touch/emulate-touch-events.html:105 > > +</script> > > Do: > var successfullyParsed = true; > here. This is not necessary with finishJSTest() in place (it has some special handling for asynchronous tests.)
Alexander Pavlov (apavlov)
Comment 26 2012-02-01 04:24:07 PST
Created attachment 124928 [details] [PATCH] more comments addressed
Ryosuke Niwa
Comment 27 2012-02-02 22:26:05 PST
Comment on attachment 124928 [details] [PATCH] more comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=124928&action=review Seems okay. We can always improve it iteratively since it's not a Web-facing API. But please make sure adding new methods to internals.settings doesn't break any builds when you land it. > Source/WebCore/testing/InternalSettings.cpp:274 > +void InternalSettings::setTouchEventEmulationEnabled(bool enabled, ExceptionCode& ec) Don't you need to add symbols for this?
Alexander Pavlov (apavlov)
Comment 28 2012-02-03 02:11:13 PST
Zoltan Arvai
Comment 29 2012-02-03 04:24:56 PST
It seems that the newly added layout test in r106642 LayoutTests/fast/events/touch/emulate-touch-events.html not running as expected on Qt Linux Release bot: --- /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/emulate-touch-events-expected.txt +++ /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/emulate-touch-events-actual.txt @@ -52,8 +52,8 @@ PASS lastEvent.changedTouches[0].clientY is 30 PASS lastEvent.changedTouches[0].identifier is 0 PASS lastEvent.shiftKey is false -PASS lastEvent.altKey is true -PASS lastEvent.ctrlKey is true +FAIL lastEvent.altKey should be true. Was false. +FAIL lastEvent.ctrlKey should be true. Was false. PASS lastEvent.metaKey is false PASS successfullyParsed is true http://build.webkit.org/results/Qt%20Linux%20Release/r106642%20(42848)/results.html
Csaba Osztrogonác
Comment 30 2012-02-03 05:29:38 PST
Zoltán file a new bug report about it, and skipped this test on Qt until fix - http://trac.webkit.org/changeset/106652
Allan Sandfeld Jensen
Comment 31 2012-02-06 03:40:23 PST
Maybe I missed it, but was there any particular reason to do this in WebCore? For Qt the touch emulation is done on application level. It is really just a simply transformation of what events we send to WebKit.
Kenneth Rohde Christiansen
Comment 32 2012-02-06 03:41:27 PST
Why is this not done in the application layer, like we do for Qt? We support pinch emulation etc. Is this due to inspector integration or ?
Pavel Feldman
Comment 33 2012-02-06 03:58:53 PST
(In reply to comment #32) > Why is this not done in the application layer, like we do for Qt? We support pinch emulation etc. Is this due to inspector integration or ? "Emulate touch events" is a part of the inspector functionality, we want it to be available to all ports.
Kenneth Rohde Christiansen
Comment 34 2012-02-06 04:01:13 PST
(In reply to comment #33) > (In reply to comment #32) > > Why is this not done in the application layer, like we do for Qt? We support pinch emulation etc. Is this due to inspector integration or ? > > "Emulate touch events" is a part of the inspector functionality, we want it to be available to all ports. Ok, great :-) Did you think about adding support for emulating pinch zoom etc?
Note You need to log in before you can comment on or make changes to this bug.