[UserDictionary] Implement user dictioanry UI to KeyboardSettingsApp

RESOLVED FIXED in 2.2 S4 (23jan)

Status

Firefox OS
Gaia::Keyboard
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mnjul, Assigned: mnjul)

Tracking

(Blocks: 1 bug)

unspecified
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=3])

Attachments

(3 attachments, 1 obsolete attachment)

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.
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.
Assignee: nobody → jlu
Status: NEW → ASSIGNED
Whiteboard: [p=2]
Target Milestone: --- → 2.2 S1 (5dec)
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.
Target Milestone: 2.2 S1 (5dec) → 2.2 S2 (19dec)
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 .
Target Milestone: 2.2 S2 (19dec) → ---
Created attachment 8535448 [details] [review]
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
Attachment #8535448 - Flags: feedback?(ofeng)
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.)
Attachment #8535448 - Flags: feedback?(ofeng)
Depends on: 1111482
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.
Attachment #8535448 - Flags: feedback?(timdream)
Attachment #8535448 - Flags: feedback?(ricky060709)
Whiteboard: [p=2] → [p=3]
Target Milestone: --- → 2.2 S3 (9jan)
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.
Attachment #8535448 - Flags: feedback?(timdream) → feedback+
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. :)
Attachment #8535448 - Flags: feedback?(ricky060709) → feedback+
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.)
Attachment #8535448 - Flags: feedback+ → feedback?(timdream)
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.
Attachment #8535448 - Flags: feedback?(timdream) → feedback+
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.
Target Milestone: 2.2 S3 (9jan) → 2.2 S4 (23jan)
And bug 1115666 will clean up the stylesheet mess.
Created attachment 8542086 [details] [review]
PR to gaia-node-modules

Ricky, as offline discussed, please check the change to gaia-node-modules. Thanks!
Attachment #8542086 - Flags: review?(ricky060709)
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.
Attachment #8535448 - Flags: review?(ricky060709)
Comment on attachment 8542086 [details] [review]
PR to gaia-node-modules

LGTM. r+
Attachment #8542086 - Flags: review?(ricky060709) → review+
gaia-node-modules bumped to https://github.com/mozilla-b2g/gaia-node-modules/commit/680b44b119eceea3775f3f99217035f802025d7c
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.
Created attachment 8542399 [details] [review]
PR again to gaia-node-modules

Alright, Ricky, please check the PR to gaia-node-modules for jsdom-nogyp.
Attachment #8542086 - Attachment is obsolete: true
Attachment #8542399 - Flags: review?(ricky060709)
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!
Attachment #8542399 - Flags: review?(ricky060709) → review+
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 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!
Attachment #8535448 - Flags: review?(ricky060709) → review+
Merged.

https://github.com/mozilla-b2g/gaia/commit/86b882eed6d520c94d0926846a9987a6526af90d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Sorry I mess up here. Reopen this bug again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
gaia-node-modules bumped to https://github.com/mozilla-b2g/gaia-node-modules/commit/fd589b7e8739da3839efc25c77e35e22d5728da7
Status: REOPENED → ASSIGNED
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.
Attachment #8535448 - Attachment description: WIP Patch (PR @ GitHub) → Patch (PR @ GitHub)
Attachment #8535448 - Flags: review?(timdream)
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.
Attachment #8535448 - Flags: review?(timdream) → review+

Comment 27

3 years ago
Created attachment 8544983 [details] [review]
[PullReq] timdream:keyboard-user-dict to mozilla-b2g:master
master: https://github.com/mozilla-b2g/gaia/commit/987c76c002a716a6787bc0576f0dd0f25fae25e4

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=2b38d7ea7b02
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Any advice on how to test this?
(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.
(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.
You need to log in before you can comment on or make changes to this bug.