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

Status: VERIFIED WORKSFORME

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

Description

[webqa.utest]

Attachment: Screen shot of tweet box remaining characters not decreasing

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)

Comment 1

[webqa.utest]

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

Comment 2

[Aaron Train [:aaronmt]]

Let's figure out if this is a fennec issue or an issue on Twitter's end.

Comment 3

[Matt Brubeck (:mbrubeck)]

Probably caused by bug 630576.

Comment 4

[Chris Peterson [:cpeterson]]

I can reproduce this with Native Fennec's VKB. HKB works correctly.

Comment 5

[Chris Peterson [:cpeterson]]

The character counter does update after you press RETURN on the VKB. GeckoInputConnection is not sending individual key events when composing a string.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

does desktop have the same behavior with IME input?

Comment 7

[Chris Peterson [:cpeterson]]

Desktop IME has the same problem.

Comment 8

[Joe Drew (not getting mail)]

Presumably this also affects B2G.

Masayuki, can you see if you can track this down?

Comment 9

[Chris Peterson [:cpeterson]]

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.

Comment 10

[Chris Peterson [:cpeterson]]

btw, the "Special Characters" problem on Mac (bug 744545) was a content problem that is also reproducible on Chrome and Safari.

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: bug-687717-send-key-events.patch

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.

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: bug-687717-send-key-events-v2.patch

patch v2 adds `if (DEBUG)` for verbose log messages and removes some unnecessary `static` modifiers.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 14

[Chris Peterson [:cpeterson]]

> 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.

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: bug-687717-send-key-events-v3.patch

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

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 17

[Chris Peterson [:cpeterson]]

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?

Comment 18

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23ad93b7c90a

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have no idea. If you want to see what will happen in that situation, you can write a test with nsIDOMWindowUtils::send*Event().

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/23ad93b7c90a

Comment 21

[Andreea Pod]

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.

Comment 22

[Andreea Pod]

Filed bug 757856 for the above issue.

Comment 23

[Ioana Chiorean]

This works now and Bug 757856 is closed as WFM. Closing this bug as well.