[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
Tue Jun 7 15:53:53 PDT 2011


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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org>  2011-06-07 15:53:53 PST ---
(From update of attachment 96299)
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.

-- 
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