Bug 195281 – [iOS] Should not scroll when checkbox, radio, submit, reset, or button is spacebar activated

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Daniel Bates]

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.

[Radar WebKit Bug Importer]

<rdar://problem/48564347>

[Daniel Bates]

Created attachment 363568 [details]
Patch and tests

[Daniel Bates]

Created attachment 363572 [details]
Patch and tests

[EWS Watchlist]

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

[EWS Watchlist]

Created attachment 363582 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Daniel Bates]

(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.

[Wenson Hsieh]

(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).

[Simon Fraser (smfr)]

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() ?

[Daniel Bates]

(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 :/

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 363698 [details]
To land

[Daniel Bates]

Created attachment 363701 [details]
To land

[Daniel Bates]

Committed r242518: <https://trac.webkit.org/changeset/242518>