Bug 191487 – Implement a new edit command to change the enclosing list type

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

...to be exposed only as editing SPI on WKWebView.

[Radar WebKit Bug Importer]

<rdar://problem/45955922>

[Wenson Hsieh]

Created attachment 354484 [details]
Patch

[Wenson Hsieh]

Created attachment 354485 [details]
Remove some debugging code.

[Ryosuke Niwa]

Comment on attachment 354485 [details]
Remove some debugging code.

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

> Source/WebCore/editing/ChangeListTypeCommand.cpp:81
> +    RefPtr<HTMLElement> listToReplace;
> +    auto type = listConversionTypeForSelection(endingSelection(), listToReplace);

Why don't we make this return both type & RefPtr using one time struct or pair?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm:129
> +    auto webView = webViewForEditActionTestingWithPageNamed(@"editable-nested-lists");
> +
> +    [webView setPosition:@"one" offset:1];
> +    [webView _changeListType:nil];
> +    EXPECT_TRUE([webView querySelectorExists:@"ol > li#one.item"]);

I think we should add a layout test instead.
In general, we should be writing layout tests over API tests whenever possible.

[Ryosuke Niwa]

Comment on attachment 354485 [details]
Remove some debugging code.

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

> Source/WebCore/editing/ChangeListTypeCommand.cpp:43
> +    auto* commonAncestor = Range::commonAncestorContainer(selection.start().containerNode(), selection.end().containerNode());

We should use RefPtr here.

> Source/WebCore/editing/ChangeListTypeCommand.cpp:60
> +    if (auto* frame = document.frame()) {

Ditto.

> Source/WebCore/editing/ChangeListTypeCommand.cpp:73
> +    auto list = ([this] () -> Ref<HTMLElement> {
> +        if (m_type == Type::ConvertToOrderedList)
> +            return HTMLOListElement::create(document());
> +        return HTMLUListElement::create(document());
> +    })();

What's the point of creating a lambda here and calling it immediately?
Why can't just do:
RefPtr<HTMLElement> list = m_type == Type::ConvertToOrderedList ? HTMLOListElement::create(document()) : HTMLUListElement::create(document())
Or alternatively,
auto list = document()->createElement(m_type == Type::ConvertToOrderedList ?  HTMLNames::olTag : HTMLNames::ulTag);

[Wenson Hsieh]

Comment on attachment 354485 [details]
Remove some debugging code.

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

Thanks for the review!

>> Source/WebCore/editing/ChangeListTypeCommand.cpp:43
>> +    auto* commonAncestor = Range::commonAncestorContainer(selection.start().containerNode(), selection.end().containerNode());
> 
> We should use RefPtr here.

Fixed!

>> Source/WebCore/editing/ChangeListTypeCommand.cpp:60
>> +    if (auto* frame = document.frame()) {
> 
> Ditto.

Fixed!

>> Source/WebCore/editing/ChangeListTypeCommand.cpp:73
>> +    })();
> 
> What's the point of creating a lambda here and calling it immediately?
> Why can't just do:
> RefPtr<HTMLElement> list = m_type == Type::ConvertToOrderedList ? HTMLOListElement::create(document()) : HTMLUListElement::create(document())
> Or alternatively,
> auto list = document()->createElement(m_type == Type::ConvertToOrderedList ?  HTMLNames::olTag : HTMLNames::ulTag);

Interestingly, I originally tried to do this:
`RefPtr<HTMLElement> list = m_type == Type::ConvertToOrderedList ? HTMLOListElement::create(document()) : HTMLUListElement::create(document());`

...but that results in an error like this:
In file included from /Volumes/main/Users/whsieh/Build/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource233.cpp:6:
editing/ChangeListTypeCommand.cpp:73:69: error: conditional expression is ambiguous; 'Ref<WebCore::HTMLOListElement>' can be converted to 'Ref<WebCore::HTMLUListElement>' and vice versa
    RefPtr<HTMLElement> list = m_type == Type::ConvertToOrderedList ? HTMLOListElement::create(document()) : HTMLUListElement::create(document());
                                                                    ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
editing/ChangeListTypeCommand.cpp:75:12: error: no viable conversion from returned value of type 'RefPtr<WebCore::HTMLElement>' to function return type 'Ref<WebCore::HTMLElement>'
    return list;

...and I also avoided the second approach because I felt calling Document::createElement() from within WebCore code was strange, so I settled on using a lambda to upcast Ref<HTMLOListElement> and Ref<HTMLUListElement> to Ref<HTMLElement>.

I'll replace the lambda here with something like this:

RefPtr<HTMLElement> list;
if (m_type == Type::ConvertToOrderedList)
    list = HTMLOListElement::create(document());
else
    list = HTMLUListElement::create(document());

>> Source/WebCore/editing/ChangeListTypeCommand.cpp:81
>> +    auto type = listConversionTypeForSelection(endingSelection(), listToReplace);
> 
> Why don't we make this return both type & RefPtr using one time struct or pair?

Sounds good — I'll use std::pair here to return both.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm:129
>> +    EXPECT_TRUE([webView querySelectorExists:@"ol > li#one.item"]);
> 
> I think we should add a layout test instead.
> In general, we should be writing layout tests over API tests whenever possible.

I generally agree, but in this case, this functionality is only exposed as WebKit SPI.

That being said, I definitely see the value in making this a layout test, so we can use Markup.dump instead of checking for few selectors here and there. I'll add an Internals hook so that we can invoke this from the test runner, and then convert this API test into a layout test. But since there's still value in having a test that ensures WebKit SPI is hooked up to the edit command, I'll keep an API test here, but make it much simpler (so that it just exercises the most basic scenario).

[Wenson Hsieh]

Created attachment 354512 [details]
Patch for EWS (w/ new layout test)

[WebKit Commit Bot]

Comment on attachment 354512 [details]
Patch for EWS (w/ new layout test)

Clearing flags on attachment: 354512

Committed r238080: <https://trac.webkit.org/changeset/238080>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

You forgot to run update-webkit-localizable-strings after this.

[Wenson Hsieh]

(In reply to Alex Christensen from comment #10)
> You forgot to run update-webkit-localizable-strings after this.

Good catch!

It looks like this was fixed in r238332.

[Tim Horton]

Comment on attachment 354512 [details]
Patch for EWS (w/ new layout test)

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

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:656
> +    case EditAction::ConvertToOrderedList: return UI_STRING_KEY_INTERNAL("Convert to Ordered List", "Convert to Ordered List (Undo action name)", "Convert to ordered list action name");

I *think* the third parameter is supposed to just be "Undo action name", not specific to this action.

[Wenson Hsieh]

(In reply to Tim Horton from comment #12)
> Comment on attachment 354512 [details]
> Patch for EWS (w/ new layout test)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354512&action=review
> 
> > Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:656
> > +    case EditAction::ConvertToOrderedList: return UI_STRING_KEY_INTERNAL("Convert to Ordered List", "Convert to Ordered List (Undo action name)", "Convert to ordered list action name");
> 
> I *think* the third parameter is supposed to just be "Undo action name", not
> specific to this action.

↳ https://bugs.webkit.org/show_bug.cgi?id=191945

Also, might as well take this opportunity to update Localizable.strings.