1044525 - Keep DOMElement handling inside render.js and use key objects for business logic

Status: RESOLVED FIXED

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

Description

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

Currently, UserPress, ActiveTargets, TargetHandlersManager, and TargetHandlers are all using the actual DOM element to handle user actions. It's a unnecessary exposure of objects in the view (render.js) to our business logic controls, and we essentially forcing render.js to copy properties to the DOM element datasets too (and using non-trival logic decide what to copy along the way).

I propose we should handle the key objects (the { value: .., keyCode: ... }'s in layouts) in business logic. LayoutManager should also normalize these object to a point where render.js don't need a lot of extra logic to decide what to render.

We probably won't be able to handle hardware key presses without dealing with this bug first unless we want to create fake dom elements to represent these user presses.

Marking dependency to bug 1011482 because it deals with normalizing keys in alt char menu to objects.

I actually wonder if we want to wrap these objects into instances of some class like "Keys" and "normalize" the properties there.

Comment 1

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

A mapping between DOM elements and key objects can be created in render.js with WeakMap -- it make no sense not to use it!

Comment 2

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

I will look into this and might take over bug 1011482 if necessary.

Comment 3

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

Sitrep: Got a WIP here: https://github.com/mnjul/gaia/tree/bug_1044525_kb_keep_dom_element_at_renderjs .
Two things are not yet ready:

1. Normalization -- there is no normalization as said in comment 0 right now. This is my next step. This will also deal with confusing 'keyCode' and 'keycode' properties. Additionally, I'm currently duplicating key objects in renderer (for fail-safe). This is probably unnecessary as we'll be doing normalization in LayoutManager soon.

However I tend to do this after bug 1047837 lands as it modifies LayoutManager quite a bit too.

2. Incorporate bug 1047837 once it's landed.

Comment 4

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

3. Early-convert 'target' into key objects during event handling path, where possible.

Comment 5

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

Attachment: WIP Patch (GitHub Branch)

Hi Tim,

Requesting your feedback on the structures of my WIP (implementation done, but no tests yet). I think things are quite self-explanatory ...to me. So if they're not, feel free to shoot me questions and/or suggestions.

I kinda want to defer coalescing |key.keycode| and |key.keyCode| to a follow-up bug, as that probably would complicate this patch too much (and again I'm not too sure if it always holds that |key.keyCode === keyChar.charCodeAt(0)| at the moment.

Thanks for your time!

Comment 6

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

Could you simply submit a pull request so I could make line comments on Github?

Comment 7

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

Attachment: WIP Patch (PR @ GitHub)

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #6)
> Could you simply submit a pull request so I could make line comments on
> Github?

Aha, here you go.

Comment 8

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

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

So far looks good, you have found most of the places that needs to be changed for this bug.

I am a bit worry about the scope of this bug -- we have many patches in flight that will conflict with this patch. I got bug 1051219, and I believe Rudy have two. We can talk about the landing order tomorrow.

Comment 9

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

My inflight patch would be this one, bug 1065961 and I hope to get it to be reviewed again in a day or two.
Thanks.

Comment 10

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

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

Alright. Things have been ironed out.

Tim & Rudy, please if you will see if things are going in correct direction. Tests will be added & fixed when the patch's architecture is quite stabilized.

Comment 11

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

And sorry for gun-jumping when I set [p=1]. At current scale this bug is probably worth at least 3 story points supposing tests are not hard to add/fix.

Comment 12

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

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

This looks good to me.
Also leave some comments on GH.

Thanks.

Comment 13

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

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

Reviewed offline and commented.

Comment 14

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

I have addressed most comments and updated my PR to see how integration tests fail and what to fix for those failing tests. Once tests are fixed I'll be requesting review.

Will also be filing follow-up bugs soon.

Comment 15

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

Sitrep: Rebased rudy's work at bug 1065961, and fixed Gip tests (by re-assigning needed dataset properties). Ongoing is fixing unit tests now (and will need to add new tests).

Fixing tests for render.js was specifically problematic as we've actually had requestAnimationFrame in IMERender [1] that was not stubbed in the tests, and I took some time to make sure that was properly addressed. It surprises me that it's been stable enough on TBPL and no one has ever raised the issue.

[1] https://github.com/mnjul/gaia/blob/bug_1044525_kb_keep_dom_element_at_renderjs_with_unittest/apps/keyboard/js/render.js#L661

WIP with unit tests is on branch https://github.com/mnjul/gaia/tree/bug_1044525_kb_keep_dom_element_at_renderjs_with_unittest .

Comment 16

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

Alright, I have amended most unit tests aside from that for LayoutManager as bug 1070487 generated some conflicts (which are not hard to fix; just many layout checkings need to be changed). I'm quite confident to get this thing delivered for formal review early next week.

Comment 17

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

Attachment: Patch with Unit Tests (PR @ GitHub)

Comment 18

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

Comment on attachment 8495863 [details] [review]
Patch with Unit Tests (PR @ GitHub)

Fixed all those LayoutManager tests. Tim & Rudy, please review the patch. Thanks!

Comment 19

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

Comment on attachment 8495863 [details] [review]
Patch with Unit Tests (PR @ GitHub)

Looks like the patch is ready except for a few nits!

Comment 20

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

Comment on attachment 8495863 [details] [review]
Patch with Unit Tests (PR @ GitHub)

f+ as offline discussion.
Please help look after "highlighting the last alt char" when you're touching the empty area of a 2-row alt char menu.

Thank you.

Comment 21

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

Discussed with Rudy and the feature said in comment 20 is already broken in current master (albeit in different manner). So I will be landing this huge patch once testing is done at https://tbpl.mozilla.org/?rev=f8df78850e81f2199461da5833c3e3990f651338&tree=Gaia-Try. I'll keep track of opening follow-up bugs.

Comment 22

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

Alright. Master: https://github.com/mozilla-b2g/gaia/commit/afeb7316826fb2172070e27632270224bc2ced13

Comment 23

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

Just some recap:

From this bug and onwards, Our desired flow from user interaction ("raw" press event) onto target resolution and onto actual handling logics is: we should always try to resolve DOM elements [that user is interacting with] into target key objects as soon as possible; anything that doesn't have a target key object (which means it has no intended user-interactivity) should not propagate downstream.

Downstream handling logics should always try to handle key objects, with the exception of views, which are allowed to retrieve "visual elements" (I hope I have a more precise definition on this...) from the reverse map. This bug has not dealt with this concept very clear-cut, and we'll manage remaining architecture issues in follow-up bugs.

Comment 24

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

And I'll be drafting & filing follow-up bugs today.

Comment 25

[howie [:howie]]

Remove the feature b2g tag to match the new v2.2 scope.