Bug 187554 – [iOS] WKWebView should match UITextView behavior when editing text with an RTL keyboard

• Component: HTML Editing
• Status: RESOLVED FIXED

In UIWebView, when the insertion point is inside an empty block in editable content, switching keyboards causes the dir attribute of the block to be set to match the writing direction associated with the keyboard. In WKWebView this isn’t happening.

<rdar://problem/42075638>

[Wenson Hsieh]

*** Bug 192393 has been marked as a duplicate of this bug. ***

[Wenson Hsieh]

Created attachment 356716 [details]
WIP

[EWS Watchlist]

Comment on attachment 356716 [details]
WIP

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

New failing tests:
editing/input/ios/rtl-keyboard-input-on-focus.html

[EWS Watchlist]

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

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

[Wenson Hsieh]

Created attachment 356736 [details]
First pass

[Tim Horton]

Comment on attachment 356736 [details]
First pass

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1875
> +    if (isValid())

This is a surprising and bizarre pattern! Would much prefer if these were all early returns, despite being more lines.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:911
> +    m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement = false;

Do we get here if the process dies? (otherwise, you might need to reset this there too?

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:176
> +
> +    if (options.editable)
> +        m_mainWebView->setEditable(true);

How did we not have this before

[Wenson Hsieh]

Created attachment 356738 [details]
Patch for EWS

[Wenson Hsieh]

Comment on attachment 356736 [details]
First pass

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1875
>> +    if (isValid())
> 
> This is a surprising and bizarre pattern! Would much prefer if these were all early returns, despite being more lines.

Ok! I'll change these to early returns.

>> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:911
>> +    m_waitingForPostLayoutEditorStateUpdateAfterFocusingElement = false;
> 
> Do we get here if the process dies? (otherwise, you might need to reset this there too?

Good catch — yes, this flag (as well as m_deferredNodeAssistanceArguments) should both be reset!

>> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:176
>> +        m_mainWebView->setEditable(true);
> 
> How did we not have this before

🤷🏻‍♂️

[Darin Adler]

Comment on attachment 356738 [details]
Patch for EWS

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

> Source/WebKit/UIProcess/WebPageProxy.h:87
> +#include <WebCore/WritingDirection.h>

Should be able to use forward declaration instead of include in this case.

> Source/WebKit/WebProcess/WebPage/WebPage.h:73
> +#include <WebCore/WritingDirection.h>

Ditto.

[Wenson Hsieh]

(In reply to Darin Adler from comment #10)
> Comment on attachment 356738 [details]
> Patch for EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356738&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:87
> > +#include <WebCore/WritingDirection.h>
> 
> Should be able to use forward declaration instead of include in this case.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:73
> > +#include <WebCore/WritingDirection.h>
> 
> Ditto.

👍 will make these forward declarations before landing.

[Wenson Hsieh]

Created attachment 356741 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356741 [details]:

workers/bomb.html bug 192050 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 356741 [details]
Patch for landing

Clearing flags on attachment: 356741

Committed r238939: <https://trac.webkit.org/changeset/238939>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.