893554 - Installation wizard should have a hook for 3rd party keyboard application exposing its layout selections

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Evelyn Hung]

Per bug 891671 describe, when a IME app is installed, the wizard will take the user to a IME setup page for completing keyboard installation and activation.

Comment 1

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: WIP patch

Hi Julien Wajsberg,
   This WIP patch is for 3rd party keyboard apps installation wizard.
   Please help review 1aabe55c49c64141eed71bdd94ece418f483d847 only.

   The ux spec is in https://bug891678.bugzilla.mozilla.org/attachment.cgi?id=776309 page 13.

   New keyboard framework got r+ but we found some gecko bug (bug#909124) let it fail to run Gaia with b2g desktop, so we can not land new keyboard framework now (in fact the framework was be backed out on Monday).

   I think if you can give me some feedback before gecko bug fixed and the framework ready (land in gaia master), it will help us to finish this feature faster and on schedule.

   Rudy has uploaded test-keyboard app on dev.marketplace for testing this feature, here is the link https://marketplace-dev.allizom.org/app/keyboard-test-1.

   If you can not install test-keyboard app from marketplace-dev, here is other way to test the installation flow.

   This is my test fake-keyboard app(http://mpizza.github.io/ime/install.html).

   thanks.

Comment 2

[Julien Wajsberg [:julienw]]

Gary, can you tell me how I can apply this on current master ? Should I use patches that are in other bugs ?

Without this it's difficult for me to review this.

Also, I'd like to have a proper pull request, or a real patch attached to this bug, otherwise the review comments will go away with a new commit.

Thanks !

Comment 3

[Julien Wajsberg [:julienw]]

I've put some comments on the commit as a start.

Comment 4

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: browser.json

Put this file under gaia/distribution/ folder. My test app will be default bookmark in browser app then you can install it more convenience.

Comment 5

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/11881

Hi Julien,
   I have updated my patch with your comment and send pull request.
   3rd party Keyboard framework was landed so you can test it in master.
   You might need this pad(https://gaia.hackpad.com/Install-Packaged-app-from-dev-marketplace-22fLYwKOyGx) for installing test-keyboard in marketplace-dev, or you can also install my test app(attachment #798371 [details]) though it is a hosted app.

   As your comment, according to UX spec we did not use actives for selecting keyboard layouts. We have new keyboard UX spec. in setting app, it will list all keyboard apps' layouts but we only list one keyboard app's layouts which is installed by user.

  The other thing is why we use "role==keyboard" to filter what is keyboard app.
  "role" field is our proposed IME manifest to identity 3rd party keyboard.( https://wiki.mozilla.org/index.php?title=WebAPI/KeyboardIME&#Proposed_Manifest_of_a_3rd-Party_IME). If we use other fields like "web actives" or "permission" might be get error because some app need to keyboard api and it will expose "permission" with keyboard but it is not a IME app. To prevent this issue, "role==keyboard" is the best way to filter it.

  Thanks for your comment, and please review my PR again.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 798390 [details]
https://github.com/mozilla-b2g/gaia/pull/11881

I added comments and questions to the PR.
Thanks !

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

FYI, just uploaded a new testing keyboard app to dev. marketplace,
https://marketplace-dev.allizom.org/app/gaia-test-keyboard/

As Gary mentioned, please ref. the following guide to install apps from dev.marketplace.
https://gaia.hackpad.com/Install-Packaged-app-from-dev-marketplace-22fLYwKOyGx

Please note that this should enable installation only, for now privileged app could not access keyboard api.

Thanks.

Comment 8

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julien,
   I have updated my PR with your comment, and also add simple unit test please review it again.
   
   Thanks.
   https://github.com/mpizza/gaia/commit/f6eefc155244b5b80f3cf99a1962bbb6c0cfd7da

Comment 9

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

After talking with Arthur[:crh0716], 
we all agree to file another bug for using 'activity' in settings app.

Arthur and I will go to Oslo workweek, maybe we can discuss this topic there. 
:)

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to GaryChen [:GaryChen] from comment #9)
> After talking with Arthur[:crh0716], 
> we all agree to file another bug for using 'activity' in settings app.
> 
> Arthur and I will go to Oslo workweek, maybe we can discuss this topic
> there. 
> :)

Sorry I don't understand why we don't do it now ? It doesn't seem to be so difficult to do and that would make this patch simpler...

Comment 11

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Sorry, I forgot to mention the root cause why we want to land this patch first.

If we use 'activity' to pass 'layout' into Setting app,
user will see the root panel first and then see keyboard layout.

That is not a good user experience and not fit in UX Spec.

Arthur, do you have any suggestion?

Comment 12

[Julien Wajsberg [:julienw]]

But he will see this after he clicks on "Yes I want to configure my layout" so it doesn't look like it's really a surprise for him. 

Who has done the UX spec, so that we can ask him ?

Comment 13

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Neo and Carrie work on IME framework UX spec.

Hi Neo and Carrie,
   Can you help to clarify this issue?

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

I think it makes sense only when there is exactly the same UI available in the settings APP. If the UI is specifically designed for allowing users to enable layouts of the newly installed keyboard app and is not displayed in normal use of the settings app, it makes more sense to keep the UI in the system app IMHO. By doing this we can avoid unnecessary transitions of switching to the settings app and navigating through panels.

Let's wait for inputs from UX designers.

Comment 15

[Julien Wajsberg [:julienw]]

Arthur> Yes, if the panels are different, then we'll need specific code while installing. But do we really want to have different panels ?

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 798390 [details]
https://github.com/mozilla-b2g/gaia/pull/11881

More comments on github pull request.

Lots of nits, but the most important things are:
* you need to call preventDefault on buttons
* we need a new "applicationinstallsuccess" because "applicationinstall" happens too soon for packaged app
* what is in the "configurations" object should be different, even if the idea is here
* we need to prevent having 2 keyboard install in the same time
* is it a good time to move all these dialogs handling to an external object ?

And still the open question about using a settings activity.

Thanks, this will be a top quality patch !

Comment 17

[Ivan Tsay (:ITsay)]

Update keyword in white board for the status query more precise

Comment 18

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julien,
   Sorry interrupt you again, I am worried about it might be block IME framework and not on schedule.
   So I have a WIP patch could you give me some feedback first, 
   though I didn't test this patch on device.(I forget to bring it home) :(

   I even try to use B2g desktop build to test my patch,
   but seems install app does not work in b2g desktop build.

   Here is the link: 
   https://github.com/mpizza/gaia/commit/70b6c6bb88e90cde38d3fd4ebe4be5f7fef1640b

   Thank you very much.
(In reply to Julien Wajsberg [:julienw] from comment #16)
> Comment on attachment 798390 [details]
> https://github.com/mozilla-b2g/gaia/pull/11881
> 
> More comments on github pull request.
> 
> Lots of nits, but the most important things are:
> * you need to call preventDefault on buttons
   I use type="button" so I do not need do this, but thanks your remind.
> * we need a new "applicationinstallsuccess" because "applicationinstall"
> happens too soon for packaged app
  good points!!
> * what is in the "configurations" object should be different, even if the
> idea is here
> * we need to prevent having 2 keyboard install in the same time
I use |this.IMEqueue|, |checkIMEQueue| and |completedIMETask| to prevent having 2 keyboard install in the same time. Can this way fix this issue or do you have any suggestion?

   
https://github.com/mpizza/gaia/commit/70b6c6bb88e90cde38d3fd4ebe4be5f7fef1640b
> * is it a good time to move all these dialogs handling to an external object
> ?
> 
> And still the open question about using a settings activity.
> 
> Thanks, this will be a top quality patch !

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to GaryChen [:GaryChen] from comment #18)
> Hi Julien,
>    Sorry interrupt you again, I am worried about it might be block IME
> framework and not on schedule.
>    So I have a WIP patch could you give me some feedback first, 
>    though I didn't test this patch on device.(I forget to bring it home) :(
> 
>    I even try to use B2g desktop build to test my patch,
>    but seems install app does not work in b2g desktop build.
> 
>    Here is the link: 
>   
> https://github.com/mpizza/gaia/commit/
> 70b6c6bb88e90cde38d3fd4ebe4be5f7fef1640b
> 
>    Thank you very much.
> (In reply to Julien Wajsberg [:julienw] from comment #16)
> > Comment on attachment 798390 [details]
> > https://github.com/mozilla-b2g/gaia/pull/11881
> > 
> > More comments on github pull request.
> > 
> > Lots of nits, but the most important things are:
> > * you need to call preventDefault on buttons
>    I use type="button" so I do not need do this, but thanks your remind.
> > * we need a new "applicationinstallsuccess" because "applicationinstall"
> > happens too soon for packaged app
>   good points!!
> > * what is in the "configurations" object should be different, even if the
> > idea is here
> > * we need to prevent having 2 keyboard install in the same time
> I use |this.IMEqueue|, |checkIMEQueue| and |completedIMETask| to prevent
> having 2 keyboard install in the same time. Can this way fix this issue or
> do you have any suggestion?

This part looks good.

Maybe just replace "IME" with "Setup" because I'd like that the general configuration part is generic and therefore has generic names.

I've put some more comments on the github PR.

Sorry I won't be able to help you more today but I'll review it tomorrow again when I arrive to the office.

Comment 20

[Carrie Wang [New account: [e-mail]]]

Hi all,
From UX perspective, it's quite weird to have an extra transition when installing a 3rd party keyboard.
Users might feel interrupted and the experience won't be good. 

Hence, I don't suggest to do the change. Thanks!

(In reply to GaryChen [:GaryChen] from comment #13)
> Neo and Carrie work on IME framework UX spec.
> 
> Hi Neo and Carrie,
>    Can you help to clarify this issue?

Comment 21

[Julien Wajsberg [:julienw]]

Kaze tells me that the extra transition is supposed to be removed.

NI Anthony who supposedly worked on this.

I'm ok with moving this part to another bug if we need to move fast.

Comment 22

[Julien Wajsberg [:julienw]]

Anthony told me that the transition is still there.

Asking NI from alive and vivien about comments 10, 11, 12, 14, 20. Basically, from a System owner point of view, is it ok to handle the layout handling after install in the System app, or should we use the "configure" activity even if everything is not like UX specified ?

Comment 23

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Juline,
   please help mo to review this patch again.
   https://github.com/mozilla-b2g/gaia/pull/11881
   
   Maybe we should also NI Evelyn who is Settings owner.

Hi Evelyn,
   Could you give some feedback about comments 10, 11, 12, 14, 20?
   Thanks.

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

My 2$:
* I dislike we have more than one codes doing the same thing at different places.
  That means if UX changes his/her mind we need to change the code two or more times.
* I tend to let settings app be able to have another standalone page for inline activity handling.
  This could be done by: use one templating HTML, rendering to both index.html#keyboard(subpage) and keyboard.html, and do the module decoupling to let keyboard_settings.js or something like this to be injected in settings 'app' and settings 'activity'.

Comment 25

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Kaze tells me that the extra transition is supposed to be removed.
> 
> NI Anthony who supposedly worked on this.
> 
> I'm ok with moving this part to another bug if we need to move fast.

Hi Julien,

Thanks for looking into this UX related issue with the consideration on better code quality and module maintainability.
However, I would suggest we open a follow-up bug for this question and land the latest patch that Gary provided here, of course with your r+. :)

This is because, without this patch, we cannot ask for QA's help to test the installation/un-installation flow of 3rd-party IME, and this is an important user story for v1.2 that we are going to finish by next week.

Comment 26

[Julien Wajsberg [:julienw]]

Removing NI from evelyn, I think it was made by mistake.

Rudy, Gary> yep, i'm ok with moving forward without using the configure activity (I said it in comment 21 already :-) ) and thinking more about this in a follow-up bug.

Will do another review tomorrow, it's late here already !

Comment 27

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Removing NI from evelyn, I think it was made by mistake.
> 
> Rudy, Gary> yep, i'm ok with moving forward without using the configure
> activity (I said it in comment 21 already :-) ) and thinking more about this
> in a follow-up bug.
> 
> Will do another review tomorrow, it's late here already !

Sure~ take your time, happy weekend. XD

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 798390 [details]
https://github.com/mozilla-b2g/gaia/pull/11881

some more comments on github

we're getting there !

Comment 29

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julien,
   Please help me review my patch again.
   https://github.com/mozilla-b2g/gaia/pull/11881

   thanks.

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 798390 [details]
https://github.com/mozilla-b2g/gaia/pull/11881

Some more comments on github.

Hopefully the last round for the code, maybe more for unit tests as I couldn't test them :)

Comment 31

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julien,
   please help review another round, thanks.
   https://github.com/mozilla-b2g/gaia/pull/11881/commits

Comment 32

[Julien Wajsberg [:julienw]]

done ;)

1 small comment in the code, otherwise mostly comments for the unit tests, as expected :)

I couldn't test the last version because I don't have my moz-central keon right now, will test the patch asap.

Comment 33

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julein,
   I have addressed your comment please review it. 
   thanks.
   https://github.com/mozilla-b2g/gaia/pull/11881

Comment 34

[Julien Wajsberg [:julienw]]

Discussed this orally: the right buttons should be blue (I think this is `class="recommend"` but please check this works), plus another assert as said on https://github.com/mpizza/gaia/commit/85b19fbeba7623980e597b5fcdee5060c932ebdc#commitcomment-4076094

then squash it and ask me again, and we're good !

Comment 35

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Julien,
   I've added |class="recommend"| in two buttons as following link,

   https://github.com/mozilla-b2g/gaia/pull/11881/files#L0R901
   https://github.com/mozilla-b2g/gaia/pull/11881/files#L0R927

   and add addition check in unit test
   https://github.com/mozilla-b2g/gaia/pull/11881/files#L6R1306


   thanks for the patience to review this patch.

Comment 36

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

I opened a follow up bug for move wizard feature into settings app.
https://bugzilla.mozilla.org/show_bug.cgi?id=915665

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 798390 [details]
https://github.com/mozilla-b2g/gaia/pull/11881

Travis looks good, r=me, land it !

Please add "r=julien" either in the commit log or in the merge commit log.

Comment 38

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

merged into gaia-master:
https://github.com/mozilla-b2g/gaia/commit/59e172cecef27cd0792c7da3ffbf87973be8f706
https://github.com/mozilla-b2g/gaia/commit/f392411207d9bad5fb0aefb65307d78b60664ed9