...to be exposed only as editing SPI on WKWebView.
<rdar://problem/45955922>
Created attachment 354484 [details] Patch
Created attachment 354485 [details] Remove some debugging code.
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.
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);
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).
Created attachment 354512 [details] Patch for EWS (w/ new layout test)
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>
All reviewed patches have been landed. Closing bug.
You forgot to run update-webkit-localizable-strings after this.
(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.
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.
(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.