[webkit-reviews] review denied: [Bug 33226] Indent inside li with br's changes order and creates new lists : [Attachment 208513] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Sudarshan C P
<cp.sudarshan at gmail.com>'s request for review:
Bug 33226: Indent inside li with br's changes order and creates new lists
https://bugs.webkit.org/show_bug.cgi?id=33226

Attachment 208513: Patch
https://bugs.webkit.org/attachment.cgi?id=208513&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208513&action=review


> Source/WebCore/ChangeLog:15
> +	   Place the cursor before or after the <br> tag,inside the <li>
element
> +	   and apply the indent command ,applied element always moves to
beginning
> +	   of the list item.
> +	   <br> tag is present inside the <li> tag , text range is split based
on
> +	   the <br> element present in the <li> element, When we apply	indent
to
> +	   particular text range of the <li> element ,we need to split the node
based
> +	   on the previous and	next element , to place the newly created
element of 
> +	   <ul><li>Text</li></ul>,so added new logic to handle this scenario.

Comma is all messed up in this description.  Each comma should appear
immediately after the preceding letters and should be followed by exactly one
space.
e.g. hello, world.  Not hello , world or hello,world.
Also, I don't really get what you mean in the second paragraph. Please revise
it.

> Source/WebCore/ChangeLog:18
> +	   Test:
editing/execCommand/indent_on_li_element_containing_br_text.html

Please use - instead of _ to separate words.

> Source/WebCore/editing/IndentOutdentCommand.cpp:-73
> -    // FIXME: previousElementSibling does not ignore non-rendered content
like <span></span>.  Should we?

Why are we removing this comment?

> Source/WebCore/editing/IndentOutdentCommand.cpp:74
> +    Node* beforeLastNode = lastNodeInSelectedParagraph->previousSibling();
> +    Node* afterLastNode = lastNodeInSelectedParagraph->nextSibling();

Why are we getting previous and next siblings?	They may not be even visible.
e.g. collapsed whitespaces.

> Source/WebCore/editing/IndentOutdentCommand.cpp:78
> +    if (!beforeLastNode && afterLastNode)
> +	   insertNodeBefore(newList, selectedListItem);

I don't understand why we need to adjust where we insert a list item based on
the content of li.
It looks like you're simply trying to work around a bug in
moveParagraphWithClones,

> Source/WebCore/editing/IndentOutdentCommand.cpp:80
> +	   splitElement(selectedListItem, lastNodeInSelectedParagraph);

lastNodeInSelectedParagraph is not necessary a child of selectedListItem. e.g.
<li><span><br></span></li>
r- because of this wrong assumption.