Bug 89098 – Add methods to select between offsets in an editable field.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oli Lan]

Add a method to select between offsets in an editable field.

[Oli Lan]

Created attachment 147580 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Oli Lan]

Hmm, it looks like the new html files in tests/data haven't been picked up in the patch.

[Oli Lan]

Created attachment 147588 [details]
Patch

[Oli Lan]

Patch 2 has the new test html files.

[Adam Barth]

@rniwa: Would you be willing to review this patch?

[Ryosuke Niwa]

Comment on attachment 147588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147588&action=review

> Source/WebKit/chromium/ChangeLog:15
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description bug after the bug url.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2098
> +    Node* node = focusedWebCoreNode();
> +    if (node && node->isElementNode()) {
> +        Element* elementNode = toElement(node);
> +        if (elementNode->isTextFormControl()) {
> +            HTMLTextFormControlElement* formElement = toTextFormControl(elementNode);
> +            formElement->setSelectionRange(start, end);
> +            return;
> +        }
> +    }

This code is redundant.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2102
> +    // For contenteditable nodes the focusedWebCoreNode may not be what we want (the editable
> +    // node may not be focused even during editing). Also, we need to go to the ancestor node
> +    // to apply the offsets.

This comment is redundant for anyone who understands editing code.
I'd suggest we move all of this code to Editor.cpp so that we don't have to add this comment.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2106
> +    node = frame->selection()->start().containerNode();

Why do you want to use start's container node here? That sounds completely arbitrary except when both start & end are in the same node.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2107
> +    if (node && node->shouldUseInputMethod()) {

If anything, what you want to do is to obtain the root editable element and use TextIterator's static functions to obtain range given start & length offsets from the beginning of the element.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2110
> +        Position startPosition(node, start, Position::PositionIsOffsetInAnchor);
> +        Position endPosition(node, end, Position::PositionIsOffsetInAnchor);

What guarantees that start and end are in the same node? If this is specific to composition, then the method name should reflect that.
Also, I'd prefer moving this code to Editor.h/cpp next to getCompositionSelection.

[Oli Lan]

Thanks for the review. I'll rework this to take into account all your comments.

[Oli Lan]

Created attachment 147835 [details]
Patch

[Adam Barth]

@rniwa: Are you happy with this patch now?

[Ryosuke Niwa]

Comment on attachment 147835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147835&action=review

r=me provided the following nits are addressed.

> Source/WebCore/editing/Editor.cpp:2352
> +    Element* rootEditable = m_frame->selection()->rootEditableElement();

Nit: root editable what? "editable" is an adjective, not a noun. I'd prefer spelling out rootEditableElement. Element is fine as well.

> Source/WebCore/editing/Editor.cpp:2360
> +    return m_frame->selection()->setSelectedRange(range.get(), WebCore::VP_DEFAULT_AFFINITY, false);

Nit: No need for WebCore::

[Adam Barth]

Created attachment 148878 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148878 [details]
Patch for landing

Clearing flags on attachment: 148878

Committed r120985: <http://trac.webkit.org/changeset/120985>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.