1102831 - [UserDictionary] Implement user dictioanry UI to KeyboardSettingsApp

Status: RESOLVED FIXED

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

Description

[John Lu [:mnjul][Please NI or I'll miss]]

We'll probably hide the UI with build script for now;

I intend to have this bug only touch "the list of UD words" indexDB entry, and not include binary blob generation in this bug. In this case, we can have a separate bug that tackle potential performance issues with blob generation.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Alright I'm on this. KeyboardSettingsApp look like completely different structure from SettingsApp (on master) and directly copying elements resulted in total style breakage.

Took me two hours to add an entry with the arrow (facepalm). Will proceed to get the next panel added.

Comment 2

[John Lu [:mnjul][Please NI or I'll miss]]

I'm pushing the target milestone one sprint back because I'm not sure how much time I can devote into this bug when I'm at Portland (according to Tim, it's probably not very much?) The ETA is the first week of the sprint.

Comment 3

[John Lu [:mnjul][Please NI or I'll miss]]

Some quick sitrep: I've implemented most UI under this sub-engineering-bug, except for the deletion of a word, which I'll have to incorporate a <gaia-confirm> (according to Arthur), and which I haven't looked into.

After I get that settled down, I will ask Omega for UX feedback and also for visual specs from him (or maybe Carol) such that things look consistent with other FxOS elements.

Then it's time to write testing and for formal ui-review and review ;)

WIP github branch is at https://github.com/mnjul/gaia/tree/bug_1102831_kb_user_dictionary_settings_ui .

Comment 4

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch (PR @ GitHub)

Alrighty. I have semi-finished (sans tests as usual) for this engineering bug.

Omega, I'd like to invite you to take a look at the UX/visual aspects of the current patch and see if it has met your expectations.

(Again, please note that this engineering sub-bug only adds UI to the keyboard settings and does not provide any real functionality to the keyboard. Adding/removing/editing words should work.)

I have tried to match the style with the building blocks I got from Harly [1] but I believe there are still quite some mismatches. So please kindly point out anything that looks wrong. I would really, really appreciate a (semi-formal) visual spec if you have a lot of (visual) things for me to fix, though.

Again this is not a known V2.2 or V3 feature so if you have time/human-resource constraints, pleaes discuss with me and perhaps Tim/Bruce/Howie.

Feel free to come to me for a demo too!

Thanks aaaaaa lot !

[1] https://developer.mozilla.org/en-US/Apps/Design/Firefox_OS_building_blocks/2.0

Comment 5

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Some comments:
1. When entering the word editor (both adding and editing a word), the input should be focused automatically.
2. When entering/leaving the word editor, the transition should be fade in/out. (See Settings > Internet Sharing > Hotspot Settings for reference.)
3. Visually, the Built-in Keyboard page alignment should move right a bit, and the bgcolor should be gray instead of white. (See Settings for reference.)

Comment 6

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Changed patch according to Omega's comments, incorporated bug 1111482 and modified build scripts to default pref off. Feedback request time!

Note that the styling changes from Omega's comments apply to the whole KBSettings so they'll still be visible even the feature is pref'ed off.

FYI for Tim & Ricky: this patch sits upon unmerged bug 1112460.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

I didn't now load the script to test it and only look at the code -- I think some interface stuff need to be fine tuned, but generally looks good.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Build part looks good to me. I recommend that tests for build script should be added after completing your patch. :)

Comment 9

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Tim, I have revised panel transition & interface between panels, please check. 

(Only ./js/settings/*.js were modified since last feedback, along with some CSS tuning.)

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

This looks good enough -- except the possible transitionend issue. Everything else can be differ to follow-ups.

Comment 11

[John Lu [:mnjul][Please NI or I'll miss]]

Bug 1115649 is for differentiating the concept of panel and dialog; bug 1115644 for implementing the RootPanel.

I'll give priority to bug 1115644 if needed, as we want to give a clear path/methodology for showing user prompts (if any, when we're not at root panel) related to downloadable dictionary.

Comment 12

[John Lu [:mnjul][Please NI or I'll miss]]

And bug 1115666 will clean up the stylesheet mess.

Comment 13

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: PR to gaia-node-modules

Ricky, as offline discussed, please check the change to gaia-node-modules. Thanks!

Comment 14

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Ricky, as offline discussed, please check the added build integration tests. gaia_node_modules.revision will be changed once we merge the gaia-node-modules PR.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8542086 [details] [review]
PR to gaia-node-modules

LGTM. r+

Comment 16

[John Lu [:mnjul][Please NI or I'll miss]]

gaia-node-modules bumped to https://github.com/mozilla-b2g/gaia-node-modules/commit/680b44b119eceea3775f3f99217035f802025d7c

Comment 17

[John Lu [:mnjul][Please NI or I'll miss]]

Changes to gaia-node-modules were reverted at https://github.com/mozilla-b2g/gaia-node-modules/commit/a95745e9cc9bffa6f54d861be0dc5caa75b56228 due to jsdom's dependency (contextify) can't be g++-compiled on TBPL.

Comment 18

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: PR again to gaia-node-modules

Alright, Ricky, please check the PR to gaia-node-modules for jsdom-nogyp.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8542399 [details] [review]
PR again to gaia-node-modules

Since there is a g++ compiling in jsdom package, we change to jsdom-nogyp in order to solve such issue. Let's land it!

Comment 20

[John Lu [:mnjul][Please NI or I'll miss]]

Sitrep: The new jsdom-nogyp works to the purpose of testing building keyboard's settings. Some minor broken tests of Gij and Gu have been fixed. Unit tests for KBSettingsApp and PanelController have been finished (the latter being the most difficult one among all tests to write) as we have to deal with MockPromise...). Onto the remaining UserDictionary/ListPanel/EditPanel unit tests.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Almost of build parts look good to me. I leaved some comments on Github, please fix them before landing your patch. Thanks!

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Merged.

https://github.com/mozilla-b2g/gaia/commit/86b882eed6d520c94d0926846a9987a6526af90d

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Sorry I mess up here. Reopen this bug again.

Comment 24

[John Lu [:mnjul][Please NI or I'll miss]]

gaia-node-modules bumped to https://github.com/mozilla-b2g/gaia-node-modules/commit/fd589b7e8739da3839efc25c77e35e22d5728da7

Comment 25

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

Alright. Remaining unit tests were fixed. Issues raised by Ricky were addressed. Let's get the patch reviewed.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8535448 [details] [review]
Patch (PR @ GitHub)

This looks good except the CloseLockManager interface. I am going to attach a follow-up commit and land this in another pull request.

Comment 27

[GitHub Autolander Bot]

Attachment: [PullReq] timdream:keyboard-user-dict to mozilla-b2g:master

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/987c76c002a716a6787bc0576f0dd0f25fae25e4

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=2b38d7ea7b02

Comment 29

[Francesco Lodolo [:flod]]

Any advice on how to test this?

Comment 30

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to Francesco Lodolo [:flod] from comment #29)
> Any advice on how to test this?

You need a special build flag |GAIA_KEYBOARD_ENABLE_USER_DICT=1| when you build Gaia. A new entry appears in Built-in Keyboard's Settings. UX spec is at bug 983043, FYI.

Comment 31

[Francesco Lodolo [:flod]]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #30)
> You need a special build flag |GAIA_KEYBOARD_ENABLE_USER_DICT=1| when you
> build Gaia. A new entry appears in Built-in Keyboard's Settings. UX spec is
> at bug 983043, FYI.

Thanks, finally able to test, no issues found on the l10n bits.