[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 #9 from Levi Weintraub <leviw at chromium.org>  2011-03-04 14:35:40 PST ---
(From update of attachment 82512)
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 :)

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