Bug 53409

Summary:

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

Product:

WebKit

Reporter:

Benjamin (Ben) Kalman <kalman>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, adele, enrica, eric, justin.garcia, leviw, leviw, mitz, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

56135

Bug Blocks:

52642

Attachments:

Description	Flags
Test case	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	rniwa: review+

Benjamin (Ben) Kalman

Reported 2011-01-30 19:52:12 PST

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

Attachments
Test case (540 bytes, text/html) 2011-01-30 19:52 PST, Benjamin (Ben) Kalman	no flags	Details
Patch (714 bytes, patch) 2011-02-10 17:56 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (45.78 KB, patch) 2011-02-15 13:07 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (14.28 KB, patch) 2011-03-04 15:55 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (13.92 KB, patch) 2011-03-08 18:13 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (16.09 KB, patch) 2011-03-10 12:48 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (16.66 KB, patch) 2011-03-10 13:59 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (16.39 KB, patch) 2011-03-10 15:13 PST, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (17.30 KB, patch) 2011-03-10 15:22 PST, Levi Weintraub	rniwa: review+	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Benjamin (Ben) Kalman

Comment 1 2011-01-30 19:52:43 PST

Created attachment 80614 [details] Test case

Levi Weintraub

Comment 2 2011-02-10 17:03:08 PST

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

Comment 3 2011-02-10 17:56:10 PST

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

Comment 4 2011-02-14 11:11:36 PST

> 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

Comment 5 2011-02-14 12:16:51 PST

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

Comment 6 2011-02-15 13:07:25 PST

Created attachment 82512 [details] Patch

Levi Weintraub

Comment 7 2011-02-15 13:24:01 PST

(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 8 2011-03-03 00:59:10 PST

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 hello world WebKit. 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 non-editable span. > + 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 9 2011-03-04 14:35:40 PST

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 hello world WebKit. 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

Comment 10 2011-03-04 15:55:34 PST

Created attachment 84819 [details] Patch

Levi Weintraub

Comment 11 2011-03-08 15:37:47 PST

Review please?

Ryosuke Niwa

Comment 12 2011-03-08 15:47:00 PST

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 13 2011-03-08 16:25:21 PST

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

Comment 14 2011-03-08 18:13:17 PST

Created attachment 85120 [details] Patch

Ryosuke Niwa

Comment 15 2011-03-09 12:37:25 PST

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 16 2011-03-09 15:10:11 PST

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

Comment 17 2011-03-10 12:48:12 PST

Created attachment 85376 [details] Patch

Levi Weintraub

Comment 18 2011-03-10 13:59:59 PST

Created attachment 85388 [details] Patch

Ryosuke Niwa

Comment 19 2011-03-10 14:55:48 PST

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

Comment 20 2011-03-10 15:13:55 PST

Created attachment 85394 [details] Patch

Levi Weintraub

Comment 21 2011-03-10 15:22:52 PST

Created attachment 85395 [details] Patch

Ryosuke Niwa

Comment 22 2011-03-10 15:26:16 PST

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

Comment 23 2011-03-10 15:54:15 PST

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

WebKit Review Bot

Comment 24 2011-03-10 17:33:32 PST

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

Note You need to log in before you can comment on or make changes to this bug.