Bug 191335 – Add an editing command for creating and inserting child lists

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/45814050>

[Wenson Hsieh]

Created attachment 354022 [details]
Patch

[EWS Watchlist]

Comment on attachment 354022 [details]
Patch

Attachment 354022 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9886301

New failing tests:
editing/execCommand/list-insertion-with-sublists.html

[EWS Watchlist]

Created attachment 354036 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354022 [details]
Patch

Attachment 354022 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9886564

New failing tests:
editing/execCommand/list-insertion-with-sublists.html

[EWS Watchlist]

Created attachment 354041 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

Created attachment 354051 [details]
Fix WK1 test failures

[Ryosuke Niwa]

Comment on attachment 354051 [details]
Fix WK1 test failures

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

> Source/WebCore/editing/InsertListCommand.cpp:201
> +    if (!frame().settings().listInsertionShouldCreateSublists() || m_allowSublistInsertion == AllowSublistInsertion::DoNotAllow)
> +        return false;

Why can't we just add a new editing command instead?
Using settings to change the behavior of existing editing command is going to debug these editing commands A LOT harder.
Since InsertListCommand is already impossible to maintain,
I'm EXTREMELY worried about the current approach being proposed.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 354051 [details]
> Fix WK1 test failures
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354051&action=review
> 
> > Source/WebCore/editing/InsertListCommand.cpp:201
> > +    if (!frame().settings().listInsertionShouldCreateSublists() || m_allowSublistInsertion == AllowSublistInsertion::DoNotAllow)
> > +        return false;
> 
> Why can't we just add a new editing command instead?

This was one of my original proposals. Dan and I discussed this, and thought it made sense to override the existing behavior of "Insert*List" so that all UI affordances in Mail for list insertion (e.g. including touch bar) would just do the right thing™️.

That being said, I'm open to adding a new edit command along the lines of "InsertUnorderedChildList" or "InsertOrderedChildList".

Dan, if there are any other reasons why Mail would prefer to go with the approach in the patch above that I missed, please let us know!

> Using settings to change the behavior of existing editing command is going
> to debug these editing commands A LOT harder.
> Since InsertListCommand is already impossible to maintain,
> I'm EXTREMELY worried about the current approach being proposed.

(In reply to Wenson Hsieh from comment #8)
> (In reply to Ryosuke Niwa from comment #7)
> > Comment on attachment 354051 [details]
> > Fix WK1 test failures
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354051&action=review
> > 
> > > Source/WebCore/editing/InsertListCommand.cpp:201
> > > +    if (!frame().settings().listInsertionShouldCreateSublists() || m_allowSublistInsertion == AllowSublistInsertion::DoNotAllow)
> > > +        return false;
> > 
> > Why can't we just add a new editing command instead?
> 
> This was one of my original proposals. Dan and I discussed this, and thought
> it made sense to override the existing behavior of "Insert*List" so that all
> UI affordances in Mail for list insertion (e.g. including touch bar) would
> just do the right thing™️.
> 
> That being said, I'm open to adding a new edit command along the lines of
> "InsertUnorderedChildList" or "InsertOrderedChildList".
> 
> Dan, if there are any other reasons why Mail would prefer to go with the
> approach in the patch above that I missed, please let us know!

When we discussed this, I was under the impression that shipping Mail’s own implementation of interactions with lists differs from WebKit’s built-in behavior not only in what the two insert actions do and how they’re validated, but also in how things like pressing Tab or Shift-Tab, or Return behave in the context of a list, and that it was not desirable or even practical to assign different actions to those key presses, nor for WebKit to keep track of the fact that a list had been created in a way that required those different behaviors within it. As such, simply having the ability to configure a web view to behave in that manner (which it could also become the default behavior for all new apps) seemed most desirable.

But if my impression was wrong, and your investigation into this concluded that the only difference between Mail’s implementation and WebKit’s behavior is in how the insert commands behave, then there is no material difference between configuring the web view to have that behavior or simply hooking up the UI to different actions.

> 
> > Using settings to change the behavior of existing editing command is going
> > to debug these editing commands A LOT harder.
> > Since InsertListCommand is already impossible to maintain,
> > I'm EXTREMELY worried about the current approach being proposed.

[Wenson Hsieh]

> When we discussed this, I was under the impression that shipping Mail’s own
> implementation of interactions with lists differs from WebKit’s built-in
> behavior not only in what the two insert actions do and how they’re
> validated, but also in how things like pressing Tab or Shift-Tab, or Return
> behave in the context of a list, and that it was not desirable or even
> practical to assign different actions to those key presses, nor for WebKit
> to keep track of the fact that a list had been created in a way that
> required those different behaviors within it. As such, simply having the
> ability to configure a web view to behave in that manner (which it could
> also become the default behavior for all new apps) seemed most desirable.

But Tab, Shift-tab, and newline trigger totally different editing commands, no? It seems that once the user is in a list, these three corresponding edit commands (indent, outdent, and insertParagraph) don't behave differently in stock WebKit than in Mail.

> But if my impression was wrong, and your investigation into this concluded
> that the only difference between Mail’s implementation and WebKit’s behavior
> is in how the insert commands behave, then there is no material difference
> between configuring the web view to have that behavior or simply hooking up
> the UI to different actions.

Yeah, I think the only difference is how list insertion is defined. As such, I would prefer to go with adding a couple of new editing commands that are suitable for Mail's list insertion semantics.

(In reply to Wenson Hsieh from comment #10)
> > When we discussed this, I was under the impression that shipping Mail’s own
> > implementation of interactions with lists differs from WebKit’s built-in
> > behavior not only in what the two insert actions do and how they’re
> > validated, but also in how things like pressing Tab or Shift-Tab, or Return
> > behave in the context of a list, and that it was not desirable or even
> > practical to assign different actions to those key presses, nor for WebKit
> > to keep track of the fact that a list had been created in a way that
> > required those different behaviors within it. As such, simply having the
> > ability to configure a web view to behave in that manner (which it could
> > also become the default behavior for all new apps) seemed most desirable.
> 
> But Tab, Shift-tab, and newline trigger totally different editing commands,
> no? It seems that once the user is in a list, these three corresponding edit
> commands (indent, outdent, and insertParagraph) don't behave differently in
> stock WebKit than in Mail.

That’s not what I see. In MiniBrowser, if I make a New WebKit1 Editor and navigate it to

data:text/html,<div%20contenteditable><ul><li>Place%20the%20insertion%20point%20at%20the%20end%20of%20this%20sentence,%20then%20hit%20Return,%20then%20Tab.

and follow the instructions, then I end up with a tab character after a second first-level bullet. In a Mail compose window, in the same situation, hitting Return then Tab makes a second list item, then turns it into a second-level list item.

What am I missing?

> 
> > But if my impression was wrong, and your investigation into this concluded
> > that the only difference between Mail’s implementation and WebKit’s behavior
> > is in how the insert commands behave, then there is no material difference
> > between configuring the web view to have that behavior or simply hooking up
> > the UI to different actions.
> 
> Yeah, I think the only difference is how list insertion is defined. As such,
> I would prefer to go with adding a couple of new editing commands that are
> suitable for Mail's list insertion semantics.

[Wenson Hsieh]

(In reply to mitz from comment #11)
> That’s not what I see. In MiniBrowser, if I make a New WebKit1 Editor and
> navigate it to
> 
> data:text/html,
> <div%20contenteditable><ul><li>Place%20the%20insertion%20point%20at%20the%20e
> nd%20of%20this%20sentence,%20then%20hit%20Return,%20then%20Tab.
> 
> and follow the instructions, then I end up with a tab character after a
> second first-level bullet. In a Mail compose window, in the same situation,
> hitting Return then Tab makes a second list item, then turns it into a
> second-level list item.
> 
> What am I missing?

After talking to Dan about this:

• Mail needs new SPI on WKWebView for list insertion…
    - insertUnorderedList:
    - insertOrderedList:
    - changeListType:
    - decreaseListLevel:
    - increaseListLevel:
• Mail also needs a configuration flag to opt into a family of list editing behaviors that will:
    - Indent on Tab
    - Outdent on Shift-Tab

[Wenson Hsieh]

Created attachment 354128 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 354128 [details]
Patch

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

> Source/WebCore/editing/EditorCommand.cpp:1600
> +        { "InsertOrderedChildList", { executeInsertOrderedChildList, supported, enabledInRichlyEditableText, stateOrderedList, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

It's a bit strange to call this editor command insert * child list since it can insert the outermost list as well.
How about InsertNestedOrderedList? Also, is it expected that this feature is exposed to the Web via execCommand?

> Source/WebCore/editing/InsertChildListCommand.cpp:52
> +    auto& document = this->document();

Do we really need this local variable? Calling document() in each place where it's used seems fine.
If we're using a local variable, I'd prefer having a Ref/RefPtr here instead of a reference here.
Who knows what happens to m_document.

> Source/WebCore/editing/InsertChildListCommand.cpp:58
> +        setEndingSelection({{ newListItem.ptr(), Position::PositionIsBeforeChildren }, DOWNSTREAM });

This is a bit cryptic why don't we explicitly say Position like so:
{Position { newListItem.ptr(), Position::PositionIsBeforeChildren }, DOWNSTREAM }

> LayoutTests/editing/execCommand/list-insertion-with-child-lists-expected.txt:94
> +|         <li>

Let's add test cases for when a list appears inside a table cell inside a list,
as well as a list inside pre, or the content you're listening is a pre.

> LayoutTests/editing/execCommand/list-insertion-with-child-lists.html:16
> +    [..."foo"].map(typeCharacterCommand);

Haha, this is pretty neat.

[Wenson Hsieh]

Comment on attachment 354128 [details]
Patch

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

>> Source/WebCore/editing/EditorCommand.cpp:1600
>> +        { "InsertOrderedChildList", { executeInsertOrderedChildList, supported, enabledInRichlyEditableText, stateOrderedList, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
> 
> It's a bit strange to call this editor command insert * child list since it can insert the outermost list as well.
> How about InsertNestedOrderedList? Also, is it expected that this feature is exposed to the Web via execCommand?

InsertNestedOrderedList and InsertNestedUnorderedList sounds good — I'll change to use those names.

I think it would be nice to expose this to the web, though it's certainly not a requirement. Do you think we ought to gate this behind an SPI preference/configuration flag?

>> Source/WebCore/editing/InsertChildListCommand.cpp:52
>> +    auto& document = this->document();
> 
> Do we really need this local variable? Calling document() in each place where it's used seems fine.
> If we're using a local variable, I'd prefer having a Ref/RefPtr here instead of a reference here.
> Who knows what happens to m_document.

True. Changed to use document()

>> Source/WebCore/editing/InsertChildListCommand.cpp:58
>> +        setEndingSelection({{ newListItem.ptr(), Position::PositionIsBeforeChildren }, DOWNSTREAM });
> 
> This is a bit cryptic why don't we explicitly say Position like so:
> {Position { newListItem.ptr(), Position::PositionIsBeforeChildren }, DOWNSTREAM }

Ok! Changed to explicitly Position { }.

>> LayoutTests/editing/execCommand/list-insertion-with-child-lists-expected.txt:94
>> +|         <li>
> 
> Let's add test cases for when a list appears inside a table cell inside a list,
> as well as a list inside pre, or the content you're listening is a pre.

Sounds good — I'll test these cases.

>> LayoutTests/editing/execCommand/list-insertion-with-child-lists.html:16
>> +    [..."foo"].map(typeCharacterCommand);
> 
> Haha, this is pretty neat.

:)

[Wenson Hsieh]

Created attachment 354163 [details]
Patch for EWS

[Wenson Hsieh]

Created attachment 354184 [details]
Patch for landing

[Wenson Hsieh]

Created attachment 354190 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 354190 [details]
Patch for EWS

Clearing flags on attachment: 354190

Committed r237976: <https://trac.webkit.org/changeset/237976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.