[Webkit-unassigned] [Bug 62092] setting innerText to an empty string on editable div loses focus

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jun 4 17:14:47 PDT 2011


https://bugs.webkit.org/show_bug.cgi?id=62092


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #96026|review?                     |review-
               Flag|                            |




--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org>  2011-06-04 17:14:47 PST ---
(From update of attachment 96026)
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;)?

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



More information about the webkit-unassigned mailing list