Last Comment Bug 688438 - [VKB] Typed characters not displayed in landscape when using VKB
: [VKB] Typed characters not displayed in landscape when using VKB
Status: RESOLVED FIXED
: regression
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 15
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
: 691617 755599 (view as bug list)
Depends on: 694880
Blocks: 612128 751513 755599
  Show dependency treegraph
 
Reported: 2011-09-22 06:58 PDT by Camelia Urian
Modified: 2012-06-04 14:26 PDT (History)
16 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (v1) (2.41 KB, patch)
2011-09-22 15:26 PDT, :Ehsan Akhgari (busy, don't ask for review please)
bzbarsky: review+
Details | Diff | Review

Description Camelia Urian 2011-09-22 06:58:51 PDT
Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110922 Firefox/9.0a1 Fennec/9.0a1
Device:  HTC Desire Z

Steps to reproduce:
1. Start Fennec in landscape mode.
2. Make sure hard keyboard is closed if available.
3. Tap twice the URL bar to open VKB.
4. Start typing url address.

Actual results:
  Typed characters are not displayed.

Expected results:
  Typed characters are correctly displayed.

Notes:
 - The characters are typed, just not displayed on screen.
 - After releasing the VKB, you can see the typed characters.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-22 07:12:24 PDT
Can we get a screenshot? The fullscreen keyboard is not part of Fennec. When the fullscreen keyboard is open, you can't see any part of Fennec. So if the characters are not appearing, I think it's a VKB problem. After you close the VKB, you _do_ see the characters in Fennec.
Comment 2 Camelia Urian 2011-09-22 07:38:26 PDT
After closing the VKB, you can see the typed characters. Please see attached video at: http://www.youtube.com/watch?v=lJq_a_mXtVc

Issue is not reproducing on previous nightly build: 
Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110921 Firefox/9.0a1 Fennec/9.0a1
Comment 3 Matt Brubeck (:mbrubeck) 2011-09-22 10:18:02 PDT
Possible regression from bug 612128. :(
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-22 10:51:44 PDT
So I can try backing that patch out again, but that will leave me with no way to proceed debugging this.  I really want to figure out what's going on here, but I need somebody who can reproduce this and who can also build to try to debug what's going wrong.  :(
Comment 5 Matt Brubeck (:mbrubeck) 2011-09-22 10:57:59 PDT
I cannot reproduce this on HTC T-Mobile G2 (Android 2.3) using Android Keyboard, Swiftkey X, or Swype.  I'll see if I can reproduce it on any other devices here.
Comment 6 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2011-09-22 11:08:20 PDT
I can reproduce this on my LG Optimus Black.
But I'm afraid I can't build to try to debug.
Comment 7 Matt Brubeck (:mbrubeck) 2011-09-22 12:25:20 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> I cannot reproduce this on HTC T-Mobile G2 (Android 2.3) using Android
> Keyboard, Swiftkey X, or Swype.  I'll see if I can reproduce it on any other
> devices here.

Oops, I was testing the wrong build before.  I actually *can* reproduce this on my G2 with the latest nightly build.  Ehsan, I'd be happy to help debug this, and sorry Android is causing so many troubles for your patch.  :(  Can we back it out (again) in the meantime?
Comment 8 Matt Brubeck (:mbrubeck) 2011-09-22 14:09:49 PDT
The "isReadOnly" variable in "set ActivePanel" is set to true on both my first and second tap because BrowserUI._isKeyboardFullScreen() returns true.  (The other clauses in the isReadOnly assignment statement evaluate to false both times.)
Comment 9 Matt Brubeck (:mbrubeck) 2011-09-22 14:12:37 PDT
I don't think this is related to the isReadOnly code in AwesomePanel.js, because this bug seems to affect every text field both in chrome and in web content.  I also tried hard-coding "isReadOnly = false" and it did not fix this bug.
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-22 15:26:46 PDT
Created attachment 561903 [details] [diff] [review]
Patch (v1)

This patch fixes the IME code to also take readwrite elements into account when searching for editable nodes.  This is required because those elements no longer have the NODE_IS_EDITABLE flag set after bug 612128.

Matt confirmed that this fixes the Fennec regression.
Comment 11 Mozilla RelEng Bot 2011-09-24 03:40:55 PDT
Try run for f68cb85052e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f68cb85052e7
Results (out of 171 total builds):
    exception: 1
    success: 146
    warnings: 22
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f68cb85052e7
Comment 12 Camelia Urian 2011-09-27 01:35:52 PDT
I verified issue with the try-build from:  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f68cb85052e7
and also with latest nightly: Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110926 Firefox/9.0a1 Fennec/9.0a1

Issue is not reproducing anymore.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-29 09:34:19 PDT
Boris - Can you suggest another reviewer, if you're swamped? Masayuki?
Comment 14 Scott Johnson (:jwir3) 2011-10-03 21:26:26 PDT
*** Bug 691617 has been marked as a duplicate of this bug. ***
Comment 15 Boris Zbarsky [:bz] 2011-10-06 15:08:41 PDT
Comment on attachment 561903 [details] [diff] [review]
Patch (v1)

r=me, though maybe we should have an nsIContent method for this....
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-06 15:25:10 PDT
(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 561903 [details] [diff] [review] [diff] [details] [review]
> Patch (v1)
> 
> r=me, though maybe we should have an nsIContent method for this....

That would encourage people to use that, but it's not what we want in almost all cases.  Using that method incorrectly can cause more bugs similar to bug 612128.  So I'd rather not do that.  :-)
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-07 07:00:15 PDT
Pushed to try to make sure that everything is working correctly before pushing this and bug 612128 to inbound.
Comment 18 Mozilla RelEng Bot 2011-10-07 16:02:36 PDT
Try run for 9bdf60a2a2cb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9bdf60a2a2cb
Results (out of 193 total builds):
    success: 150
    warnings: 20
    failure: 23
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-9bdf60a2a2cb
Comment 19 Matt Brubeck (:mbrubeck) 2011-10-13 11:49:47 PDT
This has been fixed by backout of bug 612128; leaving this bug open so that the real fix can be landed and verified when bug 612128 re-lands.
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-13 12:14:57 PDT
(In reply to Mozilla RelEng Bot from comment #18)
> Try run for 9bdf60a2a2cb is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=9bdf60a2a2cb
> Results (out of 193 total builds):
>     success: 150
>     warnings: 20
>     failure: 23
> Builds available at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.
> com-9bdf60a2a2cb

Masayuki, do you know what needs to be done about these failures?  I'm not sure what the desired behavior should be here...
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-10-14 03:25:58 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> (In reply to Mozilla RelEng Bot from comment #18)
> > Try run for 9bdf60a2a2cb is complete.
> > Detailed breakdown of the results available here:
> >     https://tbpl.mozilla.org/?tree=Try&rev=9bdf60a2a2cb
> > Results (out of 193 total builds):
> >     success: 150
> >     warnings: 20
> >     failure: 23
> > Builds available at
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.
> > com-9bdf60a2a2cb
> 
> Masayuki, do you know what needs to be done about these failures?  I'm not
> sure what the desired behavior should be here...

Don't enable IME on non-editable element. That breaks our IME state management. Your patch makes user be able to use IME on form controls (like checkbox). It provides very bad experience for IME users.

Why do you need to enable IME on such element which isn't text inputtable element?
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-10-14 03:33:58 PDT
Note that the "editable" in IME related code means "text editable", doesn't mean "form control state editable".
Comment 23 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-16 15:50:11 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> Don't enable IME on non-editable element. That breaks our IME state
> management. Your patch makes user be able to use IME on form controls (like
> checkbox). It provides very bad experience for IME users.
> 
> Why do you need to enable IME on such element which isn't text inputtable
> element?

I'm not.  Note that with bug 612128, text controls do not have the NODE_IS_EDITABLE flag any more, but they will always have the readwrite state.  This patch is changing the IME subsystem to correctly account for that, and it shouldn't (and doesn't) change any logic of which elements will have the IME state turned on.

I debugged these test failures, and they're the result of yet another bug, bug 694880.  I have a patch on that bug, which fixes these test failures.  :-)
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-10-16 21:01:44 PDT
Oh, OK. I see. Thanks.
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-20 09:31:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/269ee0275709
Comment 26 Marco Bonardo [::mak] 2011-10-21 02:15:25 PDT
https://hg.mozilla.org/mozilla-central/rev/269ee0275709
Comment 27 Camelia Urian 2011-10-25 02:36:57 PDT
Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111024 Firefox/10.0a1 Fennec/10.0a1
Device:  HTC Desire Z
OS: Android 2.3

Type characters are correctly displayed in landscape mode using VKB.
Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-25 19:34:43 PDT
Backed out because of bug 688423.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e8312683e7b
Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-24 14:50:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4651104e0d0e
Comment 30 Ed Morley [:emorley] 2012-05-25 08:27:17 PDT
https://hg.mozilla.org/mozilla-central/rev/4651104e0d0e
Comment 31 Johnathan Nightingale [:johnath] 2012-05-29 11:38:00 PDT
blocking+, please nom for approval after a few days of baking
Comment 32 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-29 12:24:43 PDT
Wait, why is this suddenly a blocker?  It doesn't even affect fennec 1.0.  Clearing the flag.
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-04 14:26:37 PDT
*** Bug 755599 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.