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

Sat Jun 4 17:14:46 PDT 2011

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 96026: proposed patch
https://bugs.webkit.org/attachment.cgi?id=96026&action=review

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

I'd say r- because there are quite few nits.

> Source/WebCore/ChangeLog:7
> +	   After setting text to empty string and removing children nodes
selection will be set to noselection. 
> +	   If this is the currently focused element update the focus appearance
which will restore the carret.
> +	   https://bugs.webkit.org/show_bug.cgi?id=62092

Missing bug title.  a change log entry should be (note there's a blank line
between bug url & description):
bug title
bug url

description

Also, typo: s/carret/caret/.

> Source/WebCore/html/HTMLElement.cpp:456
> +	       if (document()->focusedNode() == this)
> +		   updateFocusAppearance(true);

I'm not sure if this is the right fix.	Removing child nodes should have
invoked FrameSelection::nodeWillBeRemoved.  Why do we need to call
updateFocusAppearance here manually?

> LayoutTests/ChangeLog:7
> +

You should explain what kind of a test you're adding (blank lines before and
after the description).

> LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:1
> +<html>

Missing <!DOCTYPE html>.

> LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:22
> +    if (event.keyCode === 82) { //r

Is it significant that we check for this key code?  I'd rather always run the
test when we have a keydown event.

> LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:31
> +<body onload="test()">

I don't see any reason we have to wait load event.  You should just put script
in body so that you don't have to wait for load event.

> LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:33
> +<p>The test runs only under DumpRenderTree with eventSender; if you test by
hand the test result below will say FAIL.</p>

It seems like this test can be ran manually by pressing some key down in the
editable region below, and observing that the character is inserted.

> LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:34
> +<div id="it" style="-webkit-user-modify: read-write;-webkit-user-select:
text;cursor: text;" onKeyDown="resetIt(event)">text</div>

s/onKeyDown/onkeydown/.  Also, are these properties all necessary for
reproduction (especially -webkit-user-select: text and cursor: text;)?