Bug 64133 – Move selection related code from RenderTextControl to HTMLTextFormControlElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

RenderTextControl shouldn't be providing interface for selection.  We should move all selection-related member functions of RenderTextControl to HTMLTextFormControlElement.

[Dimitri Glazkov (Google)]

This sounds awesome!

[Ryosuke Niwa]

(In reply to comment #1)
> This sounds awesome!

It IS awesome :D

[Ryosuke Niwa]

Created attachment 100052 [details]
Awesome patch

[Early Warning System Bot]

Comment on attachment 100052 [details]
Awesome patch

Attachment 100052 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9003095

[Ryosuke Niwa]

In qwebpage.cpp:

void QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)
{
...
        case QInputMethodEvent::Selection: {
            hasSelection = true;
            // A selection in the inputMethodEvent is always reflected in the visible text
            if (node)
                setSelectionRange(node, qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length)));

            if (!ev->preeditString().isEmpty())
                editor->setComposition(ev->preeditString(), underlines, qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length)));
            else {
...
    if (node && ev->replacementLength() > 0) {
        int cursorPos = frame->selection()->extent().offsetInContainerNode();
        int start = cursorPos + ev->replacementStart();
        setSelectionRange(node, start, start + ev->replacementLength());
        // Commit regardless of whether commitString is empty, to get rid of selection.
        editor->confirmComposition(ev->commitString());
    } else if (!ev->commitString().isEmpty()) {

Why is this code calling setSelectionRange?  This function only works on a text node inside an input element.  This can't be right.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> In qwebpage.cpp:
> 
> void QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev)
> {
> ...
>         case QInputMethodEvent::Selection: {
>             hasSelection = true;
>             // A selection in the inputMethodEvent is always reflected in the visible text
>             if (node)
>                 setSelectionRange(node, qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length)));
> 
>             if (!ev->preeditString().isEmpty())
>                 editor->setComposition(ev->preeditString(), underlines, qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length)));
>             else {


The documentation for Selection is the following:

--
If set, the edit cursor should be moved to the specified position in the editor text contents. In contrast with Cursor, this attribute does not work on the preedit text, but on the surrounding text. The cursor will be moved after the commit string has been committed, and the preedit string will be located at the new edit position. The start position specifies the new position and the length variable can be used to set a selection starting from that point. The value is unused.
--

The documentation is not totally clear for me, but I guess this is mostly for fullscreen IME's. The qMin/qMax are used because Qt defines selections as an anchor and a cursor. Meaning if you select from right to left, the right start position is the anchor and the left position is the cursor, meaning that anchor (a.start) > cursor (a.length)

> ...
>     if (node && ev->replacementLength() > 0) {
>         int cursorPos = frame->selection()->extent().offsetInContainerNode();
>         int start = cursorPos + ev->replacementStart();
>         setSelectionRange(node, start, start + ev->replacementLength());
>         // Commit regardless of whether commitString is empty, to get rid of selection.
>         editor->confirmComposition(ev->commitString());
>     } else if (!ev->commitString().isEmpty()) {
> 
> Why is this code calling setSelectionRange?  This function only works on a text node inside an input element.  This can't be right.

Yes, I believe that we should replace this with

RefPtr<Range> replacementRange = TextIterator::rangeFromLocationAndLength(scope, cursorPos + ev->replacementStart(), ev->replacementLength());
targetFrame->selection()->setSelection(VisibleSelection(replacementRange.get(), SEL_DEFAULT_AFFINITY));

[Ryosuke Niwa]

(In reply to comment #6)
> The documentation for Selection is the following:
> 
> --
> If set, the edit cursor should be moved to the specified position in the editor text contents. In contrast with Cursor, this attribute does not work on the preedit text, but on the surrounding text. The cursor will be moved after the commit string has been committed, and the preedit string will be located at the new edit position. The start position specifies the new position and the length variable can be used to set a selection starting from that point. The value is unused.

I don't think this is specific to RenderTextControl.

> The documentation is not totally clear for me, but I guess this is mostly for fullscreen IME's. The qMin/qMax are used because Qt defines selections as an anchor and a cursor. Meaning if you select from right to left, the right start position is the anchor and the left position is the cursor, meaning that anchor (a.start) > cursor (a.length)

Could you replace this code by
RefPtr<Range> range = TextIterator::rangeFromLocationAndLength(node, qMin(a.start, (a.start + a.length)), qMax(a.start, (a.start + a.length));
targetFrame->selection()->setSelection(VisibleSelection(range.get(), SEL_DEFAULT_AFFINITY));

?

> Yes, I believe that we should replace this with
> 
> RefPtr<Range> replacementRange = TextIterator::rangeFromLocationAndLength(scope, cursorPos + ev->replacementStart(), ev->replacementLength());
> targetFrame->selection()->setSelection(VisibleSelection(replacementRange.get(), SEL_DEFAULT_AFFINITY));

Great!  Could you do that please?

[Ryosuke Niwa]

Created attachment 100134 [details]
Update calls to setSelectionRange

[Ryosuke Niwa]

Kenneth has a patch to fix Qt dependency on setSelectionRange on https://bugs.webkit.org/show_bug.cgi?id=64172.  Unfortunately, nobody can test that patch for now so I'm updating the code to call HTMLTextFormControlElement::setSelectionRange instead in this patch.

[Ryosuke Niwa]

Created attachment 100140 [details]
resubmit for qt-ews

[Ryosuke Niwa]

Ping reviewers because this patch is blocking further cleanups.

[Ryosuke Niwa]

Committed r90662: <http://trac.webkit.org/changeset/90662>

[Ryosuke Niwa]

Created attachment 100219 [details]
Fixed the regression

The problem was the change in line 719/819 of HTMLFormControlElement.cpp:
-    RenderTextControl* renderTextControl = toRenderTextControl(renderer());
-    cacheSelection(renderTextControl->selectionStart(), renderTextControl->selectionEnd());
+    cacheSelection(selectionStart(), selectionEnd());

When the element isn't focused, selectionStart and selectionEnd can return m_cachedSelectionStart or m_cachedSelectionEnd.  I fixed this by adding computeSelectionStart and computeSelectiionEnd that do exactly what RenderText::selectionStart and RenderText::selectionEnd did.  Unfortunately, I could not come up with a layout test for this.

[Dimitri Glazkov (Google)]

Comment on attachment 100219 [details]
Fixed the regression

What's up with the EWS bots?

[Ryosuke Niwa]

Committed r90763: <http://trac.webkit.org/changeset/90763>