[iOS] Allow programmatic focus when hardware keyboard is attached

• Component: WebKit2
• Status: RESOLVED FIXED

[Daniel Bates]

We should allow a web page to programmatically focus a field when a hardware keyboard is attached to an iOS device. Allowing programmatic focus without a hardware keyboard could be seen as a jarring experience on iOS because of the visual disruption it causes (we show the keyboard on screen, taking space, and scroll the view). When a hardware keyboard is attached on iOS the experience resembles a Mac as we only draw the caret. That is, we do not show the software keyboard or scroll to the focused field.

[Daniel Bates]

<rdar://problem/42270463>

[Daniel Bates]

Created attachment 350922 [details]
Work-in-progress

[EWS Watchlist]

Comment on attachment 350922 [details]
Work-in-progress

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

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

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

[Daniel Bates]

Created attachment 350975 [details]
Patch

[Wenson Hsieh]

Comment on attachment 350975 [details]
Patch

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

It seems a bit odd to create and rely on the state of a UIKeyboardImpl that lives in the web process. Is there any precedent for asking UIKit for information about the keyboard from within the web process?

An alternative is having the UI process notify the web process whenever hardware keyboard state changes.

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:226
> +    static bool isWebKitTestRunner = applicationBundleIsEqualTo("org.webkit.WebKitTestRunnerApp"_s); // e.g. org.webkit.WebKitTestRunnerApp0

Nit - comments should be sentence-like (i.e. capitalized first word, period at the end).

[Daniel Bates]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 350975 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350975&action=review
> 
> It seems a bit odd to create and rely on the state of a UIKeyboardImpl that
> lives in the web process.

Querying UIKeyboardImpl for the hardware keyboard mode ultimately queries the underlying OS for this information. I elaborate further about the choice to query UIKit directly in my response to your alternative approach remark. 

> Is there any precedent for asking UIKit for
> information about the keyboard from within the web process?
> 

There is not a precedent for querying for UIKeyboard explicitly. We have precedent for querying UIKit functionality directly from the WebProcess. One such example is Settings::defaultTextAutosizingEnabled(). We have existing precedent on Mac for querying the keyboard state. One instance is in PlatformKeyboardEvent::currentCapsLockState() (https://trac.webkit.org/browser/trunk/Source/WebCore/platform/mac/KeyEventMac.mm?rev=236599#L262).

> An alternative is having the UI process notify the web process whenever
> hardware keyboard state changes.
> 

I plan to do this work in a subsequent bug to in order to track caps lock state changes (when you change keyboards). Such work is not necessary to make the proposed enhancement.

> > Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:226
> > +    static bool isWebKitTestRunner = applicationBundleIsEqualTo("org.webkit.WebKitTestRunnerApp"_s); // e.g. org.webkit.WebKitTestRunnerApp0
> 
> Nit - comments should be sentence-like (i.e. capitalized first word, period
> at the end).

"e.g. org.webkit.WebKitTestRunnerApp0" is not meant to be a sentence and we have an exception for such end of line remarks. See the second sentence of <https://webkit.org/code-style-guidelines/#comments-sentences>.

[Wenson Hsieh]

(In reply to Daniel Bates from comment #7)
> (In reply to Wenson Hsieh from comment #6)
> > Comment on attachment 350975 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350975&action=review
> > 
> > It seems a bit odd to create and rely on the state of a UIKeyboardImpl that
> > lives in the web process.
> 
> Querying UIKeyboardImpl for the hardware keyboard mode ultimately queries
> the underlying OS for this information. I elaborate further about the choice
> to query UIKit directly in my response to your alternative approach remark. 
> 
> > Is there any precedent for asking UIKit for
> > information about the keyboard from within the web process?
> > 
> 
> There is not a precedent for querying for UIKeyboard explicitly. We have
> precedent for querying UIKit functionality directly from the WebProcess. One
> such example is Settings::defaultTextAutosizingEnabled(). We have existing
> precedent on Mac for querying the keyboard state. One instance is in
> PlatformKeyboardEvent::currentCapsLockState()
> (https://trac.webkit.org/browser/trunk/Source/WebCore/platform/mac/
> KeyEventMac.mm?rev=236599#L262).
> 
> > An alternative is having the UI process notify the web process whenever
> > hardware keyboard state changes.
> > 
> 
> I plan to do this work in a subsequent bug to in order to track caps lock
> state changes (when you change keyboards). Such work is not necessary to
> make the proposed enhancement.

👍 I think this will work for now, then.

> 
> > > Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:226
> > > +    static bool isWebKitTestRunner = applicationBundleIsEqualTo("org.webkit.WebKitTestRunnerApp"_s); // e.g. org.webkit.WebKitTestRunnerApp0
> > 
> > Nit - comments should be sentence-like (i.e. capitalized first word, period
> > at the end).
> 
> "e.g. org.webkit.WebKitTestRunnerApp0" is not meant to be a sentence and we
> have an exception for such end of line remarks. See the second sentence of
> <https://webkit.org/code-style-guidelines/#comments-sentences>.

[Daniel Bates]

Comment on attachment 350975 [details]
Patch

Clearing flags on attachment: 350975

Committed r236619: <https://trac.webkit.org/changeset/236619>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 350975 [details]
Patch

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

> Source/WebKit/Shared/ios/NativeWebKeyboardEventIOS.mm:39
> +    return !WebCore::IOSApplication::isDumpRenderTree() && !WebCore::IOSApplication::isWebKitTestRunner() && [UIKeyboard isInHardwareKeyboardMode];

Have you considered not introducing isWebKitTestRunner() and instead just having those two clients use the Objective-C runtime to dynamically override the behavior of +isInHardwareKeyboardMode? We can often avoid adding testing-only complexity to WebKit by using generic testing techniques like that (and we often do!).

[Darin Adler]

Comment on attachment 350975 [details]
Patch

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

>> Source/WebKit/Shared/ios/NativeWebKeyboardEventIOS.mm:39
>> +    return !WebCore::IOSApplication::isDumpRenderTree() && !WebCore::IOSApplication::isWebKitTestRunner() && [UIKeyboard isInHardwareKeyboardMode];
> 
> Have you considered not introducing isWebKitTestRunner() and instead just having those two clients use the Objective-C runtime to dynamically override the behavior of +isInHardwareKeyboardMode? We can often avoid adding testing-only complexity to WebKit by using generic testing techniques like that (and we often do!).

I strongly support this suggestion.

Besides Dan’s reasoning above, we also would like to avoid backward dependencies, where WebKit specifically knows about which testing tools exist and enumerates them, and instead have the testing tools make calls to change behavior. If we couldn’t use the Objective-C runtime to do this, the alternative solution I would propose would be some function for the test tools to call to change behavior, rather than code where WebKit itself checks if it’s running under a test tool.

Since it’s quite straightforward to override +[UIKeyboard isInHardwareKeyboardMode] to make it return NO, we can simply put calls to [UIKeyboard isInHardwareKeyboardMode] directly into the two call sites.

The code in the test runners would be something like this:

    static BOOL returnNo()
    {
        return NO;
    }

    method_setImplementation(class_getClassMethod([UIKeyboard class], @selector(isInHardwareKeyboardMode)), reinterpret_cast<IMP>(returnNo));

[Daniel Bates]

(In reply to mitz from comment #11)
> Comment on attachment 350975 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350975&action=review
> 
> > Source/WebKit/Shared/ios/NativeWebKeyboardEventIOS.mm:39
> > +    return !WebCore::IOSApplication::isDumpRenderTree() && !WebCore::IOSApplication::isWebKitTestRunner() && [UIKeyboard isInHardwareKeyboardMode];
> 
> Have you considered not introducing isWebKitTestRunner() and instead just
> having those two clients use the Objective-C runtime to dynamically override
> the behavior of +isInHardwareKeyboardMode?

I like this suggestion. Will look to swizzle +isInHardwareKeyboardMode and remove isWebKitTestRunner() shortly.

[Daniel Bates]

(In reply to Darin Adler from comment #12)
> Comment on attachment 350975 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350975&action=review
> 
> >> Source/WebKit/Shared/ios/NativeWebKeyboardEventIOS.mm:39
> >> +    return !WebCore::IOSApplication::isDumpRenderTree() && !WebCore::IOSApplication::isWebKitTestRunner() && [UIKeyboard isInHardwareKeyboardMode];
> > 
> > Have you considered not introducing isWebKitTestRunner() and instead just having those two clients use the Objective-C runtime to dynamically override the behavior of +isInHardwareKeyboardMode? We can often avoid adding testing-only complexity to WebKit by using generic testing techniques like that (and we often do!).
> 
> I strongly support this suggestion.
> 
> Besides Dan’s reasoning above, we also would like to avoid backward
> dependencies, where WebKit specifically knows about which testing tools
> exist and enumerates them, and instead have the testing tools make calls to
> change behavior. If we couldn’t use the Objective-C runtime to do this, the
> alternative solution I would propose would be some function for the test
> tools to call to change behavior, rather than code where WebKit itself
> checks if it’s running under a test tool.
> 
> Since it’s quite straightforward to override +[UIKeyboard
> isInHardwareKeyboardMode] to make it return NO, we can simply put calls to
> [UIKeyboard isInHardwareKeyboardMode] directly into the two call sites.
> 
> The code in the test runners would be something like this:
> 
>     static BOOL returnNo()
>     {
>         return NO;
>     }
> 
>     method_setImplementation(class_getClassMethod([UIKeyboard class],
> @selector(isInHardwareKeyboardMode)), reinterpret_cast<IMP>(returnNo));

Will look to do this.

[Daniel Bates]

(In reply to Daniel Bates from comment #13)
> (In reply to mitz from comment #11)
> > Comment on attachment 350975 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350975&action=review
> > 
> > > Source/WebKit/Shared/ios/NativeWebKeyboardEventIOS.mm:39
> > > +    return !WebCore::IOSApplication::isDumpRenderTree() && !WebCore::IOSApplication::isWebKitTestRunner() && [UIKeyboard isInHardwareKeyboardMode];
> > 
> > Have you considered not introducing isWebKitTestRunner() and instead just
> > having those two clients use the Objective-C runtime to dynamically override
> > the behavior of +isInHardwareKeyboardMode?
> 
> I like this suggestion. Will look to swizzle +isInHardwareKeyboardMode and
> remove isWebKitTestRunner() shortly.

Filed bug #190141 for this.

[Truitt Savell]

Looks like https://trac.webkit.org/changeset/236619/webkit

has caused one API failure and two API crashes on iOS
Test suite failed

Failed

    TestWebKitAPI.WKWebViewAutofillTests.AutofillRequiresInputSession
        
        /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:176
        Value of: [webView textInputHasAutofillContext]
          Actual: true
        Expected: false
        

Crashed

    TestWebKitAPI.DragAndDropTests.ExternalSourceJPEGOnly
        2018-09-28 17:08:26.849 TestWebKitAPI[44882:20535689] *** Assertion failure in NSComparisonResult UIContentSizeCategoryCompareToCategory(__strong UIContentSizeCategory _Nonnull, __strong UIContentSizeCategory _Nonnull)(), /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKitCore_Sim/UIKit-3698.84.15/UIContentSizeCategory.m:87
        Child process terminated with signal 11: Segmentation fault

    TestWebKitAPI.DragAndDropTests.ExternalSourceUTF8PlainTextOnly
        2018-09-28 17:08:36.948 TestWebKitAPI[44923:20536258] *** Assertion failure in NSComparisonResult UIContentSizeCategoryCompareToCategory(__strong UIContentSizeCategory _Nonnull, __strong UIContentSizeCategory _Nonnull)(), /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKitCore_Sim/UIKit-3698.84.15/UIContentSizeCategory.m:87
        Child process terminated with signal 11: Segmentation fault


Log:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/135/steps/run-api-tests/logs/stdio

[Daniel Bates]

(In reply to Truitt Savell from comment #16)
> Looks like https://trac.webkit.org/changeset/236619/webkit
> 
> has caused one API failure and two API crashes on iOS
> Test suite failed
> 
> Failed
> 
>     TestWebKitAPI.WKWebViewAutofillTests.AutofillRequiresInputSession
>         
>        
> /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/
> ios/WKWebViewAutofillTests.mm:176
>         Value of: [webView textInputHasAutofillContext]
>           Actual: true
>         Expected: false
>         
> 
> Crashed
> 
>     TestWebKitAPI.DragAndDropTests.ExternalSourceJPEGOnly
>         2018-09-28 17:08:26.849 TestWebKitAPI[44882:20535689] *** Assertion
> failure in NSComparisonResult
> UIContentSizeCategoryCompareToCategory(__strong UIContentSizeCategory
> _Nonnull, __strong UIContentSizeCategory _Nonnull)(),
> /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKitCore_Sim/UIKit-3698.84.
> 15/UIContentSizeCategory.m:87
>         Child process terminated with signal 11: Segmentation fault
> 
>     TestWebKitAPI.DragAndDropTests.ExternalSourceUTF8PlainTextOnly
>         2018-09-28 17:08:36.948 TestWebKitAPI[44923:20536258] *** Assertion
> failure in NSComparisonResult
> UIContentSizeCategoryCompareToCategory(__strong UIContentSizeCategory
> _Nonnull, __strong UIContentSizeCategory _Nonnull)(),
> /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKitCore_Sim/UIKit-3698.84.
> 15/UIContentSizeCategory.m:87
>         Child process terminated with signal 11: Segmentation fault
> 
> 
> Log:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/135/steps/
> run-api-tests/logs/stdio

Committed temporary fix in <https://trac.webkit.org/changeset/236751>. Filed bug #190211 to override +[UIKeyboard isInHardwareKeyboardMode] in all TestWebKitAPI tests.

[Dawei Fenton (:realdawei)]

Reverted r236751 for reason:

broke the iOS Build

Committed r236755: <https://trac.webkit.org/changeset/236755>

[Daniel Bates]

(In reply to Dawei Fenton (:realdawei) from comment #18)
> Reverted r236751 for reason:
> 
> broke the iOS Build
> 
> Committed r236755: <https://trac.webkit.org/changeset/236755>

Declared UIKeyboard interface in TestWebKitAPI/ios/UIKitSPI.h and re-landed in <https://trac.webkit.org/changeset/236761>.