Bug 151227 – Hardware keyboard spacebar scrolls too far on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Hardware keyboard spacebar scrolls too far on iOS

[Tim Horton]

Created attachment 265445 [details]
Patch

[Simon Fraser (smfr)]

This should be testable with the new stuff. Please make a test.

[Beth Dakin]

(In reply to comment #2)
> This should be testable with the new stuff. Please make a test.

Yup, Tim's working on one.

[Tim Horton]

Created attachment 265457 [details]
Patch

[Simon Fraser (smfr)]

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.

[David Kilzer (:ddkilzer)]

<rdar://problem/23500681>

[Tim Horton]

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.

[Tim Horton]

http://trac.webkit.org/changeset/192432

[Tim Horton]

(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