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.

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.

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 !

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

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

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 on attachment 798390 [details] https://github.com/mozilla-b2g/gaia/pull/11881 I added comments and questions to the PR. Thanks !

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.

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

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

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

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?

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 ?

Neo and Carrie work on IME framework UX spec. Hi Neo and Carrie, Can you help to clarify this issue?

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.

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

Update keyword in white board for the status query more precise

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 !

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

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?

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.

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 ?

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.

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

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

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 !

(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 on attachment 798390 [details] https://github.com/mozilla-b2g/gaia/pull/11881 some more comments on github we're getting there !

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

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 :)

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

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.

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

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 !

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.

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

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.

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