Bug 151227

Summary: Hardware keyboard spacebar scrolls too far on iOS
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, ddkilzer, enrica, mitz, sam, simon.fraser
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+

Description Tim Horton 2015-11-12 15:54:51 PST
Hardware keyboard spacebar scrolls too far on iOS
Comment 1 Tim Horton 2015-11-12 15:56:02 PST
Created attachment 265445 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-11-12 16:27:38 PST
This should be testable with the new stuff. Please make a test.
Comment 3 Beth Dakin 2015-11-12 16:28:33 PST
(In reply to comment #2)
> This should be testable with the new stuff. Please make a test.

Yup, Tim's working on one.
Comment 4 Tim Horton 2015-11-12 17:25:06 PST
Created attachment 265457 [details]
Patch
Comment 5 Simon Fraser (smfr) 2015-11-12 17:45:47 PST
Comment on attachment 265457 [details]
Patch

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

> Source/WebKit2/ChangeLog:43
> +        If we're in the middle of resending an event we didn't handle the first
> +        time, just ignore it.

Why does this happen? Would be nice to have an explanation in a comment.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:89
> +    [_uiEvent release];

Doesn't RetainPtr take care of this for you?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2808
> +    else

Would prefer a return instead of an else.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2815
> +    if (event == _uiEventBeingResent)
> +        return;

Rather confusing.

> LayoutTests/fast/events/ios/keyboard-scrolling-distance.html:10
> +            // Need to tap to focus so that key events go through (seems wrong!).

Please file a bug on this.
Comment 6 David Kilzer (:ddkilzer) 2015-11-12 19:29:21 PST
<rdar://problem/23500681>
Comment 7 Tim Horton 2015-11-12 19:45:00 PST
Comment on attachment 265457 [details]
Patch

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

>> Source/WebKit2/ChangeLog:43
>> +        time, just ignore it.
> 
> Why does this happen? Would be nice to have an explanation in a comment.

See below.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:89
>> +    [_uiEvent release];
> 
> Doesn't RetainPtr take care of this for you?

It's just a property, no RetainPtr. We could, but that would be wordier.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2808
>> +    else
> 
> Would prefer a return instead of an else.

Seems reasonable.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:2815
>> +        return;
> 
> Rather confusing.

Which part? We want to ignore it if we've already had a crack at the event and didn't want to handle it, and instead let it bubble further up the chain. This is very similar to what happens on Mac. On Mac, there is a comment near the member:
    // We keep here the event when resending it to
    // the application to distinguish the case of a new event from one
    // that has been already sent to WebCore.
Do you want something like that here?
Or maybe like the one in performKeyEquivalent:
        // WebCore has already seen the event, no need for custom processing.

>> LayoutTests/fast/events/ios/keyboard-scrolling-distance.html:10
>> +            // Need to tap to focus so that key events go through (seems wrong!).
> 
> Please file a bug on this.

Will do.
Comment 8 Tim Horton 2015-11-13 10:12:29 PST
http://trac.webkit.org/changeset/192432
Comment 9 Tim Horton 2015-11-13 10:12:51 PST
(In reply to comment #7)
> >> LayoutTests/fast/events/ios/keyboard-scrolling-distance.html:10
> >> +            // Need to tap to focus so that key events go through (seems wrong!).
> > 
> > Please file a bug on this.
> 
> Will do.

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