Bug 187554

Summary: [iOS] WKWebView should match UITextView behavior when editing text with an RTL keyboard
Product: WebKit Reporter: mitz
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, darin, ews-watchlist, megan_gardner, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
ews-watchlist: commit-queue-
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
First pass
none
Patch for EWS
wenson_hsieh: commit-queue+
Patch for landing none

Description mitz 2018-07-11 09:42:49 PDT
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.
Comment 1 mitz 2018-07-11 09:44:11 PDT
<rdar://problem/42075638>
Comment 2 Wenson Hsieh 2018-12-04 21:09:21 PST
*** Bug 192393 has been marked as a duplicate of this bug. ***
Comment 3 Wenson Hsieh 2018-12-05 22:52:55 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-12-06 00:44:24 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-12-06 00:44:25 PST Comment hidden (obsolete)
Comment 6 Wenson Hsieh 2018-12-06 09:52:21 PST
Created attachment 356736 [details]
First pass
Comment 7 Tim Horton 2018-12-06 11:04:11 PST
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
Comment 8 Wenson Hsieh 2018-12-06 11:25:28 PST
Created attachment 356738 [details]
Patch for EWS
Comment 9 Wenson Hsieh 2018-12-06 11:25:44 PST
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

πŸ€·πŸ»β€β™‚οΈ
Comment 10 Darin Adler 2018-12-06 12:33:55 PST
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.
Comment 11 Wenson Hsieh 2018-12-06 12:45:06 PST
(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.
Comment 12 Wenson Hsieh 2018-12-06 12:52:52 PST
Created attachment 356741 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2018-12-06 13:21:30 PST
The commit-queue encountered the following flaky tests while processing attachment 356741 [details]:

workers/bomb.html bug 192050 (author: fpizlo@apple.com)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2018-12-06 13:22:28 PST
Comment on attachment 356741 [details]
Patch for landing

Clearing flags on attachment: 356741

Committed r238939: <https://trac.webkit.org/changeset/238939>
Comment 15 WebKit Commit Bot 2018-12-06 13:22:30 PST
All reviewed patches have been landed.  Closing bug.