[Webkit-unassigned] [Bug 162367] Add long press selection test

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162367

Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #289495|review?                     |review-
              Flags|                            |

--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 289495
  --> https://bugs.webkit.org/attachment.cgi?id=289495
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>?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160922/8c7e63b8/attachment-0001.html>