Bug 31750

Summary: Can focus but not type into content editable block that contains only non-editable content
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Severity: Normal CC: abarth, adele, darin, michael.vm, rpaplin
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Description Flags
Repro test
Proposed patch
Patch2 darin: review+

Description Enrica Casucci 2009-11-20 15:18:10 PST
Created attachment 43618 [details]
Repro test

Can't type when the content-editable object has focus. This occurs when there is a DIV element with only a non-editable elemnt.
(see attached test case)
Comment 1 Enrica Casucci 2009-11-20 15:20:28 PST
Created attachment 43619 [details]
Proposed patch

Working on the test case. No change log yet.
Comment 2 Enrica Casucci 2009-11-24 17:55:02 PST
Created attachment 43818 [details]
Comment 3 Adam Barth 2009-11-30 12:39:45 PST
Attachment 43818 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebCore/rendering/RenderText.h
Done processing WebCore/editing/htmlediting.cpp
WebCore/dom/Position.cpp:327:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Done processing WebCore/dom/Position.cpp
Done processing WebCore/dom/PositionIterator.cpp
Done processing WebCore/dom/Position.h
Done processing WebCore/dom/PositionIterator.h
Done processing WebCore/rendering/RenderObject.cpp
Done processing WebCore/rendering/RenderText.cpp
Total errors found: 1
Comment 4 Enrica Casucci 2009-11-30 13:10:32 PST
Created attachment 44038 [details]

Fixed line 327 in Position.cpp to have && at the beginning of the line.
Comment 5 Adam Barth 2009-11-30 13:16:23 PST
style-queue ran check-webkit-style on attachment 44038 [details] without any errors.
Comment 6 Darin Adler 2009-11-30 14:19:13 PST
Comment on attachment 44038 [details]

It seems unnecessary to make atEditingBoundary be a member function on Position and PositionIterator. Can we instead just make those free functions?

> +// A position is considered at editing boundary if one of the following is true:"

Stray quote mark on the end of the line above.

> +// 1. it is the first position in the node and the next visually equivalent position
> +//    is non editable
> +// 2. it is the last position in the node and the previous visually equivalent position
> +//    is non editable
> +// 3. is is an editable position and both the next and previous visually equivalent
> +//    positions are both non editable

Typo "is is" here. I would put capital letters and periods on these three conditions.

> +    return (nextPosition.isNotNull() && !nextPosition.node()->isContentEditable())
> +           && (prevPosition.isNotNull() && !prevPosition.node()->isContentEditable());

I think this would read better without the parentheses. Also, the second line should be indented only 4 spaces, not 7 -- we don't try to line things up.

> +Position Position::upstream(EditingBoundaryCrossingRule upRule) const

I'd probably call this boundaryRule or crossingRule or even "rule" rather than upRule.

>          return Position();
> -
> +    
>      // iterate forward from there, looking for a qualified position

The patch has a lot of whitespace changes like the above. It would be better to not have these changes.

> +            bool lastPosition = (caretOffset == lastOffsetInNode(node()));
> +            Node* startNode = (lastPosition)? node()->childNode(caretOffset - 1): node()->childNode(caretOffset);

It should be formatted as "lastPosition ? a : b" rather than "(lastPosition)? a: b".

> +bool PositionIterator::atEditingBoundary() const
> +{
> +    if (!m_anchorNode)
> +        return false;
> +        
> +    return Position(*this).atEditingBoundary();
> +}

I don't think you need a special case for !m_anchorNode here. Do you?

>      // If this is a non-anonymous renderer, then it's simple.

This comment seems wrong now. It's not all that simple any more! Maybe we should say something more about it rather than "it's simple".

> -    if (Node* node = this->node())
> +    if (Node* node = this->node()) {
> +        if (!node->isContentEditable()) {
> +            // prefer a visually equivalent position that is editable

We use sentence format for this type of comment. Capitals and a period at the end.

> +            Position pos(node, offset);

It would be better to name this "position" rather than "pos".

> +    int len = textLength();

This should be unsigned rather than int. And should be named a word, like "length" rather than an abbreviation such as "len".

> +    const UChar* txt = characters();

Instead of "txt" I suggest either "characters" or "text" as the name for this local variable.

> +    for (int i = 0; i < len; i++)
> +        if (!style()->isCollapsibleWhiteSpace(txt[i]))
> +            return false;

WebKit coding style uses braces around a multi-line for statement like this one.

r=me -- all the comments are optional things
Comment 7 Enrica Casucci 2009-11-30 16:46:24 PST
Committed revision 51522.
I've addressed all comments from Darin.
Comment 8 Enrica Casucci 2009-11-30 16:48:23 PST
*** Bug 20325 has been marked as a duplicate of this bug. ***
Comment 9 Tony Chang 2010-03-23 17:27:43 PDT
*** Bug 19698 has been marked as a duplicate of this bug. ***