Closed Bug 912010 Opened 6 years ago Closed 6 years ago

[Keyboard][V1.2] Default Keyboard when all keyboards are de-selected

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

VERIFIED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: whsu, Assigned: gnarf)

References

Details

(Whiteboard: [FT:System-Platform], [Sprint:3], [3rd-party-keyboard])

Attachments

(3 files, 1 obsolete file)

* Description:
  This problem happens on v1.2 build(M-C) and related to 912007
  We shouldn't allow user to only select number keyboard because it caused no default text keyboard

* Reproduction steps:
  1. Launch the keyboard settings app
  2. Tap "Active Keyboard"
  3. Tap "Add more keyboards"
  4. Unselect all keyboards and only select number keyboard
  5. Launch the message app
  6. Tap pencil to add a new message
  7. Tap text field

* Expected result:
  Having a text keyboard pop up

* Actual result:
  No keyboard pop up

* Reproduction build:(Mozilla-central-unagi-eng/2013-09-02-04-02-02)
  + Mercurial-Information
    - Gecko revision="0457c197a382a219b1050fd2ccfddfdbbdad7a92"
  + Git-information
    - Gaia revision="1179318fb5aa"
  + Gecko version: 26.0a1
blocking-b2g: --- → koi?
Whiteboard: [FT:System-Platform], [Sprint:3]
Fix it in v1.2
Assignee: nobody → gchen
blocking-b2g: koi? → koi+
What should the desired result be?  Should the default english keyboard reactivate itself?

I can imagine another scenario to get here too which would be enabling only a custom keyboard app, then uninstalling it leading to no selected input methods being available.
Hi,

Thanks for the comment.
Yes, the default English keyboard will reactivate itself if user unselects the all keyboard layout.
Attaching the user scenario. FYI.

By the way, user cannot uninstall the built-in keyboard.
Thanks!
Attached image Unselect all keyboards
Is the "default keyboard language" baked into the settings somewhere, could we have it use Polish in Polish areas for instance?

Also, this should make sure it can cover the scenario where you only have custom keyboards selected and then you uninstall that application.  The system/keyboard_manager should know to re-activate the "default language" keyboard (which could be english by default, but if we set a default at build time it might be best to use the default)

Gary Chen - Are you working on this or can I take it?
Flags: needinfo?(gchen)
sure take it!!
Flags: needinfo?(gchen)
Assignee: gchen → gnarf37
Summary: [Keyboard][V1.2] Only selecting the number keyboard cause the text keyboard cannot display → [Keyboard][V1.2] Default Keyboard when all keyboards are de-selected
Corey,

In 1.1, the default keyboard is based on the default locale. There is code in apps/communcations/ftu that sets the default keyboard somehow when the user selects a language.  And I think there is code in apps/settings that ensures that an appropriate keyboard is enabled when the user selects a new language.

So you'll want to see if there are parts of that code that you can salvage in this new world of multiple keyboard apps.
Corey,

Any updates here?
Flags: needinfo?(gnarf37)
Yeah, sorry, lost a week dealing with getting the ZTE open working properly for debugging for our team.

I've been working on refactoring KeyboardHelper to be better for the tasks it helps with such that this problem can be solved in system, settings, and ftu apps.
Flags: needinfo?(gnarf37)
Not ready for review yet, just hoping for some feedback on the refactoring already done.
Attachment #817401 - Flags: feedback?(dflanagan)
Attachment mime type: text/plain → text/x-github-pull-request
So, most of the code has been put in place here to be able to enable default keyboards if they somehow all become unselected (deals with the removal of a keyboard app causing this).  At this point, Inside the settings app, I'm leaning much more towards disabling the the checkbox on a keyboard that is the last of its type (i.e. can't disable the last number keyboard, or the last text keyboard) in the settings app rather than displaying any new dialog.  Is this an acceptable solution, or must we implement this dialog alert as shown in the attachment?
Flags: needinfo?(whsu)
Hi, Corey,

Thanks for your help!
Could you please follow UX specification?
Reasons are as below.
1. User may feel confuse without alarm dialog pop up.
2. Stakeholders and QAnalysist follow the specification to review this feature.
3. If we want to change the specification, we need UX to review it.

Many thanks! :)
Flags: needinfo?(whsu)
I like Corey's proposition a lot more. Preventing the user to get in a bad situation is better than informing the user that he/she is in a bad situation.

Could we get UX opinion on this?
Who from UX do we need to needsinfo?
Flags: needinfo?(whsu)
Flags: needinfo?(whsu) → needinfo?(firefoxos-ux-bugzilla)
Here is an alternate idea where the checkbox just can't be un-checked: https://github.com/gnarf/gaia/commit/a54aed8904163a2dfda0355e9022483cf4b8f9f3
Comment on attachment 817401 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12866

Rudy, David: I think this is ready for review.  Can you please also flag whoever should be flagged for the settings/ portions of this?
Attachment #817401 - Flags: review?(rlu)
Attachment #817401 - Flags: review?(dflanagan)
Attachment #817401 - Flags: feedback?(dflanagan)
ni UX to answer comment 13.
Carrie, do you think the proposal in comment 11 is better?
Flags: needinfo?(cawang)
We'd suggest to keep the original design in this version. 
The solution in comment 11 is quite neat, but the one in spec can provide more information to users.
Hence, if we want to provide a new approach, I'd suggest to postpone it to next version and we can consider it more comprehensively at that time. Thanks!
Flags: needinfo?(cawang)
Comment on attachment 817401 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12866

I can help review the part of settings app.
Attachment #817401 - Flags: review?(arthur.chen)
(In reply to cawang from comment #18)
> We'd suggest to keep the original design in this version. 
> The solution in comment 11 is quite neat, but the one in spec can provide
> more information to users.
> Hence, if we want to provide a new approach, I'd suggest to postpone it to
> next version and we can consider it more comprehensively at that time.
> Thanks!
I don't understand the logic here. If it's a neat solution, why postpone it? It's easier to implement. Less UI to create, less strings to translate. And again, "giving information to the user" is good but not as interesting as "preventing the user to do something stupid".
Whether something is stupid or not really depends. The original solution enables users to do what they want to do. For example if users just want to disable a layout that is happened to be the last one, they don't need to enable another layout before doing that. And with a long installed layout list, users may have difficulty in understanding why they cannot disable a layout.
Another open question for someone... Should changing language in the settings app change the default keyboards also?
I don't think so because it's not always clear. F.e. when setting Serbian locale, which keyboard should be loaded? Serbian Cyrillic or Latin?
I am removing the ni? to UX because Carrie, from our team, advised a solution in comment #18. There are a lot of keyboard changes coming for both 1.3 and 1.4, so anything beyond the existing spec can be evaluated as part of those releases, given how late in the game it is for 1.2.
Flags: needinfo?(firefoxos-ux-bugzilla)
Comment on attachment 817401 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12866

Ask Gary's help to be one more pair of eyes on this significant patch.
I will also try to review this patch with Gary on the keyboard manager and helper part.
Attachment #817401 - Flags: review?(dflanagan) → review?(gchen)
Okay, I will help on KM and KH part.
Comment on attachment 817401 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12866

Looks good to me for keyboard_manager.
r=me with the review comments addressed.

Still need Arthur to review the settings and Gary to give a more comprehensive review on keyboard_helper.

Corey,

Thanks for your work.
Attachment #817401 - Flags: review?(rlu) → review+
Comment on attachment 817401 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12866

Looks good for me in KM and KH part,
r=me, 

still need to wait settings part.

Thanks for your work.
Attachment #817401 - Flags: review?(gchen) → review+
Status: NEW → ASSIGNED
Whiteboard: [FT:System-Platform], [Sprint:3] → [FT:System-Platform], [Sprint:3], [3rd-party-keyboard]
Duplicate of this bug: 912375
Duplicate of this bug: 912007
Comment on attachment 817401 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12866

r=me for the settings part. Please squash the commits before merging, thanks!
Attachment #817401 - Flags: review?(arthur.chen) → review+
Corey, just a soft reminder that you're going to squash all your commits and land it, right?
Thanks.
Flags: needinfo?(gnarf37)
Will land this later today.  I always squash :)
Flags: needinfo?(gnarf37)
closed https://github.com/mozilla-b2g/gaia/commit/ad0bf992cd379a32de18b9a4bb765757db8af986
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Broke gaia-ui-test, "test_everythingme_launch_packaged_app.py test_everythingme_launch_packaged_app.TestEverythingMeSearchPanel.test_launch_packaged_app_from_search_panel | NoSuchElementException: Unable to locate element: button.keyboard-key[data-keycode="97"]", https://tbpl.mozilla.org/php/getParsedLog.php?id=29681010&tree=B2g-Inbound.

b2g-inbound is closed.
That test didn't fail on the travis build before or after squash - I'm not sure that one is my fault.
Travis isn't a perfect guide to what'll happen in releng's automation, since from the way I understand it, travis runs all the tests in one lump, letting things leak from one test to another, while releng's automation doesn't leak them. I might have been wrong since I missed noticing that the push before yours didn't actually run gaia-ui-test, but I retriggered it, and https://tbpl.mozilla.org/?tree=B2g-Inbound&fromchange=b6084fd98880&tochange=3067fa69485b&jobname=gaia-ui-test certainly seems to be pointing at this.
I attempted to revert this so we could get b-i reopened, but I couldn't do so cleanly. This is going to require attention from someone with more git knowledge than me.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/28] from comment #40)
> I attempted to revert this so we could get b-i reopened, but I couldn't do
> so cleanly. This is going to require attention from someone with more git
> knowledge than me.

corey can you help us here to get the tree reopen and to find out if that push was really causing this bustage ?
Flags: needinfo?(gnarf37)
I don't think the patch here is at fault here either. The dedicated test for keyboard test_keyboard.py runs fine everywhere.

The closest e.me change took place on bug 928946. Could we rule out that bug?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #42)
> I don't think the patch here is at fault here either. The dedicated test for
> keyboard test_keyboard.py runs fine everywhere.
> 
> The closest e.me change took place on bug 928946. Could we rule out that bug?

Hm Bug 928046 landed on Wed Oct 22 https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=4697a6556365 and the GU Test where fine according to TBPL Results. And the Test failure happened first Friday at around noon for the tests for this bug here it seems.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #42)
> I don't think the patch here is at fault here either. The dedicated test for
> keyboard test_keyboard.py runs fine everywhere.
> 
> The closest e.me change took place on bug 928946. Could we rule out that bug?

ISTR an email going around recently about not being afraid to backout if there are test failures and *then* investigate. What's the big deal about backing this out when TBPL is clearly pointing to it as the culprit? You have 3 sheriffs who do this for a living telling you that now.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/28] from comment #44)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #42)
> > I don't think the patch here is at fault here either. The dedicated test for
> > keyboard test_keyboard.py runs fine everywhere.
> > 
> > The closest e.me change took place on bug 928946. Could we rule out that bug?
> 
> ISTR an email going around recently about not being afraid to backout if
> there are test failures and *then* investigate. What's the big deal about
> backing this out when TBPL is clearly pointing to it as the culprit? You
> have 3 sheriffs who do this for a living telling you that now.

Right. Thanks for the clarification.
I'm pretty sure whats happening is that e.me's test is booting up so fast that the keyboard has created a frame but hasn't actually loaded the keyboard app.  It might take me until tuesday to actually try and solve this integration test failure.  I'll see if I can get the revert to work
Flags: needinfo?(gnarf37)
Backed out with the following commit, please refer to Comment 39 for the failed test case of gaia-ui-tests,
90fbd75eccd3408ca04907fd80b72275771b3b30

--
Corey helped to look into the test failure and came out with a patch to "workaround" the failure,
https://gist.github.com/gnarf/0e19c99af5f81476167b

We in Taipei will follow up to check this patch since it is quite late for Corey in his timezone.
Thanks to Corey for working on this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file PR 13125
walter, could you help to review the quick patch?
Attachment #823194 - Flags: review?(wachen)
Comment on attachment 823194 [details] [review]
PR 13125

I ran it in my pc with desktop version and phone. It passed in my pc. r+
hopefully, this will fix the issue in the server!
Attachment #823194 - Flags: review?(wachen) → review+
master: https://github.com/mozilla-b2g/gaia/commit/273d6223094f2bf617e2189e87296dbd1acb31f0
This patch fixes the potential bug in Gaia UI test

master: https://github.com/mozilla-b2g/gaia/commit/5441c37b27ed6220ca0545c576e4d26c2ecf4ca3
This patch reverts the revert.

Let's see if TPBL complains. Corey, Taipei is passing the torch here, if the patch is backed out again please help.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(gnarf37)
Resolution: --- → FIXED
Thank you for reverting this.

However, I'm slightly frustrated that b2g-inbound was closed for 2.5 days because of test failures in the days leading up to uplift. This really isn't fair on all the other B2G developers and will have significantly hampered our ability to smoketest recent b2g-inbound changes on mozilla-central over the weekend, which is pretty harmful right before uplift.

The first reaction should be to revert changes to green the tree up, and worry about fixing the patch later. Ordinarily sheriffs would make the revert immediately as a matter of course (for gecko landings), but I'm presuming in this instance there were conflicts or else ambiguity as to which changeset needed backing out, given gaia repo landings are slightly less easily sheriffed.
(In reply to Ed Morley [:edmorley UTC+1] from comment #53)
> but I'm presuming in this instance there were conflicts

Ah yes comment 40, remember seeing it in bugmail now.
(In reply to Ed Morley [:edmorley UTC+1] from comment #54)
> (In reply to Ed Morley [:edmorley UTC+1] from comment #53)
> > but I'm presuming in this instance there were conflicts
> 
> Ah yes comment 40, remember seeing it in bugmail now.

Yes, we now have to land/backout bug 928281 together with this bug.

The fact that Gaia has not try server available means we now have to make educated guesses and land the patch again later. Not sure how many times it would take...
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #55)
> (In reply to Ed Morley [:edmorley UTC+1] from comment #54)
> > (In reply to Ed Morley [:edmorley UTC+1] from comment #53)
> > > but I'm presuming in this instance there were conflicts
> > 
> > Ah yes comment 40, remember seeing it in bugmail now.
> 
> Yes, we now have to land/backout bug 928281 together with this bug.
> 
> The fact that Gaia has not try server available means we now have to make
> educated guesses and land the patch again later. Not sure how many times it
> would take...

Hey Tim,

so just wondering

the merge pull request
https://hg.mozilla.org/integration/b2g-inbound/rev/366cb465a555
turned into green gaia ui tests:
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=366cb465a555

the revert after 
https://hg.mozilla.org/integration/b2g-inbound/rev/8a6d13df4e44
turned into failed/busted tests
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=8a6d13df4e44

was this expected ?
https://github.com/mozilla-b2g/gaia/commit/4fd4b736c967cdf4456e1f7664c75a466ffe34f2
(In reply to Carsten Book [:Tomcat] from comment #56)
> was this expected ?
>https://github.com/mozilla-b2g/gaia/commit/4fd4b736c967cdf4456e1f7664c75a466ffe34f2

grr ignore the github link here, was a copy & paste error
Finally I can reproduce this issue with:
  -  Run Gaia-ui-tests again b2g-desktop (Gecko 27)
  -  On my Macbook air

Failed Case
===========
It seems that in this case, the keyboard app itself would not correctly invoke "inputMethod.activate()" in its showKeyboard(), and it affected the switching of uppercase status.
This case would need to input "Calendar" into the search bar and it would enter "Caps-Locked" mode after inputting "C", so it failed to input "a". (The log said failed to find data-keycode="97", i.e. ASCII code of "a".)
It entered "Capslocked" mode because the test automation pressed "Caps" key for the first time for capitalized "C" and then pressed it again for lowercase "a".

Success Case
=============
If the latin inputMethod would have been inited correctly, it would change back to lowercase mode automatically after inputing "C", so it is fine to input "a" directly.

Why inputMethod.activate() would not be invoked is still under investigation.
Note: The keyboard app was not touched in the original patch.
b2g inbound closed again; now 3 days since this first broke :-(
https://github.com/mozilla-b2g/gaia/pull/13149 seems to fix this test every time - I still want to know what the hell is exposing this timing problem, but merging this "fix" in to the gaia-ui-test would allow us to not have a broken tree while we hunt it down.
Flags: needinfo?(gnarf37)
temp fix for the e.me broken test
Attachment #823495 - Flags: review?(jlal)
Attachment #823495 - Flags: review?(emorley)
This looks like a timing issue... To debug this we would need a try solution which we don't have for gaia-ui-tests (since the source is gaia) and it does not reproduce locally easily OR on other CI solutions which let us "pre-test" the change.. Its time to xfail this and move on. We need an owner to un-xfail the test and figure out whats going on (looks like Rudy is on track to fixing this) BUT we also need this patch landed to continue work for 1.2.

Assuming we manually tested this post landing and the keyboard is still working we should xfail and wait for the longer term fix.
Comment on attachment 823495 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/13149

I strongly prefer an xfail over a sleep unless we have a real condition we are waiting for...

I landed the xfail here: https://github.com/mozilla-b2g/gaia/commit/7078b6ec8e0a70afff056dcc60a1136d204c4d7c
Attachment #823495 - Flags: review?(jlal) → review-
Attachment #823495 - Flags: review- → review?(jlal)
^ now with a solution not involving a sleep, just don't bother messing with the shift key in the everything.me search bar
Comment on attachment 823495 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/13149

moved to bug 931997 to re-enabled the xfail'ed test
Attachment #823495 - Attachment is obsolete: true
Attachment #823495 - Flags: review?(jlal)
(In reply to Phil Ringnalda (:philor) from comment #39)
> Travis isn't a perfect guide to what'll happen in releng's automation, since
> from the way I understand it, travis runs all the tests in one lump, letting
> things leak from one test to another, while releng's automation doesn't leak
> them. I might have been wrong since I missed noticing that the push before
> yours didn't actually run gaia-ui-test, but I retriggered it, and
> https://tbpl.mozilla.org/?tree=B2g-
> Inbound&fromchange=b6084fd98880&tochange=3067fa69485b&jobname=gaia-ui-test
> certainly seems to be pointing at this.

Travis runs the tests with a fresh Firefox profile each time, there is no leak.
This is currently fixed on master
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
(In reply to Carsten Book [:Tomcat] from comment #56)

I think I did something wrong here before I afk yesterday.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #52)
> Reverted:
> https://github.com/mozilla-b2g/gaia/commit/4fd4b736c967cdf4456e1f7664c75a466ffe34f2

This commit did not reach gaia master and instead it reaches the master branch of my own repo:
https://github.com/timdream/gaia/commit/4fd4b736c967cdf4456e1f7664c75a466ffe34f2

I have no idea why Github does not resolve to 404 for the incorrect link.

So the actual commits of this bug are

ad0bf992cd379a32de18b9a4bb765757db8af986 (comment 53 -- first push)
90fbd75eccd3408ca04907fd80b72275771b3b30 (comment 47 -- revert w/ bug 928281)
273d6223094f2bf617e2189e87296dbd1acb31f0 (comment 48 -- one attempt to fix the test)
5441c37b27ed6220ca0545c576e4d26c2ecf4ca3 (comment 50 -- revert the revert)
7078b6ec8e0a70afff056dcc60a1136d204c4d7c (comment 64 -- xfail)
I've got no clue what's happening with this bug.  Tim, you seem to have a better idea what's going on here.  Could you take a look at this uplift?
Flags: needinfo?(timdream)
Blocks: 928281
master: 5441c37b27ed6220ca0545c576e4d26c2ecf4ca3 contains bug 912010 && bug 928281 "un reverted"

It should be possible to uplift them individually still anyway:

from ad0bf992cd379a32de18b9a4bb765757db8af986 (for 912010) 
and 9c6586d85d4340600ace0aa69f5741c5f3f67970 (for 928181)
and then b6221a5ecdeb1252c42f6a7b5628c90c1888346b (for 931997)

There were a couple of conflicts I resolved while doing it: 

https://github.com/gnarf/gaia/compare/mozilla-b2g:v1.2...gnarf:uplift-temp

This contains all 3 bugs that were caught up or created by this revert-o-rama :)
Flags: needinfo?(jhford)
This was uplifted in 90d2ede..cce2ba1
Flags: needinfo?(jhford)
(In reply to Corey Frang [:gnarf] from comment #73)
> and 9c6586d85d4340600ace0aa69f5741c5f3f67970 (for 928181)

928281

> and then b6221a5ecdeb1252c42f6a7b5628c90c1888346b (for 931997)

931977
Flags: needinfo?(timdream)
v1.2 reland: e717aec947571f5daf923c040a82f9f0719bb526

Let's see if we could isolate the issue.
Hi, all,

Thanks for all your help.
I verified this patch.
But, we seemed not yet localized this page.
I will file another bug to trace it. :)

* Test Build:
 - Gaia:     2140c987fdde1c99097018f7e93b0bbd43d2125d
 - Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6a831fcb96f4
 - BuildID   20131106004004
 - Version   26.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.