[iOS WK2] WKWebView schedules nonstop layout after pressing cmb+b,i,u inside a contenteditable div

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/28279301>

[Wenson Hsieh]

Created attachment 317093 [details]
WIP, not for review

[Build Bot]

Comment on attachment 317093 [details]
WIP, not for review

Attachment 317093 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4245083

New failing tests:
editing/inserting/insert-div-024.html
editing/inserting/insert-div-026.html
editing/style/5084241.html
editing/style/unbold-in-bold.html

[Build Bot]

Created attachment 317101 [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.12.5

[Wenson Hsieh]

These 4 test failures are due to a lack of:

    RenderBlock (anonymous) at (0,<Y>) size 784x0

...in the render tree. Otherwise the DOM appears correct. I wonder if this is a byproduct of/related to how we keep on adding and removing temporary span elements while the caret is at a position with a B/I/U typing style.

[Wenson Hsieh]

After a bit of investigation, I believe the test expectations here can be safely rebaselined. This anonymous RenderBlock is inserted as a result of us currently inserting and removing a span in Editor::styleForSelectionStart.

I was able to "fix" these tests locally by adding code in WebPage::editorState to add and then remove a span if we have a typing style at the selection start (even if the temporary span isn't styled). This suggests that the anonymous render objects are just a result of us tweaking the DOM in WebPage::editorState. Furthermore, removing "RenderBlock (anonymous) at (0,__) size 784x0" from the expected render tree makes the WK2 expectations consistent with WK1 expectations for 3 of the LayoutTests: insert-div-024.html, insert-div-026.html and 5084241.html. unbold-in-bold.html does not have iOS WK1 expectations.

[Wenson Hsieh]

Created attachment 317296 [details]
Test patch 2

[Wenson Hsieh]

Created attachment 317329 [details]
Patch

[Wenson Hsieh]

Created attachment 317333 [details]
Fix 32-bit Mac build

[Darin Adler]

Comment on attachment 317333 [details]
Fix 32-bit Mac build

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

> Source/WebCore/css/StyleProperties.h:118
> +    WEBCORE_EXPORT std::optional<Color> getPropertyAsColor(CSSPropertyID) const;
> +    WEBCORE_EXPORT CSSValueID getPropertyAsValueID(CSSPropertyID) const;
> +    WEBCORE_EXPORT std::optional<float> getPropertyAsFloat(CSSPropertyID) const;

Since these are not to be exposed as public DOM API then I think they should follow normal WebKit coding style, and not have the word "get" in their names. I also think they should possibly be in a separate paragraph, since they aren’t really the same kind of thing as the web-exposed DOM API.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1088
> +static NSTextAlignment nsTextAlignmentForTextAlignment(WebKit::TextAlignment alignment)

I’m not sure that we need to include "ForTextAlignment" in the name of this function; given C++ overloading rules it seems like overkill and it also makes the function name so long.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1100
> +    default:
> +        return NSTextAlignmentNatural;

Better coding style is to lead out default so we get a waring if any case value is not handled. Also we should ASSERT_NOT_REACHED if the value is not one of the expected ones, outside the switch.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1124
> +    id <WKUIDelegatePrivate> uiDelegate = (id <WKUIDelegatePrivate>)self.UIDelegate;
> +    if ([uiDelegate respondsToSelector:@selector(_webView:editorStateDidChange:)])
> +        [uiDelegate _webView:self editorStateDidChange:dictionaryRepresentationForEditorState(_page->editorState())];

Does something guarantee this is never called when _page is null?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:874
> +                    if (styleProperties->getPropertyValue(CSSPropertyFontStyle) == "italic")
> +                        postLayoutData.typingAttributes |= AttributeItalics;

This does not seem quite right. I wrote considerably more complex code for this in EditorCommand.cpp that checks for more than just the literal string "italic", and there are other versions in ContextMenuController.cpp (uses selectionHasStyle which uses EditingStyle::triStateOfStyle) and HTMLConverter.mm (checks for two hard coded strings, "italic" and "oblique"). I am annoyed there are so many versions. I also wonder why this can’t use getPropertyAsValueID.

I am concerned that we are writing more and more editing code that does *almost* the same thing rather than refactoring so we uses shared code that works consistently.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:876
> +                    if (styleProperties->getPropertyValue(CSSPropertyWebkitTextDecorationsInEffect) == "underline")

Why not getPropertyAsValueID?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:910
> +            if (RefPtr<HTMLElement> enclosingListElement = enclosingList(selection.start().deprecatedNode())) {

Why a RefPtr? All we do with this list is check its type; no need for a smart pointer. Should just be auto*.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:933
> +    if (commandName == "toggleBold" || commandName == "toggleItalic" || commandName == "toggleUnderline")
> +        send(Messages::WebPageProxy::EditorStateChanged(editorState()));

I know this code was just moved, but I think it needs a why comment. It’s strange to have three specific command names listed here, so we need some explanation why. Maybe even a named function to help document what is special about these three.

[Wenson Hsieh]

Comment on attachment 317333 [details]
Fix 32-bit Mac build

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

Thanks for taking a look! I'll make adjustments and upload a new version, since I'm going to make a few non-trivial tweaks to style computation in WebPage::editorState (so that it goes through the triStateOfStyle codepath instead of just checking strings).

>> Source/WebCore/css/StyleProperties.h:118
>> +    WEBCORE_EXPORT std::optional<float> getPropertyAsFloat(CSSPropertyID) const;
> 
> Since these are not to be exposed as public DOM API then I think they should follow normal WebKit coding style, and not have the word "get" in their names. I also think they should possibly be in a separate paragraph, since they aren’t really the same kind of thing as the web-exposed DOM API.

Good point -- renamed.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1088
>> +static NSTextAlignment nsTextAlignmentForTextAlignment(WebKit::TextAlignment alignment)
> 
> I’m not sure that we need to include "ForTextAlignment" in the name of this function; given C++ overloading rules it seems like overkill and it also makes the function name so long.

That makes sense -- changed to just nsTextAlignment()

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1100
>> +        return NSTextAlignmentNatural;
> 
> Better coding style is to lead out default so we get a waring if any case value is not handled. Also we should ASSERT_NOT_REACHED if the value is not one of the expected ones, outside the switch.

Ok! Fixed.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1124
>> +        [uiDelegate _webView:self editorStateDidChange:dictionaryRepresentationForEditorState(_page->editorState())];
> 
> Does something guarantee this is never called when _page is null?

_page should be non-null here, since all calls to -[WKWebView _didChangeEditorState] are just plumbed from WebPageProxy::editorStateChanged, through the page client, and the web view impl (Mac) or the content view (iOS).

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:874
>> +                        postLayoutData.typingAttributes |= AttributeItalics;
> 
> This does not seem quite right. I wrote considerably more complex code for this in EditorCommand.cpp that checks for more than just the literal string "italic", and there are other versions in ContextMenuController.cpp (uses selectionHasStyle which uses EditingStyle::triStateOfStyle) and HTMLConverter.mm (checks for two hard coded strings, "italic" and "oblique"). I am annoyed there are so many versions. I also wonder why this can’t use getPropertyAsValueID.
> 
> I am concerned that we are writing more and more editing code that does *almost* the same thing rather than refactoring so we uses shared code that works consistently.

I see...it seems to me that selectionStartHasStyle is really what I wanted here in the first place. Adopting this would then make us consistent with ContextMenuController!

However, I'd also want to avoid repeatedly computing EditingStyle::styleAtSelectionStart every style query here, so maybe I could add something like bool EditorState::hasStyle(CSSPropertyID, const String& value) that can be called directly from WebKit1/WebKit2, but that selectionStartHasStyle would also use. This way, WebPage::editorState would funnel through the EditingStyle::triStateOfStyle codepath as well, but not have to perform unnecessary work by calling EditingStyle::styleAtSelectionStart repeatedly.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:876
>> +                    if (styleProperties->getPropertyValue(CSSPropertyWebkitTextDecorationsInEffect) == "underline")
> 
> Why not getPropertyAsValueID?

I think the refactoring above should address this.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:910
>> +            if (RefPtr<HTMLElement> enclosingListElement = enclosingList(selection.start().deprecatedNode())) {
> 
> Why a RefPtr? All we do with this list is check its type; no need for a smart pointer. Should just be auto*.

True, RefPtr here is just unnecessary overhead. Changed to auto*.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:933
>> +        send(Messages::WebPageProxy::EditorStateChanged(editorState()));
> 
> I know this code was just moved, but I think it needs a why comment. It’s strange to have three specific command names listed here, so we need some explanation why. Maybe even a named function to help document what is special about these three.

I agree, this is quite odd. I think this warrants more investigation. Perhaps we could make this a bit more palatable for the time being by pulling it out into a new static helper to the effect of shouldEnsureEditorStateUpdateAfterExecutingCommand(const String& commandName) that returns YES only for these commands.

[Wenson Hsieh]

Created attachment 317358 [details]
Adjust for review feedback.

[Ryosuke Niwa]

Comment on attachment 317358 [details]
Adjust for review feedback.

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:906
> +            if (auto* enclosingListElement = enclosingList(selection.start().deprecatedNode())) {

Please use containerNode() instead.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:52
> +    [testHarness insertHTML:@"<span style='font-weight: 800'> third</span>" andExpectEditorStateWith:@{ @"bold": @YES }];

Please add a test for font-weight: 400, 300, 700, etc...

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:73
> +    [testHarness insertHTML:@"<span style='font-style: italic'> third</span>" andExpectEditorStateWith:@{ @"italic": @YES }];

Please add a test case for oblique, which we treat as italic.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:96
> +    [testHarness insertHTML:@"<span style='text-decoration: underline'> third</span>" andExpectEditorStateWith:@{ @"underline": @YES }];

Please add a test case when line-through and other text-decorations are also specified.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:145
> +    [webView stringByEvaluatingJavaScript:@"document.body.style.direction = 'rtl'"];

Please adda  test case with dir="auto" where the directionality is determined by a strongly RTL letter.

[Wenson Hsieh]

Comment on attachment 317358 [details]
Adjust for review feedback.

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:906
>> +            if (auto* enclosingListElement = enclosingList(selection.start().deprecatedNode())) {
> 
> Please use containerNode() instead.

👍

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:52
>> +    [testHarness insertHTML:@"<span style='font-weight: 800'> third</span>" andExpectEditorStateWith:@{ @"bold": @YES }];
> 
> Please add a test for font-weight: 400, 300, 700, etc...

👍

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:73
>> +    [testHarness insertHTML:@"<span style='font-style: italic'> third</span>" andExpectEditorStateWith:@{ @"italic": @YES }];
> 
> Please add a test case for oblique, which we treat as italic.

👍

BTW: I also added "oblique" as a case to consider when computing WebPage::editorState.

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:96
>> +    [testHarness insertHTML:@"<span style='text-decoration: underline'> third</span>" andExpectEditorStateWith:@{ @"underline": @YES }];
> 
> Please add a test case when line-through and other text-decorations are also specified.

👍

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/EditorStateTests.mm:145
>> +    [webView stringByEvaluatingJavaScript:@"document.body.style.direction = 'rtl'"];
> 
> Please adda  test case with dir="auto" where the directionality is determined by a strongly RTL letter.

👍

[Wenson Hsieh]

Created attachment 317537 [details]
Patch for landing

[Build Bot]

Comment on attachment 317537 [details]
Patch for landing

Attachment 317537 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4274866

New failing tests:
quicklook/multi-sheet-numbers-09.html

[Build Bot]

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

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

[Wenson Hsieh]

Comment on attachment 317537 [details]
Patch for landing

The test failure above (quicklook/multi-sheet-numbers-09.html) looks unrelated -- I ran this test locally and passed.

[WebKit Commit Bot]

Comment on attachment 317537 [details]
Patch for landing

Clearing flags on attachment: 317537

Committed r220393: <http://trac.webkit.org/changeset/220393>

[Wenson Hsieh]

Reverted r220393 for reason:

This patch regresses the performance of WebPage::editorState.

Committed r220443: <http://trac.webkit.org/changeset/220443>

[Wenson Hsieh]

Re-landed in https://trac.webkit.org/changeset/221065/webkit.

Comment on attachment 317537 [details]
Patch for landing

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

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:81
> +- (void)_webView:(WKWebView *)webView editorStateDidChange:(NSDictionary *)editorState WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Strange to see an NSDictionary * as an argument type in an API. Is this intended solely to support testing?

[Wenson Hsieh]

(In reply to mitz from comment #21)
> Comment on attachment 317537 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317537&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:81
> > +- (void)_webView:(WKWebView *)webView editorStateDidChange:(NSDictionary *)editorState WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Strange to see an NSDictionary * as an argument type in an API. Is this
> intended solely to support testing?

Correct -- this is only here to support testing.

[Radar WebKit Bug Importer]

<rdar://problem/34694091>