[webkit-reviews] review denied: [Bug 62092] setting innerText to an empty string on editable div loses focus : [Attachment 96299] proposed patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Una Sabovic <una.sabovic at palm.com>'s
request for review:
Bug 62092: setting innerText to an empty string on editable div loses focus
https://bugs.webkit.org/show_bug.cgi?id=62092

Attachment 96299: proposed patch
https://bugs.webkit.org/attachment.cgi?id=96299&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96299&action=review
I'm certain that new test belongs to LayoutTests/editing/selection instead of
fast/dom/HTMLElement.

> Source/WebCore/dom/Position.cpp:218
> +void Position::nodeWillBeRemoved(Node* node)

I'm not sure if the name nodeWillBeRemoved makes sense here.

>> Source/WebCore/dom/Position.h:105
>> +	void nodeWillBeRemoved(Node* node);
> 
> The parameter name "node" adds no information, so it should be removed. 
[readability/parameter_name] [5]

Please remove the argument name 'node'.

> Source/WebCore/editing/FrameSelection.cpp:284
> +	   // When endpoints are removed move the selection to valid anchor
nodes.

This comment repeats what code says. I don't think we should be adding this
comment. In fact, the function name should convey this if any.

> Source/WebCore/editing/FrameSelection.cpp:297
> +	   if (start.isNotNull() && end.isNotNull()) {
> +	       if (m_selection.isBaseFirst())
> +		   m_selection.setWithoutValidation(start, end);
> +	       else
> +		   m_selection.setWithoutValidation(end, start);
> +	   } else {

I'm pretty sure you'd have to clear render tree selection in this case.  See
comment below that starts with "If we did nothing here"

> LayoutTests/editing/execCommand/4920488-expected.txt:13
> -| ""
> +| "<#selection-anchor>"
>  | <a>
>  |   href="http://www.google.com/"
> +|   <#selection-focus>

This seems wrong. Selection-anchor seems to be at a non-canonicalized position.
 Then again, we're setting position without validation.  I'm not sure how we
can fix this.

> LayoutTests/editing/selection/character-data-mutation-expected.txt:6
> -PASS: Removing the parent of startContainer cleared selection
> -PASS: Replacing nodeValue of startContainer cleared selection
> -PASS: Replacing nodeValue of endContainer cleared selection
> +FAIL: Removing the parent of startContainer did not clear selection
> +FAIL: Replacing nodeValue of startContainer did not clear selection
> +FAIL: Replacing nodeValue of endContainer did not clear selection

These tests are failing.  Why is it okay for these tests to fail?  If anything
we should be modifying the test so that they pass.  But I'd really like to know
why these tests are failing and why new behavior is correct. r- because of
this.

> LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:13
> +    if (window.layoutTestController)
> +	   layoutTestController.dumpAsText();
> +

There's no point in indenting like this.  Please outdent.