Bug 190017 - [iOS] Allow programmatic focus when hardware keyboard is attached
Summary: [iOS] Allow programmatic focus when hardware keyboard is attached
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 11
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks: 190564 190571 194570
  Show dependency treegraph
 
Reported: 2018-09-26 16:26 PDT by Daniel Bates
Modified: 2019-03-19 17:21 PDT (History)
7 users (show)

See Also:


Attachments
Work-in-progress (3.43 KB, patch)
2018-09-26 16:28 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.40 MB, application/zip)
2018-09-26 21:47 PDT, Build Bot
no flags Details
Patch (9.17 KB, patch)
2018-09-27 10:48 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-09-26 16:26:08 PDT
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.
Comment 1 Daniel Bates 2018-09-26 16:26:20 PDT
<rdar://problem/42270463>
Comment 2 Daniel Bates 2018-09-26 16:28:15 PDT
Created attachment 350922 [details]
Work-in-progress
Comment 3 Build Bot 2018-09-26 21:47:48 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2018-09-26 21:47:50 PDT Comment hidden (obsolete)
Comment 5 Daniel Bates 2018-09-27 10:48:58 PDT
Created attachment 350975 [details]
Patch
Comment 6 Wenson Hsieh 2018-09-28 11:32:00 PDT
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).
Comment 7 Daniel Bates 2018-09-28 12:06:31 PDT
(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>.
Comment 8 Wenson Hsieh 2018-09-28 12:16:20 PDT
(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>.
Comment 9 Daniel Bates 2018-09-28 16:02:45 PDT
Comment on attachment 350975 [details]
Patch

Clearing flags on attachment: 350975

Committed r236619: <https://trac.webkit.org/changeset/236619>
Comment 10 Daniel Bates 2018-09-28 16:02:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 mitz 2018-09-28 16:19:22 PDT
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!).
Comment 12 Darin Adler 2018-09-30 19:55:55 PDT
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));
Comment 13 Daniel Bates 2018-09-30 20:48:03 PDT
(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.
Comment 14 Daniel Bates 2018-09-30 20:48:33 PDT
(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.
Comment 15 Daniel Bates 2018-10-01 10:09:41 PDT
(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.
Comment 16 Truitt Savell 2018-10-01 14:46:11 PDT
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
Comment 17 Daniel Bates 2018-10-02 10:52:50 PDT
(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.
Comment 18 Dawei Fenton (:realdawei) 2018-10-02 11:39:14 PDT
Reverted r236751 for reason:

broke the iOS Build

Committed r236755: <https://trac.webkit.org/changeset/236755>
Comment 19 Daniel Bates 2018-10-02 12:44:28 PDT
(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>.