Bug 64133

Summary: Move selection related code from RenderTextControl to HTMLTextFormControlElement
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: FormsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, ap, darin, dglazkov, eric, inferno, kenneth, kling, mjs, tkent, tonikitoo, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 64210, 64212    
Bug Blocks: 47865, 60403, 64134    
Attachments:
Description Flags
Awesome patch
none
Update calls to setSelectionRange
none
resubmit for qt-ews
dglazkov: review+
Fixed the regression dglazkov: review+, rniwa: commit-queue+

Description Ryosuke Niwa 2011-07-07 16:27:55 PDT
RenderTextControl shouldn't be providing interface for selection.  We should move all selection-related member functions of RenderTextControl to HTMLTextFormControlElement.
Comment 1 Dimitri Glazkov (Google) 2011-07-07 16:30:01 PDT
This sounds awesome!
Comment 2 Ryosuke Niwa 2011-07-07 16:31:15 PDT
(In reply to comment #1)
> This sounds awesome!

It IS awesome :D
Comment 3 Ryosuke Niwa 2011-07-07 17:04:58 PDT
Created attachment 100052 [details]
Awesome patch
Comment 4 Early Warning System Bot 2011-07-07 17:13:49 PDT
Comment on attachment 100052 [details]
Awesome patch

Attachment 100052 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9003095
Comment 5 Ryosuke Niwa 2011-07-07 17:30:20 PDT
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.
Comment 6 Kenneth Rohde Christiansen 2011-07-08 01:56:40 PDT
(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));
Comment 7 Ryosuke Niwa 2011-07-08 08:16:38 PDT
(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?
Comment 8 Ryosuke Niwa 2011-07-08 11:04:00 PDT
Created attachment 100134 [details]
Update calls to setSelectionRange
Comment 9 Ryosuke Niwa 2011-07-08 11:19:38 PDT
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.
Comment 10 Ryosuke Niwa 2011-07-08 11:39:24 PDT
Created attachment 100140 [details]
resubmit for qt-ews
Comment 11 Ryosuke Niwa 2011-07-08 12:20:27 PDT
Ping reviewers because this patch is blocking further cleanups.
Comment 12 Ryosuke Niwa 2011-07-08 13:31:06 PDT
Committed r90662: <http://trac.webkit.org/changeset/90662>
Comment 13 Ryosuke Niwa 2011-07-09 18:57:47 PDT
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.
Comment 14 Dimitri Glazkov (Google) 2011-07-10 09:45:14 PDT
Comment on attachment 100219 [details]
Fixed the regression

What's up with the EWS bots?
Comment 15 Ryosuke Niwa 2011-07-11 10:55:32 PDT
Committed r90763: <http://trac.webkit.org/changeset/90763>