Bug 195281

Summary: [iOS] Should not scroll when checkbox, radio, submit, reset, or button is spacebar activated
Product: WebKit Reporter: Daniel Bates <dbates>
Component: ScrollingAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar, PlatformOnly
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 12   
Attachments:
Description Flags
Patch and tests
none
Patch and tests
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
To land
none
To land none

Description Daniel Bates 2019-03-04 09:31:15 PST
With recent UIKit work and the patch for bug #192824 checkboxes and radio buttons can now be activated by pressing the spacebar when focused. The spacebar is also interpreted by the iOS scroll animator as a request to scroll the page. So, we may scroll the page when trying to activate a checkbox or radio button. That does not feel natural.

We have heuristics to not scroll when the spacebar is pressed if the currently focused element is a content editable element or a <select>. Clearly this does not cover checkboxes and radio buttons. So, we should amend the heuristic. Also, we should consider amending the heuristic to avoid scrolling when other form elements are focused such as a submit, reset or button element.

Note that content edibility covers text fields (all the different kinds) and text areas.
Comment 1 Radar WebKit Bug Importer 2019-03-04 09:31:56 PST
<rdar://problem/48564347>
Comment 2 Daniel Bates 2019-03-04 16:44:15 PST
Created attachment 363568 [details]
Patch and tests
Comment 3 Daniel Bates 2019-03-04 17:11:07 PST
Created attachment 363572 [details]
Patch and tests
Comment 4 EWS Watchlist 2019-03-04 19:03:34 PST
Comment on attachment 363572 [details]
Patch and tests

Attachment 363572 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11370023

New failing tests:
fast/events/beforeunload-alert-user-interaction2.html
fast/forms/datalist/datalist-textinput-suggestions-order.html
fast/images/imagemap-polygon-focus-ring.html
fast/scrolling/ios/hit-testing-iframe-005.html
editing/selection/thai-word-at-document-end.html
fast/events/click-handler-on-body-simple.html
Comment 5 EWS Watchlist 2019-03-04 19:03:35 PST Comment hidden (obsolete)
Comment 6 Daniel Bates 2019-03-05 09:19:11 PST
(In reply to Build Bot from comment #4)
> Comment on attachment 363572 [details]
> Patch and tests
> 
> Attachment 363572 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/11370023
> 
> New failing tests:
> fast/events/beforeunload-alert-user-interaction2.html
> fast/forms/datalist/datalist-textinput-suggestions-order.html
> fast/images/imagemap-polygon-focus-ring.html
> fast/scrolling/ios/hit-testing-iframe-005.html
> editing/selection/thai-word-at-document-end.html
> fast/events/click-handler-on-body-simple.html

Just eyeballing the EWS results and these tests failed on trunk without my patch. Confirmed locally that all of these tests pass with my patch.
Comment 7 Wenson Hsieh 2019-03-05 09:25:20 PST
(In reply to Daniel Bates from comment #6)
> (In reply to Build Bot from comment #4)
> > Comment on attachment 363572 [details]
> > Patch and tests
> > 
> > Attachment 363572 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > Output: https://webkit-queues.webkit.org/results/11370023
> > 
> > New failing tests:
> > fast/events/beforeunload-alert-user-interaction2.html
> > fast/forms/datalist/datalist-textinput-suggestions-order.html
> > fast/images/imagemap-polygon-focus-ring.html
> > fast/scrolling/ios/hit-testing-iframe-005.html
> > editing/selection/thai-word-at-document-end.html
> > fast/events/click-handler-on-body-simple.html
> 
> Just eyeballing the EWS results and these tests failed on trunk without my
> patch. Confirmed locally that all of these tests pass with my patch.

Looks like this was fallout from r242403 (since reverted in r242467).
Comment 8 Simon Fraser (smfr) 2019-03-05 14:29:01 PST
Comment on attachment 363572 [details]
Patch and tests

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

> Source/WebCore/ChangeLog:17
> +        WebCore implements spacebar activation on keydown for form controls. For IE compatibility
> +        WebCore does not mark such keydown events as handled so that a DOM keypress event will
> +        be subsequently dispatched. The current logic only skips calling the base class's default

Do we still care about IE combat? What do other browsers do? Maybe HTML5 specifies this now?

> Source/WebCore/ChangeLog:21
> +        event is not marked as handled, but WebCore actaully accounted for this event then we need

actaully

> Source/WebCore/html/BaseCheckableInputType.cpp:67
> -void BaseCheckableInputType::handleKeydownEvent(KeyboardEvent& event)
> +auto BaseCheckableInputType::handleKeydownEvent(KeyboardEvent& event) -> ShouldCallBaseEventHandler

I kinda feel like a bool return value would be as easily understood.

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:120
>      BaseClickableWithKeyInputType::handleKeydownEvent(*element(), event);
> +    return ShouldCallBaseEventHandler::Yes;

Why not return BaseClickableWithKeyInputType::handleKeydownEvent() ?
Comment 9 Daniel Bates 2019-03-05 15:03:35 PST
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 363572 [details]
> Patch and tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363572&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        WebCore implements spacebar activation on keydown for form controls. For IE compatibility
> > +        WebCore does not mark such keydown events as handled so that a DOM keypress event will
> > +        be subsequently dispatched. The current logic only skips calling the base class's default
> 
> Do we still care about IE combat? 

I don't know.

> What do other browsers do? 

Chrome and Firefox also dispatch keypress.

> Maybe HTML5
> specifies this now?
> 

Nope. Filed <https://github.com/w3c/uievents/issues/225> to fix up UI Events spec.

> > Source/WebCore/ChangeLog:21
> > +        event is not marked as handled, but WebCore actaully accounted for this event then we need
> 
> actaully
> 

Will fix,

> > Source/WebCore/html/BaseCheckableInputType.cpp:67
> > -void BaseCheckableInputType::handleKeydownEvent(KeyboardEvent& event)
> > +auto BaseCheckableInputType::handleKeydownEvent(KeyboardEvent& event) -> ShouldCallBaseEventHandler
> 
> I kinda feel like a bool return value would be as easily understood.
> 
> > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:120
> >      BaseClickableWithKeyInputType::handleKeydownEvent(*element(), event);
> > +    return ShouldCallBaseEventHandler::Yes;
> 
> Why not return BaseClickableWithKeyInputType::handleKeydownEvent() ?

I could, but I need to teach that overload about ShouldCallBaseEventHandler and it would always return YES. I guess I could :/
Comment 10 Daniel Bates 2019-03-05 15:34:46 PST
Comment on attachment 363572 [details]
Patch and tests

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

>> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:120
>> +    return ShouldCallBaseEventHandler::Yes;
> 
> Why not return BaseClickableWithKeyInputType::handleKeydownEvent() ?

Will fix.
Comment 11 Daniel Bates 2019-03-05 15:36:04 PST
Created attachment 363698 [details]
To land
Comment 12 Daniel Bates 2019-03-05 15:38:37 PST
Created attachment 363701 [details]
To land
Comment 13 Daniel Bates 2019-03-05 15:40:16 PST
Committed r242518: <https://trac.webkit.org/changeset/242518>