Bug 35933

Summary: [Qt] [Symbian] Can not backward select (highlight) text using virtual keyboard
Product: WebKit Reporter: Shen Yi <shenyi2006>
Component: WebKit QtAssignee: Shen Yi <shenyi2006>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hausmann, laszlo.gombos, shenyi2006
Priority: P2 Keywords: PlatformOnly, Qt
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Attachments:
Description Flags
proposal fix for bug 35933
none
proposal patch
none
patch for bug35933
hausmann: review-
new patch with the unit test none

Description Shen Yi 2010-03-09 11:20:37 PST
By using the virtual keyboard on a symbian device, the user can not select texts from right to left (the cursor position is before the anchor position);
Comment 1 Shen Yi 2010-03-09 11:37:45 PST
Created attachment 50334 [details]
proposal fix for bug 35933
Comment 2 Shen Yi 2010-03-09 11:55:19 PST
This issue is Qt only, and the root cause is that webcore assumes the selection's start position is always before the end position.

In RenderTextControl.cpp;

void RenderTextControl::setSelectionEnd(int end)
{
    setSelectionRange(min(end, selectionStart()), end);
}

The setSelectionEnd(int) function was called from QWebPagePrivate::inputMethodEvent(QInputMethodEvent*) at qwebpage.cpp

  case QInputMethodEvent::Selection: {
     if (renderTextControl) {
         renderTextControl->setSelectionStart(a.start);
         renderTextControl->setSelectionEnd(a.start + a.length);
     }
     break;
  }

However, in above code, the a.length, which is from QcoeFepInputContext, is negative when doing backward selection:

void QCoeFepInputContext::SetCursorSelectionForFepL(const TCursorSelection&) {
    int pos = aCursorSelection.iAnchorPos;
    int length = aCursorSelection.iCursorPos - pos;
    QList<QInputMethodEvent::Attribute> attributes;
    attributes << QInputMethodEvent::Attribute(QInputMethodEvent::Selection, pos, length, QVariant());
}

It can explain why the backward selection doesn't work, that is,

when start position is larger than end position,

setSelectionRange(min(end, selectionStart()), end);
  equals
setSelectionRange(end, end);

The patch I attached would check/adjust the start & end position before calling the setSelectionStart & setSelectionEnd. I have tested it on both s60 emulator & target.
Comment 3 Shen Yi 2010-03-16 08:20:20 PDT
Created attachment 50795 [details]
proposal patch
Comment 4 Shen Yi 2010-03-16 11:20:07 PDT
Created attachment 50812 [details]
patch for bug35933
Comment 5 Simon Hausmann 2010-03-19 00:57:27 PDT
Comment on attachment 50812 [details]
patch for bug35933

I think the patch is fine, but please make a unit test for this. See the inputMethods() unit tests in WebKit/qt/tests/qwebpage.

You could test this by sending a synthetic input method event with a selection set. After sending the event you could verify that the selection is set correctly.
Comment 6 Shen Yi 2010-03-19 08:54:21 PDT
Created attachment 51157 [details]
new patch with the unit test

The new patch contains the unit test for qwebpage.
Comment 7 WebKit Commit Bot 2010-03-22 08:10:04 PDT
Comment on attachment 51157 [details]
new patch with the unit test

Clearing flags on attachment: 51157

Committed r56334: <http://trac.webkit.org/changeset/56334>
Comment 8 WebKit Commit Bot 2010-03-22 08:10:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Simon Hausmann 2010-03-29 04:02:58 PDT
Cherry-picked into qtwebkit-4.6 with commit edc778e0b452640d8e05c4a88903c18b6afe20ec