Bug 187554 - [iOS] WKWebView should match UITextView behavior when editing text with an RTL keyboard
Summary: [iOS] WKWebView should match UITextView behavior when editing text with an RT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 192393 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-07-11 09:42 PDT by mitz
Modified: 2018-12-06 13:22 PST (History)
8 users (show)

See Also:


Attachments
WIP (46.77 KB, patch)
2018-12-05 22:52 PST, Wenson Hsieh
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.37 MB, application/zip)
2018-12-06 00:44 PST, Build Bot
no flags Details
First pass (39.60 KB, patch)
2018-12-06 09:52 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (40.94 KB, patch)
2018-12-06 11:25 PST, Wenson Hsieh
wenson_hsieh: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (41.82 KB, patch)
2018-12-06 12:52 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 2018-12-06 00:44:24 PST Comment hidden (obsolete)
Comment 5 Build Bot 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.