Bug 148963 - Implement EventSenderProxy for WebKitTestRunner on iOS
Summary: Implement EventSenderProxy for WebKitTestRunner on iOS
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-08 09:47 PDT by Wenson Hsieh
Modified: 2015-12-09 18:02 PST (History)
11 users (show)

See Also:


Attachments
Patch (25.44 KB, patch)
2015-09-08 11:49 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (33.60 KB, patch)
2015-09-10 10:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (32.94 KB, patch)
2015-09-10 10:25 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (45.80 KB, patch)
2015-09-11 16:15 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (50.46 KB, patch)
2015-09-12 17:12 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (57.96 KB, patch)
2015-09-13 00:02 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (61.15 KB, patch)
2015-09-19 11:17 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (60.86 KB, patch)
2015-09-19 11:26 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2015-09-08 09:47:15 PDT
The methods in EventSenderProxyIOS.mm are currently unimplemented, preventing us from testing iOS using JavaScript via window.eventSender.
Comment 1 Wenson Hsieh 2015-09-08 09:48:26 PDT
Note: This will also require some (unobtrusive) changes to UIKit.
Comment 2 Wenson Hsieh 2015-09-08 11:49:20 PDT
Created attachment 260776 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2015-09-08 13:09:16 PDT
<rdar://problem/22615821>
Comment 4 Wenson Hsieh 2015-09-08 13:39:43 PDT
WebFrame::fromCoreFrame(*frameRespondingToClick)->firstLayerTreeTransactionIDAfterDidCommitLoad() when it is called in the body's onload handler. This currently causes tests that tap elements immediately after onload to fail.

I'm investigating why this is the case, and how we can fix it.
Comment 5 Wenson Hsieh 2015-09-08 13:42:17 PDT
(In reply to comment #4)
> WebFrame::fromCoreFrame(*frameRespondingToClick)-
> >firstLayerTreeTransactionIDAfterDidCommitLoad() when it is called in the
> body's onload handler. This currently causes tests that tap elements
> immediately after onload to fail.
> 
> I'm investigating why this is the case, and how we can fix it.

(the comment on the top was cut off. Here's what I meant to write):

The current issue is that WebPage::commitPotentialTap hits an early return due to lastLayerTreeTransactionId < WebFrame::fromCoreFrame(*frameRespondingToClick)->firstLayerTreeTransactionIDAfterDidCommitLoad() when it is called in the body's onload handler. This currently causes tests that tap elements immediately after onload to fail.

I'm investigating why this is the case, and how we can fix it.
Comment 6 Alexey Proskuryakov 2015-09-09 09:12:50 PDT
Comment on attachment 260776 [details]
Patch

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

> LayoutTests/fast/forms/disabled-search-input.html:46
> -    }, 0);
> +    }, 500);

This is not right. Nothing in regression tests is guaranteed to happen within 500 ms - there are many processes competing for the CPU, and with hyperthreading, some of the virtual cores can be very slow. Furthermore, CPU load increases even further when there is a crash, or even media playing.

> LayoutTests/platform/ios-simulator/TestExpectations:1767
> -fast/forms/disabled-search-input.html [ Failure ]
> +fast/forms/disabled-search-input.html [ Pass ]

We now need separate results per iOS version, as we can't get this pass without UIKit changes.
Comment 7 Wenson Hsieh 2015-09-09 09:17:52 PDT
Comment on attachment 260776 [details]
Patch

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

>> LayoutTests/fast/forms/disabled-search-input.html:46
>> +    }, 500);
> 
> This is not right. Nothing in regression tests is guaranteed to happen within 500 ms - there are many processes competing for the CPU, and with hyperthreading, some of the virtual cores can be very slow. Furthermore, CPU load increases even further when there is a crash, or even media playing.

Sorry about that -- I don't intend on landing with this timeout here. There's one last major issue I've been investigating regarding WKTR, which is that sending a tap immediately on body load by calling eventSender.mouseDown, eventSender.mouseUp causes us to hit an early return and ignore the tap. I added the timeout here for now just to circumvent this and fix the rest of the issues with eventSender. When I figure out how to best address this, the timeout will not be necessary.
Comment 8 Wenson Hsieh 2015-09-10 10:22:22 PDT
Created attachment 260937 [details]
Patch
Comment 9 Wenson Hsieh 2015-09-10 10:25:25 PDT
Created attachment 260938 [details]
Patch
Comment 10 Wenson Hsieh 2015-09-10 10:29:14 PDT
I addressed the early return in commitPotentialTap by forcing a paint and spinning until the paint finishes the first time we try to interact with the page.
Comment 11 Wenson Hsieh 2015-09-10 15:41:17 PDT
It seems the repainting strategy only works when the test uses eventSender in a setTimeout after the body loads. Investigating further into this...
Comment 12 Wenson Hsieh 2015-09-11 16:15:54 PDT
Created attachment 261024 [details]
Patch
Comment 13 Wenson Hsieh 2015-09-11 16:31:06 PDT
The latest patches addresses the above problem. As long as eventSender is used after the body loads (which it should be for these tests), no more flakiness is observed. To do this, I block the web process from immediately synchronously messaging the UI process in EventSendingController until the web process has received a 'DidUpdate' message from the UI process' drawing area proxy indicating that it is safe to force a compositing layer flush right away. This allows us to start sending touch events in the onload handler of the body element rather than calling eventSender through a setTimeout.

Other outstanding issues:
- EventSender is currently unable to type more than a couple characters before text input starts becoming flaky.
- The testing harness occasionally crashes in Springboard while running the tests, but I'm not currently sure if that's due to my patch or if it's a preexisting issue.
- Still need to find a way to correctly map document coords to screen coords so that we can send touch events when the document is zoomed.
- Clean up the key window context ID hack in EventSenderProxy::sendIOHIDEvent which I'm using to make touch events hit-test successfully in UIKit. Namely, figure out why the application's key window appears to be nil when I try to access it.
Comment 14 Wenson Hsieh 2015-09-12 17:12:01 PDT
Created attachment 261069 [details]
Patch
Comment 15 Wenson Hsieh 2015-09-13 00:02:51 PDT
Created attachment 261079 [details]
Patch
Comment 16 Darin Adler 2015-09-16 10:28:40 PDT
Comment on attachment 261079 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.h:1082
> +    void sendForceRepaintWithCallback(RefPtr<VoidCallback>);

Argument types should almost never be RefPtr. Please consider a plain reference or pointer or RefPtr&& or Ref&&. Details at <http://www.webkit.org/coding/RefPtr.html>.
Comment 17 Wenson Hsieh 2015-09-16 11:44:24 PDT
(In reply to comment #16)
> Comment on attachment 261079 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261079&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:1082
> > +    void sendForceRepaintWithCallback(RefPtr<VoidCallback>);
> 
> Argument types should almost never be RefPtr. Please consider a plain
> reference or pointer or RefPtr&& or Ref&&. Details at
> <http://www.webkit.org/coding/RefPtr.html>.

Got it -- I'll change this. Thanks for the link!
Comment 18 Wenson Hsieh 2015-09-19 11:17:17 PDT
Created attachment 261580 [details]
Patch
Comment 19 Wenson Hsieh 2015-09-19 11:26:42 PDT
Created attachment 261582 [details]
Patch
Comment 20 Wenson Hsieh 2015-09-19 11:30:19 PDT
This patch makes it unnecessary to expose the size of the HID event queue in UIKit by adding WK API functions that allow us to wait until WebKit receives touch and key events. This also solves the problem of key events flaking when sending more than a few at a time, which is a big plus.

After r189959, the weird key window context id hack I was using to make touch events hit-test properly is also no longer needed. Thanks Simon!

I'm in the process of landing the necessary UIKeyboardImpl changes in UIKit, which will need to happen before this patch can land.
Comment 21 Enrica Casucci 2015-09-24 14:21:44 PDT
I don't think it is necessary to expose the ability to show the keyboard on programmatic focus via the eventSendingController.
This should be a preference set at the level of the TestController like many other ones (see resetPreferencesToConsistentValues in TestController.cpp).
This was you don't need to modify the actual tests and simplifies your patch.
Comment 22 Wenson Hsieh 2015-09-25 09:12:57 PDT
Good point -- this should also make it not necessary to plumb the focus enabling message from the web process to UI process.

My only concern is that we may want to write platform iOS tests in the future where we test that the keyboard does not show when an input has autofocus or the focus() method is programmatically called. However, these tests should be much more rare than existing tests that expect the keyboard to come up during programmatic focus, so it probably makes sense to make the TestController allow programmatic focus to assist the keyboard by preference.
Comment 23 Wenson Hsieh 2015-10-20 22:58:55 PDT
Closing as invalid, since we've switched over to UI-side-script-driven testing instead of the existing synchronous model.
Comment 24 Csaba Osztrogonác 2015-11-17 02:37:34 PST
Comment on attachment 261582 [details]
Patch

Cleared review? from attachment 261582 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).