Closed Bug 687717 Opened 8 years ago Closed 8 years ago

mobile.twitter.com->When writing into tweet box with IME, remaining characters of tweet isn't decreased

Categories

(Firefox for Android :: Keyboards and IME, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED WORKSFORME
Firefox 14
Tracking Status
firefox7 --- affected
firefox8 --- affected
firefox9 --- affected
firefox10 --- affected
blocking-fennec1.0 --- +

People

(Reporter: webqa.utest, Assigned: cpeterson)

References

(Blocks 1 open bug, )

Details

(Whiteboard: uTest-beta, [website-compatibility], [short-list],VKB)

Attachments

(2 files, 2 obsolete files)

To reproduce:
1. Goto https://mobile.twitter.com both in Firefox and default browser
2. Login with valid email/username and password
3. Input tweet characters into tweet box, and observe tweet length 140 is not decreased in Firefox but decrease in defualt browser.
(please see attached screenshot)
Environment Information:

Device Maker / Manufacturer : HTC
Device Model : 001HT
Device OS : Android version 2.3.3
Device Carrier : Softbank
FireFox 7 beta browser build id : Mozilla/5.0 (Android; Linux armv7l; rv:7.0) Gecko/20110902 Firefox/7.0 Fennec/7.0
Was the term "Webkit" found in the websites source code? : No
OS: Windows 7 → Android
Hardware: x86_64 → All
Whiteboard: uTest-beta → uTest-beta, [website-compatibility]
Let's figure out if this is a fennec issue or an issue on Twitter's end.
Version: Firefox 7 → Trunk
Probably caused by bug 630576.
Depends on: 630576
Whiteboard: uTest-beta, [website-compatibility] → uTest-beta, [website-compatibility][short-list]
Whiteboard: uTest-beta, [website-compatibility][short-list] → uTest-beta, [website-compatibility], [short-list]
I can reproduce this with Native Fennec's VKB. HKB works correctly.
Component: General → IME
Product: Fennec → Fennec Native
QA Contact: general → ime
Whiteboard: uTest-beta, [website-compatibility], [short-list] → uTest-beta, [website-compatibility], [short-list],VKB
The character counter does update after you press RETURN on the VKB. GeckoInputConnection is not sending individual key events when composing a string.
blocking-fennec1.0: --- → ?
Assignee: nobody → cpeterson
does desktop have the same behavior with IME input?
Desktop IME has the same problem.
Presumably this also affects B2G.

Masayuki, can you see if you can track this down?
Assignee: cpeterson → nobody
blocking-fennec1.0: ? → +
Component: IME → Widget
Product: Fennec Native → Core
QA Contact: ime → general
Summary: mobile.twitter.com->While input into tweet box remaining characters of tweet isn't decreased in FF → mobile.twitter.com->When writing into tweet box with IME, remaining characters of tweet isn't decreased
I am moving this bug back to Fennec:IME because I think I saw a different Mac problem.

With the Mac's built-in IME, the character count DOES decrease correctly after committing the composition string. The bug I saw was that the character counter does not decrease when inserting characters using the "Special Characters" window. I just opened bug 744545 to track that problem.

I believe this bug is caused by Fennec is not sending keydown or keyup events to Gecko. If I hack Fennec to send keydown/keyup events, then the character counter decreases as expected.
Assignee: nobody → cpeterson
Component: Widget → IME
Product: Core → Fennec Native
QA Contact: general → ime
btw, the "Special Characters" problem on Mac (bug 744545) was a content problem that is also reproducible on Chrome and Safari.
Status: NEW → ASSIGNED
Attached patch bug-687717-send-key-events.patch (obsolete) — Splinter Review
Send key events rather than committing single-character composition strings.

Based on alexp's WIP patch from bug 630576, this patch will send Gecko keydown/keyup events instead of composition events if GeckoInputConnection is committing a single character and did not have an active composition string. The GTK2 widget uses a similar strategy.

This patch only sends key events for Latin characters entered on the VKB. This patch DOES NOT fix multibyte characters entered using an IME like Simeji. I plan to fix that problem in bug 737658.
Attachment #614611 - Flags: review?(masayuki)
patch v2 adds `if (DEBUG)` for verbose log messages and removes some unnecessary `static` modifiers.
Attachment #614611 - Attachment is obsolete: true
Attachment #614611 - Flags: review?(masayuki)
Attachment #614633 - Flags: review?(masayuki)
Comment on attachment 614633 [details] [diff] [review]
bug-687717-send-key-events-v2.patch

First, I'm not familiar with Java. So, please request to review to a good another reviewer later.

>     @Override
>     public boolean commitText(CharSequence text, int newCursorPosition) {
>+        mCommittingText = true;
>         replaceText(text, newCursorPosition, false);
>+        mCommittingText = false;

Is this safe? Shouldn't you use try-finally for ensuring to reset the mCommittingText?

>@@ -559,61 +564,105 @@ public class GeckoInputConnection
> 
>     // TextWatcher
>     public void onTextChanged(CharSequence s, int start, int before, int count) {
>         if (hasCompositionString() && mCompositionStart != start) {
>             // Changed range is different from the composition, need to reset the composition
>             endComposition();
>         }
> 
>-        if (count == 1 && s.charAt(start) == '\n') {
>+        CharSequence changedText = s.subSequence(start, start + count);
>+
>+        if (count == 1 && changedText.charAt(0) == '\n') {

nit: please use changedText.length instead of count.

>+        boolean sentKeyEvents = false;
>+
>+        // If we are committing a single character and did not have an active composition string,
>+        // we can send Gecko keydown/keyup events instead of composition events.
>+        if (mCommittingText && changedText.length() == 1 && !hasCompositionString()) {
>+            sentKeyEvents = sendKeyEventsToGecko(changedText.charAt(0));
>+        }

How about you return from this if block if sendKeyEventsToGeckl() returns true? It seems that you need to call GeckoAppShell.geckoEventSync(); in the block though. Our coding rule recommends "early return" style.

>+    private boolean sendKeyEventsToGecko(char c) {

Please use two or more characters for variable name.

>+        boolean sentKeyEvents = false;
>+
>+        for (KeyEvent event : events) {
>+            if (!KeyEvent.isModifierKey(event.getKeyCode())) {
>+                event = KeyEvent.changeFlags(event, event.getFlags() | KeyEvent.FLAG_SOFT_KEYBOARD);

Why KeyEvent.FLAG_SOFT_KEYBOARD is needed? Please add comment for the reason.

>+                GeckoAppShell.sendEventToGecko(GeckoEvent.createKeyEvent(event));
>+                sentKeyEvents = true;
>+            }
>+        }

Does this loop dispatch one or more sets of NS_KEY_DOWN/NS_KEY_PRESS/NS_KEY_UP? I mean, if keyup isn't included, we need to generate it manually.
Blocks: 707353
> Is this safe? Shouldn't you use try-finally for ensuring to reset the mCommittingText?

commitText() is not expected to throw any exceptions. We don't need to worry about resetting mCommittingText because any exception here is a bug and will be handled by our crash reporter.


> Does this loop dispatch one or more sets of NS_KEY_DOWN/NS_KEY_PRESS/NS_KEY_UP? I mean, if keyup isn't included, we need to generate it manually.

AFAIK, Android's KeyCharacterMap will synthesize a KEY_DOWN and a KEY_UP event (and no KEY_PRESS) for each virtual key press, so sendKeyEventsToGecko() loop will dispatch an NS_KEY_DOWN and an NS_KEY_UP event.
patch v3:

* Add blassey for Java review recommended by Masayuki
* Check changedText.length, not count
* Return early after calling sendKeyEventsToGecko()
* Rename char c to inputChar
* Remove unnecessary FLAG_SOFT_KEYBOARD bit twiddling because Android's synthesized KeyEvents already have FLAG_SOFT_KEYBOARD
Attachment #614633 - Attachment is obsolete: true
Attachment #614633 - Flags: review?(masayuki)
Attachment #615933 - Flags: review?(masayuki)
Attachment #615933 - Flags: review?(blassey.bugs)
Comment on attachment 615933 [details] [diff] [review]
bug-687717-send-key-events-v3.patch

Looks great, thank you.

But I have a worry.

> +        if (changedText.length() == 1) {
> +            char changedChar = changedText.charAt(0);
> +            if (changedChar == '\n') {
> +                processKeyDown(KeyEvent.KEYCODE_ENTER, new KeyEvent(KeyEvent.ACTION_DOWN,
> +                                                                    KeyEvent.KEYCODE_ENTER), false);
> +                processKeyUp(KeyEvent.KEYCODE_ENTER, new KeyEvent(KeyEvent.ACTION_UP,
> +                                                                  KeyEvent.KEYCODE_ENTER), false);
> +                return;
> +            }
> +            if (mCommittingText && !hasCompositionString() && synthesizeKeyEvents(changedChar)) {
> +                // Block this thread until all pending events are processed
> +                GeckoAppShell.geckoEventSync();
> +                return;
> +            }

By this change, we see the Enter key special case could run during composition. I think that this may cause some bugs in nsEditor. So, I think that the first |if| condition should be |changedText.length() == 1 && !hasCompositionString()|. *However*, please don't do that in this bug. We should change the logic in another bug. If it would cause another regression, separated landing would be very helpful for both testers and developers.
Attachment #615933 - Flags: review?(masayuki) → review+
Blocks: 746657
The |processKeyDown(KeyEvent.KEYCODE_ENTER| code was not added in my patch; I just indented it. The Enter key special case was added for bug 721393. I agree that the code looks problematic, especially because processKeyDown() may cause onTextChange() to be reentered.

I just opened bug 746657 to fix the Enter key special case.

Do you know of a test case that will reproduce the Enter key during composition?
Attachment #615933 - Flags: review?(blassey.bugs) → review+
I have no idea. If you want to see what will happen in that situation, you can write a test with nsIDOMWindowUtils::send*Event().
https://hg.mozilla.org/mozilla-central/rev/23ad93b7c90a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
On the mobile version of twitter there is no counter for the tweet characters, which I think it's a bug because on the Android browser it appears on the top right part of the screen. 

On the desktop version of twitter the number appears and it decreases when characters are typed.
Depends on: 757856
Filed bug 757856 for the above issue.
This works now and Bug 757856 is closed as WFM. Closing this bug as well.
Status: RESOLVED → VERIFIED
Resolution: FIXED → WORKSFORME
You need to log in before you can comment on or make changes to this bug.