Bug 89098 - Add methods to select between offsets in an editable field.
Summary: Add methods to select between offsets in an editable field.
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oli Lan
Depends on:
Blocks: 66687
  Show dependency treegraph
Reported: 2012-06-14 07:15 PDT by Oli Lan
Modified: 2012-06-21 17:49 PDT (History)
8 users (show)

See Also:

Patch (5.90 KB, patch)
2012-06-14 07:26 PDT, Oli Lan
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2012-06-14 07:43 PDT, Oli Lan
no flags Details | Formatted Diff | Diff
Patch (8.80 KB, patch)
2012-06-15 09:26 PDT, Oli Lan
no flags Details | Formatted Diff | Diff
Patch for landing (8.91 KB, patch)
2012-06-21 13:43 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oli Lan 2012-06-14 07:15:08 PDT
Add a method to select between offsets in an editable field.
Comment 1 Oli Lan 2012-06-14 07:26:05 PDT
Created attachment 147580 [details]
Comment 2 WebKit Review Bot 2012-06-14 07:29:47 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Oli Lan 2012-06-14 07:32:42 PDT
Hmm, it looks like the new html files in tests/data haven't been picked up in the patch.
Comment 4 Oli Lan 2012-06-14 07:43:10 PDT
Created attachment 147588 [details]
Comment 5 Oli Lan 2012-06-14 07:44:19 PDT
Patch 2 has the new test html files.
Comment 6 Adam Barth 2012-06-14 08:51:43 PDT
@rniwa: Would you be willing to review this patch?
Comment 7 Ryosuke Niwa 2012-06-14 10:38:04 PDT
Comment on attachment 147588 [details]

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.
Comment 8 Oli Lan 2012-06-15 08:18:54 PDT
Thanks for the review. I'll rework this to take into account all your comments.
Comment 9 Oli Lan 2012-06-15 09:26:41 PDT
Created attachment 147835 [details]
Comment 10 Adam Barth 2012-06-18 15:44:38 PDT
@rniwa: Are you happy with this patch now?
Comment 11 Ryosuke Niwa 2012-06-18 16:02:47 PDT
Comment on attachment 147835 [details]

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::
Comment 12 Adam Barth 2012-06-21 13:43:38 PDT
Created attachment 148878 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-06-21 17:49:42 PDT
Comment on attachment 148878 [details]
Patch for landing

Clearing flags on attachment: 148878

Committed r120985: <http://trac.webkit.org/changeset/120985>
Comment 14 WebKit Review Bot 2012-06-21 17:49:48 PDT
All reviewed patches have been landed.  Closing bug.