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)

All
Android
defect
Not set
normal

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)

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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b3+
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...
Assignee: nobody → mbrubeck
Attached patch patch (obsolete) — Splinter Review
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)
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)
Attached patch patch v2 (obsolete) — Splinter Review
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
Attached patch patch v3Splinter Review
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 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+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
verified FIXED on build: Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101129 Namoroka/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: