[webkit-reviews] review denied: [Bug 62092] setting innerText to an empty string on editable div loses focus : [Attachment 96534] 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 96534: proposed patch
https://bugs.webkit.org/attachment.cgi?id=96534&action=review

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

> Source/WebCore/dom/Position.cpp:227
> +	       *this = positionInParentBeforeNode(node);

Assigning to *this is very unusual in WebKit but I can't think of a better
alternative.

> Source/WebCore/dom/Position.h:104
> +    // Updates the anchor node if it is about to be deleted.

This comment repeats what the function name says.  Please remove.

> LayoutTests/editing/selection/5497643-expected.txt:12
> -ALERT: SUCCESS: The selection was cleared.
> -This tests to make sure that a selection inside a textarea is cleared when
the textarea is removed from the document. Not clearing it led to crashes.
> -
> -
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x576
> +	 RenderBlock {P} at (0,0) size 784x22
> +	   RenderText {#text} at (0,0) size 735x22
> +	     text run at (0,0) width 735: "This tests to make sure that a
selection is correctly updated when the textarea is removed from the document."

> +	 RenderBlock (anonymous) at (0,38) size 784x0
> +	   RenderText {#text} at (0,0) size 0x0
> +	   RenderText {#text} at (0,0) size 0x0
> +caret: position 0 of child 0 {DIV} of {#shadow-root} of document

This is not right.  render tree dump will be platform-specific.  We want to
avoid it as much as we can in editing because it doesn't tell us useful
information.

> LayoutTests/editing/selection/5497643.html:-5
> -if (window.layoutTestController)
> -    window.layoutTestController.dumpAsText();

Why can't this test be dump as text?

> LayoutTests/editing/selection/5497643.html:-12
> -if (window.getSelection().type != "None")
> -    alert("FAILURE: There shouldn't be a selection.")
> -else
> -    alert("SUCCESS: The selection was cleared.")

We should be checking that selection is correctly updated as the test claims.

> LayoutTests/editing/selection/character-data-mutation.html:73
> +	   return 'Removing text in ' + nodeName + ' containing the end point';
}, 0, 1);

Why is start offset 0?	I'd imagine it should be 1 because we're moving "ello"
from "hello" and the selection start is between "he" and "llo".  I think you
need to modify shouldRemovePositionAfterAdoptingTextReplacement now that
removing node will never clear selection. r- because of this.