Summary: | Move selection related code from RenderTextControl to HTMLTextFormControlElement | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | Forms | Assignee: | 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
Ryosuke Niwa
2011-07-07 16:27:55 PDT
This sounds awesome! (In reply to comment #1) > This sounds awesome! It IS awesome :D Created attachment 100052 [details]
Awesome patch
Comment on attachment 100052 [details] Awesome patch Attachment 100052 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9003095 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. (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)); (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? Created attachment 100134 [details]
Update calls to setSelectionRange
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. Created attachment 100140 [details]
resubmit for qt-ews
Ping reviewers because this patch is blocking further cleanups. Committed r90662: <http://trac.webkit.org/changeset/90662> 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 on attachment 100219 [details]
Fixed the regression
What's up with the EWS bots?
Committed r90763: <http://trac.webkit.org/changeset/90763> |