Can focus but not type into content editable block that contains only non-editable content

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

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)

[Enrica Casucci]

Created attachment 43619 [details]
Proposed patch

Working on the test case. No change log yet.

[Enrica Casucci]

Created attachment 43818 [details]
Patch

[Adam Barth]

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

[Enrica Casucci]

Created attachment 44038 [details]
Patch2

Fixed line 327 in Position.cpp to have && at the beginning of the line.

[Adam Barth]

style-queue ran check-webkit-style on attachment 44038 [details] without any errors.

[Darin Adler]

Comment on attachment 44038 [details]
Patch2

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

[Enrica Casucci]

Committed revision 51522.
I've addressed all comments from Darin.

[Enrica Casucci]

*** Bug 20325 has been marked as a duplicate of this bug. ***

[Tony Chang]

*** Bug 19698 has been marked as a duplicate of this bug. ***