[Webkit-unassigned] [Bug 53409] InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org>  2011-03-09 12:37:25 PST ---
(From update of attachment 85120)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.