Closed
Bug 614293
Opened 14 years ago
Closed 14 years ago
[HWKB] Regression: Typing in urlbar does not replace selection
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec2.0b3+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b3+ | --- |
People
(Reporter: mbrubeck, Assigned: mbrubeck)
References
Details
(Whiteboard: [HKB])
Attachments
(1 file, 2 obsolete files)
3.20 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce:
1. With the hardware keyboard open, tap the titlebar
2. Tap the urlbar again to select the text
3. Type a character on the hardware keyboard
Expected results: Text is replaced by the typed character.
Actual results: The typed character is appended or prepended to the text.
Assignee | ||
Updated•14 years ago
|
tracking-fennec: --- → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0b3+
Assignee | ||
Comment 1•14 years ago
|
||
It seems that GeckoSurfaceView::mEditable is getting out of sync with the focused text field, because nsWindow::OnIMETextChange is not called when the focus changes. Still investigating...
Updated•14 years ago
|
Assignee: nobody → mbrubeck
Assignee | ||
Comment 2•14 years ago
|
||
As mentioned above, mEditable was getting out of sync because we did not query for the current text and selection on focus. Also, after you type something, we notify using just the text up to the cursor position. That causes the Java code to forget about the rest of the text, causing problems with an incorrect "maxLen" in later calls to GeckoInputConnection::notifySelectionChange.
This patch solves the problem in a very simple way: Always pass the full text, and always update the text and selection when focus changes. I'm looking now to see if there smarter ways to do this.
Attachment #492793 -
Flags: feedback?(blassey.bugs)
![]() |
||
Updated•14 years ago
|
Whiteboard: [HKB]
Comment 3•14 years ago
|
||
Comment on attachment 492793 [details] [diff] [review]
patch
> NS_IMETHODIMP
> nsWindow::OnIMEFocusChange(PRBool aFocus)
> {
> ALOGIME("IME: OnIMEFocusChange: f=%d", aFocus);
>
>- AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_FOCUSCHANGE,
>- int(aFocus));
>+ if (aFocus) {
if (!aFocus)
return NS_OK;
>+ AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_FOCUSCHANGE,
>+ int(aFocus));
>+
>+ nsQueryContentEvent event(PR_TRUE, NS_QUERY_TEXT_CONTENT, this);
>+ InitEvent(event, nsnull);
>+ event.InitForQueryTextContent(0, PR_UINT32_MAX);
>+
>+ DispatchEvent(&event);
>+ if (!event.mSucceeded)
>+ return NS_OK;
>+
>+ AndroidBridge::NotifyIMEChange(event.mReply.mString.get(),
>+ event.mReply.mString.Length(),
>+ 0, 0, 0);
This takes a rather interesting path from here to mEditable. I suppose the reason why we require the whole thing to get sent is because the mEditable updating code in GeckoInputConnection.notifyTextChange isn't implemented quite right. If it did a replace instead of regenerating the Editable, things may work a bit better.
Attachment #492793 -
Flags: feedback?(blassey.bugs)
Assignee | ||
Comment 5•14 years ago
|
||
Functionally the same as the previous patch, but fixes the "if (aFocus)" check.
I'm still working on a patch that uses Editable.replace to avoid resending the whole text every time. It's not working yet and might be a little more involved.
Attachment #492793 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
This is the most I'm going to finish today. My attempts to use Editable.replace were complicated by the fact that our TextWatcher needs to distinguish changes made by the Android side from changes made by Gecko. Something needs to be reworked if we want to change the Editable without triggering infinite notification loops, and I'm not the person to do that today.
For now, this patch makes it more correct than before, and fixes the regressions.
Attachment #493092 -
Attachment is obsolete: true
Attachment #493115 -
Flags: review?(blassey.bugs)
Comment 7•14 years ago
|
||
Comment on attachment 493115 [details] [diff] [review]
patch v3
> mUpdateExtract.partialStartOffset = 0;
> mUpdateExtract.partialEndOffset = oldEnd;
>
> // Faster to not query for selection
> mUpdateExtract.selectionStart = newEnd;
> mUpdateExtract.selectionEnd = newEnd;
>
>- mUpdateExtract.text = text;
>+ mUpdateExtract.text = text.substring(0, newEnd);
> mUpdateExtract.startOffset = 0;
we discussed on irc that they're may be quirks in the android IME code about assuming that start is always 0. if that's true, please add a comment to that effect (I see the comment below, please at least reference it here)
Attachment #493115 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 8•14 years ago
|
||
Pushed with added comment:
http://hg.mozilla.org/mozilla-central/rev/8fc4da44d2b9
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 9•14 years ago
|
||
verified FIXED on build:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101129 Namoroka/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•