Bug 162367 – Add long press selection test

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Add long press selection test

[Megan Gardner]

Created attachment 289495 [details]
Patch

[Tim Horton]

Comment on attachment 289495 [details]
Patch

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

> Tools/ChangeLog:31
> +        * DumpRenderTree/ios/UIScriptControllerIOS.mm:
> +        (WTR::UIScriptController::longPressAtPoint):
> +        (WTR::UIScriptController::forcePressAtPoint):
> +        (WTR::UIScriptController::dragFromPointToPoint): Deleted.
> +        * Scripts/webkitpy/common/config/contributors.json:
> +        * TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
> +        * TestRunnerShared/UIScriptContext/UIScriptController.cpp:
> +        (WTR::UIScriptController::longPressAtPoint):
> +        (WTR::UIScriptController::forcePressAtPoint):
> +        (WTR::UIScriptController::dragFromPointToPoint): Deleted.
> +        * TestRunnerShared/UIScriptContext/UIScriptController.h:
> +        * WebKitTestRunner/ios/HIDEventGenerator.h:
> +        * WebKitTestRunner/ios/HIDEventGenerator.mm:
> +        (-[HIDEventGenerator _createIOHIDEventType:]):
> +        (-[HIDEventGenerator sendTaps:location:withNumberOfTouches:completionBlock:]):
> +        (-[HIDEventGenerator clearTap:]):
> +        (-[HIDEventGenerator longPressTimerCall:]):
> +        (-[HIDEventGenerator longPressFinish:completionBlock:]):
> +        (-[HIDEventGenerator longPress:completionBlock:]):
> +        (-[HIDEventGenerator forcePress:completionBlock:]):
> +        * WebKitTestRunner/ios/UIScriptControllerIOS.mm:
> +        (WTR::UIScriptController::longPressAtPoint):
> +        (WTR::UIScriptController::forcePressAtPoint):
> +        (WTR::UIScriptController::dragFromPointToPoint): Deleted.

This region is for explaining why/what you did in each of these places, making it easier to review and making it possible for me to ask less questions below!

> Tools/Scripts/webkitpy/common/config/contributors.json:3500
> +	  "Megan Gardner" : {

This should happen in a different patch.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:40
> -static const NSTimeInterval fingerLiftDelay = 5e7;
> -static const NSTimeInterval multiTapInterval = 15e7;
> +static const long fingerLiftDelay = 5e7;
> +static const long multiTapInterval = 15e7;

Why?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:548
> +    [self longPressFinish:[[userInfo valueForKey:@"location"] CGPointValue] completionBlock:[userInfo objectForKey:@"block"]];

I think we can userInfo[@"location"] and such.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:549
> +    [timer invalidate];

No need to invalidate a non-repeating timer.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:558
> +    [self clearTap:CGPointMake(50, 50)];

Why the random hardcoded 50, 50?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:569
> +    NSTimer *timer = [[NSTimer timerWithTimeInterval:longPressHoldDelay target:self selector:@selector(longPressTimerCall:) userInfo:info repeats:NO] retain];

Probably better to use scheduledTimerWith... and not have to explicitly add it, no? Also, I think the runloop will retain it in that case, so you don't have to (and don't have to release it when it fires).

> LayoutTests/fast/events/touch/ios/long-press-to-select-text.html:33
> +					var selection = document.getSelection().toString();

Spaces, not tabs, please.

[Simon Fraser (smfr)]

Comment on attachment 289495 [details]
Patch

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

> Tools/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

There should be a summary of your changes here. Describe what features you are adding to the test harness, and how they work.

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:42
> +    void forcePressAtPoint(long x, long y, object callback);

I think this will want some extra parameters to describe the force values. I think it should also be done in a separate patch.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:163
> +    } else if (eventType == HandEventPress) {
> +        eventMask &= ~kIOHIDDigitizerEventTouch;
> +        eventMask |= kIOHIDDigitizerEventAttribute;
>      } else if (eventType == HandEventChordChanged) {
>          eventMask |= kIOHIDDigitizerEventPosition;
>          eventMask |= kIOHIDDigitizerEventAttribute;
> -    } else if (eventType == HandEventTouched  || eventType == HandEventCanceled) {
> -        eventMask |= kIOHIDDigitizerEventIdentity;
> -    } else if (eventType == HandEventLifted)
> +    } else if (eventType == HandEventTouched)

I can't tell if these are necessary for force press, or long press, which is a good reason to do those in two separate patches.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:509
> +    struct timespec doubleDelay = { 0, multiTapInterval };
> +    struct timespec pressDelay = { 0, fingerLiftDelay };

Would prefer it the old way.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:528
> +- (void)clearTap:(CGPoint)location
> +{
> +    [self touchDown:location touchCount:1];
> +    struct timespec pressDelay = { 0, fingerLiftDelay };
> +    nanosleep(&pressDelay, 0);
> +    [self liftUp:location touchCount:1];
> +}

This is weird. Why is a "clear tap" sending another tap?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:545
> +- (void)longPressTimerCall:(NSTimer *)timer

Weird function name.

>> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:558
>> +    [self clearTap:CGPointMake(50, 50)];
> 
> Why the random hardcoded 50, 50?

Also I think it should be the job of the test, or the harness to remove the callout bar. This code is too low-level, and is just involved in synthesizing touches.

> LayoutTests/fast/events/touch/ios/long-press-to-select-text.html:37
> +						output += "No Selection";

It's best if the test result is self-describing. For example:
"FAIL: failed to select a word as a result of a long press"
"PASS: successfully selected the word " + selection

> LayoutTests/fast/events/touch/ios/long-press-to-select-text.html:59
> +	PressMe<br>

This is kinda gross HTML. Use a <p>?

[Megan Gardner]

Created attachment 289594 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 289594 [details]
Patch

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

> Tools/ChangeLog:8
> +        Added support of Long Press tests.

I think this needs a little more explanation. I suggestion something like:

Add support to UIScriptController to synthesize long press events on iOS. This required adding long-press functionality to HIDEventGenerator.

HIDEventGenerator sends the touchDown, but must then send the touchUp on a timer (rather than sleeping, as we do for other events) in order for the gesture recognizers to correctly detect a long press.

Use the long press synthesis in a test that detects whether a long press gesture triggers text selection.

> Tools/ChangeLog:9
> +        Fixed oddly typed constants.

They aren't oddly typed; NSTimeInterval is the correct way to store durations. What's odd is that they get shoved into a timespec. I'd leave this as-is.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.h:49
> +// Special Presses

Let's say "Long press and force press"

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:40
> -static const NSTimeInterval fingerLiftDelay = 5e7;
> -static const NSTimeInterval multiTapInterval = 15e7;
> +static const long fingerLiftDelay = 5e7;
> +static const long multiTapInterval = 15e7;

Revert this part.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:-55
> -    HandEventCanceled,

I think we should keep Canceled, because we should test event cancellation at some point.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:-191
> -        } else if (eventType == HandEventLifted || eventType == HandEventCanceled || eventType == StylusEventLifted) {

Keep canceled.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:505
> -    struct timespec doubleDelay = { 0, static_cast<long>(multiTapInterval) };
> -    struct timespec pressDelay = { 0, static_cast<long>(fingerLiftDelay) };
> +    struct timespec doubleDelay = { 0, multiTapInterval };
> +    struct timespec pressDelay = { 0, fingerLiftDelay };

Revert.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:524
> +- (void)clearTap:(CGPoint)location
> +{
> +    [self touchDown:location touchCount:1];
> +    struct timespec pressDelay = { 0, fingerLiftDelay };
> +    nanosleep(&pressDelay, 0);
> +    [self liftUp:location touchCount:1];
> +}

I still don't think this belongs here.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:556
> +
> +    [self touchDown:location touchCount:1];

Weird blank line.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:561
> +    [NSTimer scheduledTimerWithTimeInterval:longPressHoldDelay target:self selector:@selector(longPressTimerFired:) userInfo:info repeats:NO];

If you used the block form. you wouldn't have to build an info dictionary or have a separate function for the callback.

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:562
> +    [completionBlockCopy release];

Use Block_release()

[Tim Horton]

(In reply to comment #5)
> Comment on attachment 289594 [details]
> Patch
> > Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:561
> > +    [NSTimer scheduledTimerWithTimeInterval:longPressHoldDelay target:self selector:@selector(longPressTimerFired:) userInfo:info repeats:NO];
> 
> If you used the block form. you wouldn't have to build an info dictionary or
> have a separate function for the callback.

You can't do that, it wasn't available until recently.

[Megan Gardner]

Created attachment 289609 [details]
Patch

[Tim Horton]

Comment on attachment 289609 [details]
Patch

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

> Tools/ChangeLog:14
> +        Fixed incorrectly typed constats. NSTimeInterval is in seconds, origial numbers were nanoseconds and typedefed to long without reguard to the type differences.

Lots of typos in this line (constants, reguard), but I think it's also just not factual anymore, right?

> Tools/WebKitTestRunner/ios/HIDEventGenerator.mm:549
> +    void (^completionBlockCopy)() = Block_copy(completionBlock);

Wonder if this should be using BlockPtr :D

> LayoutTests/fast/events/touch/ios/long-press-to-select-text.html:59
> +	PressMe<p>

I think smfr probably meant

<p>PressMe</p>

[Megan Gardner]

Created attachment 289614 [details]
Patch

[Tim Horton]

Comment on attachment 289614 [details]
Patch

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

> LayoutTests/fast/events/touch/ios/long-press-to-select-text.html:34
> +                    if (selection!=="")

Spaces around operators.

[Simon Fraser (smfr)]

Comment on attachment 289614 [details]
Patch

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

> Tools/ChangeLog:14
> +        Add support to UIScriptController to synthesize long press events on iOS. This required adding long-press functionality to HIDEventGenerator.
> +
> +        HIDEventGenerator sends the touchDown, but must then send the touchUp with a dispatch_after (rather than sleeping, as we do for other events) in order for the gesture recognizers to correctly detect a long press.
> +
> +        Use the long press synthesis in a test that detects whether a long press gesture triggers text selection.
> +
> +        Fixed incorrect constants. NSTimeInterval is in seconds, original numbers were nanoseconds and typedefed to long without regard to the type differences. Redid constants to be the right value, and converted upon use.

You should hard wrap these lines to 80 chars or so. Look at the other entries for examples.

> LayoutTests/fast/events/touch/ios/long-press-to-select-text.html:33
> +                    var selection = document.getSelection().toString();

Variable would be better named as "selectionText" or "seletionAsString"

[Megan Gardner]

Created attachment 289627 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 289627 [details]
Patch

Clearing flags on attachment: 289627

Committed r206282: <http://trac.webkit.org/changeset/206282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.