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

Thu Mar 3 00:59:10 PST 2011

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #82512|review?                     |review-
               Flag|                            |

--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org>  2011-03-03 00:59:10 PST ---
(From update of attachment 82512)
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.

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