Bug 53409 – InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop

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

[Benjamin (Ben) Kalman]

See attachment - I suggest running in chrome because it will enter an infinite loop.  Or maybe safari handles that well, too?

This might be related to webkit.org/b/52846

[Benjamin (Ben) Kalman]

Created attachment 80614 [details]
Test case

[Levi Weintraub]

Amongst potentially other problems, the visible_units function endOfParagraph is returning a position before the non-editable span, which is *not* the end of the paragraph. Also, this asserts within moveParagraphs in a debug build...

[Levi Weintraub]

Created attachment 82082 [details]
Patch

Actually that seems to be the only problem, though I'm running the layout tests to confirm. No more infinite loop :)

[Levi Weintraub]

>  though I'm running the layout tests to confirm...

This breaks the layout test format-block-contenteditable-false. I'll come up with a fix.

[Levi Weintraub]

Functions such as FormatBlock rely on start/endOfParagraph not traversing editable content when requested (makes sense, huh). With my fix, the non-editable content is still subject to block-level changes like putting things into a list.

I did a quick check to see how this works in FF and IE9, but found some strange results:

- IE9 seems to completely disrespect contentEditable=false in a contentEditable=true context.
- FF will apply a formatting like italic to a block that is fully selected, even if contains non-editable content, but the formatblock execCommand will pull out only the editable content to nest inside the created block.

The original code is more true to its name, as it bails immediately when a node changes editability and its set to CannotCrossEditingBoundary. I'd love to have some consensus on how we expect non-editable content to be handled (e.g. move into lists and new blocks but always preserve style?), but in the meantime I don't think my original patch is the way to go. Instead, there's clearly a problem with the cleanupAfterDeletion function, as it is responsible in the assert that occurs when running this in debug builds.

[Levi Weintraub]

Created attachment 82512 [details]
Patch

[Levi Weintraub]

(In reply to comment #6)
> Created an attachment (id=82512) [details]
> Patch

There were actually a number of troublesome things going on, specifically dealing with InsertListCommand's destination placeholder being removed in moveParagraphs, and in the actual paragraph iteration code found in InsertListCommand::doApply().

[Ryosuke Niwa]

Comment on attachment 82512 [details]
Patch

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

The change looks sane but I'd like to see some brush ups.

> Source/WebCore/editing/InsertListCommand.cpp:181
> +                } while (nextStart.isNotNull() && nextStart != startOfLastParagraph && nextStart == nextEnd
> +                         && (endOfParagraph(nextEnd, CanCrossEditingBoundary) != startOfParagraph(nextStart, CanCrossEditingBoundary)

We should probably put all these condition into some inline function with a descriptive name.

> Source/WebCore/editing/InsertListCommand.cpp:182
> +                         || nextStart.deepEquivalent().anchorType() == Position::PositionIsOffsetInAnchor));

I don't understand why we need this condition.

> Source/WebCore/editing/InsertListCommand.cpp:394
> -    moveParagraph(start, end, positionBeforeNode(placeholder.get()), true);
> +    bool preserveSelection = true;
> +    moveParagraph(start, end, positionBeforeNode(placeholder.get()), preserveSelection);

left over?

> Source/WebCore/editing/visible_units.cpp:762
> +            while (n && n->isContentEditable() != startNode->isContentEditable())
> +                n = n->traversePreviousNodePostOrder(startBlock);

This can move from one editable region to another that don't share the highest editable root.  e.g. WebKit to hello in <span contentditable>hello</span> world <span contenteditale>WebKit</span>.  Is that intended behavior?  It seems odd that "skip over" means crossing arbitrary editing boundaries.

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:1
> +<div contenteditable="true" id="test" style="padding: 1em;">

You should put <!DOCTYPE html> here.

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:3
> +  Editable paragraph containing a <span contenteditable="false" style="background-color: LightGray;">non-editable</span> span.<br />
> +  Another editable paragraph.

xml style br?  Is that required?

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:17
> +  function insertList() {
> +      document.execCommand('insertunorderedlist', false, '');
> +  }
> +
> +  var s = window.getSelection();
> +  var div = document.getElementById("test");
> +  s.setPosition(div.childNodes[0], 10);
> +  s.modify("extend", "forward", "line");
> +  
> +  if (window.layoutTestController)
> +      insertList();

Nit: We don't normally indent script like this.  Please outdent them.

I don't think you have enough descriptions on this test.  When I just look at this test, I don't know what is the correct behavior.  You should probably explain that.  And I highly doubt that this test requires pixel results.  You should probably call dumpAsText here so that we can share the expected results across platforms.

[Levi Weintraub]

Comment on attachment 82512 [details]
Patch

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

>> Source/WebCore/editing/InsertListCommand.cpp:181
>> +                         && (endOfParagraph(nextEnd, CanCrossEditingBoundary) != startOfParagraph(nextStart, CanCrossEditingBoundary)
> 
> We should probably put all these condition into some inline function with a descriptive name.

Done!

>> Source/WebCore/editing/InsertListCommand.cpp:182
>> +                         || nextStart.deepEquivalent().anchorType() == Position::PositionIsOffsetInAnchor));
> 
> I don't understand why we need this condition.

I found a better way to represent this. I was trying to fix a case where we ended up with an extra list item due to how a position at the end of a root editable element directly following non-editable content was handled.

>> Source/WebCore/editing/InsertListCommand.cpp:394
>> +    moveParagraph(start, end, positionBeforeNode(placeholder.get()), preserveSelection);
> 
> left over?

This is a stylistic change away from magic boolean values. It's a pet peeve, but I can remove it.

>> Source/WebCore/editing/visible_units.cpp:762
>> +                n = n->traversePreviousNodePostOrder(startBlock);
> 
> This can move from one editable region to another that don't share the highest editable root.  e.g. WebKit to hello in <span contentditable>hello</span> world <span contenteditale>WebKit</span>.  Is that intended behavior?  It seems odd that "skip over" means crossing arbitrary editing boundaries.

Good catch. I'm now checking to make sure that doesn't happen!

>> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:3
>> +  Another editable paragraph.
> 
> xml style br?  Is that required?

Nope :)

>> LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html:17
>> +      insertList();
> 
> Nit: We don't normally indent script like this.  Please outdent them.
> 
> I don't think you have enough descriptions on this test.  When I just look at this test, I don't know what is the correct behavior.  You should probably explain that.  And I highly doubt that this test requires pixel results.  You should probably call dumpAsText here so that we can share the expected results across platforms.

No problem :)

[Levi Weintraub]

Created attachment 84819 [details]
Patch

[Levi Weintraub]

Review please?

[Ryosuke Niwa]

Comment on attachment 84819 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:175
> +                VisiblePosition nextStart(startOfNextParagraph(endingSelection().visibleStart()));

I think we normally use operator= instead of copy constructor.

> Source/WebCore/editing/InsertListCommand.cpp:393
> -    moveParagraph(start, end, positionBeforeNode(placeholder.get()), true);
> +    bool preserveSelection = true;
> +    moveParagraph(start, end, positionBeforeNode(placeholder.get()), preserveSelection);

I don't like this change since we're not modifying this function call in any other way.  I'd rather replace the boolean argument by an enum in a separate patch.

> Source/WebCore/editing/htmlediting.h:176
> +bool shouldSkipParagraphForIteration(const VisiblePosition& paragraphStart);

Is there a plan to use this function in other places?  If not, I'd prefer to put in InsertListCommand.cpp to avoid polluting the namespace.

> Source/WebCore/editing/visible_units.cpp:883
>  VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
>  {
> -    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition));
> +    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition, CanSkipOverEditingBoundary));

Did you really check that all call sites expect this function to cross the editing boundary?

[Levi Weintraub]

Comment on attachment 84819 [details]
Patch

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

>> Source/WebCore/editing/InsertListCommand.cpp:393
>> +    moveParagraph(start, end, positionBeforeNode(placeholder.get()), preserveSelection);
> 
> I don't like this change since we're not modifying this function call in any other way.  I'd rather replace the boolean argument by an enum in a separate patch.

Okay.

>> Source/WebCore/editing/htmlediting.h:176
>> +bool shouldSkipParagraphForIteration(const VisiblePosition& paragraphStart);
> 
> Is there a plan to use this function in other places?  If not, I'd prefer to put in InsertListCommand.cpp to avoid polluting the namespace.

We talked and agreed this needs a better name more than anything.

>> Source/WebCore/editing/visible_units.cpp:883
>> +    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition, CanSkipOverEditingBoundary));
> 
> Did you really check that all call sites expect this function to cross the editing boundary?

This function is only used in InsertListCommand and TextCheckingHelper, which we also wouldn't want to get caught up while iterating paragraphs by a non-editable node.

[Levi Weintraub]

Created attachment 85120 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85120 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:952
> -    VisiblePosition beforeParagraph = startOfParagraphToMove.previous();
> -    VisiblePosition afterParagraph(endOfParagraphToMove.next());
> +    bool stayInEditableContent = true;
> +    VisiblePosition beforeParagraph = startOfParagraphToMove.previous(stayInEditableContent);
> +    VisiblePosition afterParagraph(endOfParagraphToMove.next(stayInEditableContent));

As I mentioned earlier, we should either change the argument type to enum or not do this change.

> Source/WebCore/editing/InsertListCommand.cpp:69
> +    return start == paragraphEnd && (!start.deepEquivalent().upstream(CanCrossEditingBoundary).anchorNode()->isContentEditable()

Why are we comparing with the end of paragraph? I'm also skeptical about the correctness of anchorNode() here.  Shouldn't it be containerNode?  Why do we care that node before/after the position is editable?

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt:5
> +Editable paragraph containing a 
> + span in the middle.
> +   Another editable paragraph.
> +non-editable

Why do we have 4 paragraphs here?  Shouldn't we just have 2 paragraphs?  If we're only concerned with the crash and ignoring the correctness of output, then we should say that this test is testing that WebKit does not crash, and we should hide #test and print "PASS" indiscriminately.

[Levi Weintraub]

Comment on attachment 85120 [details]
Patch

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

>> Source/WebCore/editing/CompositeEditCommand.cpp:952
>> +    VisiblePosition afterParagraph(endOfParagraphToMove.next(stayInEditableContent));
> 
> As I mentioned earlier, we should either change the argument type to enum or not do this change.

This isn't the same change you mentioned earlier, I reverted that one. The default value for both of these is "false" not "true" (hence the change) and the added boolean is for readability. (See thread "Bools are strictly worse than enums" on WebKit Dev for my rationale). If you'd rather see a magic value, I'm not dogmatic about this.

>> Source/WebCore/editing/InsertListCommand.cpp:69
>> +    return start == paragraphEnd && (!start.deepEquivalent().upstream(CanCrossEditingBoundary).anchorNode()->isContentEditable()
> 
> Why are we comparing with the end of paragraph? I'm also skeptical about the correctness of anchorNode() here.  Shouldn't it be containerNode?  Why do we care that node before/after the position is editable?

If the start and end of the paragraph are the same when staying within editable boundaries, but not when skipping over them, we're at a caret position we wish to ignore.

Since we're looking at content with mixed editability, it would be feasible for containerNode's editability to differ from the anchorNode's for before/after positions.

>> LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt:5
>> +non-editable
> 
> Why do we have 4 paragraphs here?  Shouldn't we just have 2 paragraphs?  If we're only concerned with the crash and ignoring the correctness of output, then we should say that this test is testing that WebKit does not crash, and we should hide #test and print "PASS" indiscriminately.

We're matching FFX by treating regions of editable content in the same logical paragraph with non-editable content as different paragraphs. With that in mind, we should care about the output.

[Levi Weintraub]

Created attachment 85376 [details]
Patch

[Levi Weintraub]

Created attachment 85388 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85388 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Fixing broken handling of mixed-editability content for InsertListCommand.

More descriptions?

> Source/WebCore/editing/visible_units.cpp:826
>      Node *node = startNode;

Could you also fix this declaration?  (Node* instead of Node *)

> Source/WebCore/editing/visible_units.cpp:884
> +// FIXME: isStartOfParagraph(startOfNextParagraph(pos)) is not always true

Do we have a bug filed for this?  If not, please file one.

> LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt:4
> +Editable paragraph containing a non-editable in the middle
> +Another editable paragraph.
> +Insert List

I think we want to use dump as markup here because we'd certainly would like to see what kind of markup we're generating here, right?

[Levi Weintraub]

Created attachment 85394 [details]
Patch

[Levi Weintraub]

Created attachment 85395 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85395 [details]
Patch

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

> Source/WebCore/editing/InsertListCommand.cpp:379
> +        // the insertion.

You should just put "the insertion" in the previous line.

[Levi Weintraub]

Committed r80780: <http://trac.webkit.org/changeset/80780>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80780 might have broken GTK Linux 32-bit Release