Closed Bug 1044525 Opened 10 years ago Closed 10 years ago

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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S6 (10oct)

People

(Reporter: timdream, Assigned: mnjul)

References

Details

(Whiteboard: [p=5])

Attachments

(1 file, 2 obsolete files)

Patch with Unit Tests (PR @ GitHub)
(deleted), text/x-github-pull-request
timdream
: review+
rudyl
: feedback+
Details 
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.
A mapping between DOM elements and key objects can be created in render.js with WeakMap -- it make no sense not to use it!
I will look into this and might take over bug 1011482 if necessary.
Assignee: nobody → jlu
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.
3. Early-convert 'target' into key objects during event handling path, where possible.
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S5 (26sep)
Attached file WIP Patch (GitHub Branch) (obsolete) (deleted) — 
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!
Attachment #8489313 - Flags: feedback?(timdream)
Could you simply submit a pull request so I could make line comments on Github?
Flags: needinfo?(jlu)
Attached file WIP Patch (PR @ GitHub) (obsolete) (deleted) — 
(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.
Attachment #8489313 - Attachment is obsolete: true
Attachment #8489313 - Flags: feedback?(timdream)
Attachment #8489388 - Flags: feedback?(timdream)
Flags: needinfo?(jlu)
Comment on attachment 8489388 [details]
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.
Attachment #8489388 - Flags: feedback?(timdream) → feedback+
Flags: needinfo?(rlu)
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.
Flags: needinfo?(rlu)
Comment on attachment 8489388 [details]
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.
Attachment #8489388 - Flags: feedback?(timdream)
Attachment #8489388 - Flags: feedback?(rlu)
Attachment #8489388 - Flags: feedback+
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.
Whiteboard: [p=1] → [p=3]
Comment on attachment 8489388 [details]
WIP Patch (PR @ GitHub)

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

Thanks.
Attachment #8489388 - Flags: feedback?(rlu) → feedback+
Comment on attachment 8489388 [details]
WIP Patch (PR @ GitHub)

Reviewed offline and commented.
Attachment #8489388 - Flags: feedback?(timdream) → feedback+
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.
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 .
Whiteboard: [p=3] → [p=5]
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
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.
Attached file Patch with Unit Tests (PR @ GitHub) (deleted) — 

        Attachment #8489388 -
        Attachment is obsolete: true

    
    

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

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

        Attachment #8495863 -
        Attachment description: WIP Patch with Unit Tests (PR @ GitHub) → Patch with Unit Tests (PR @ GitHub)

        Attachment #8495863 -
        Flags: review?(timdream)

        Attachment #8495863 -
        Flags: review?(rlu)

    
    

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

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

        Attachment #8495863 -
        Flags: review?(timdream) → review+

    
    

    
  
Comment on attachment 8495863 [details]
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.

        Attachment #8495863 -
        Flags: review?(rlu) → feedback+

    
    

    
  
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.

    
    

    
  
Alright. Master: https://github.com/mozilla-b2g/gaia/commit/afeb7316826fb2172070e27632270224bc2ced13
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

    
    

    
  
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.

    
    

    
  
And I'll be drafting & filing follow-up bugs today.
Depends on: 1075970
Depends on: 1085384
feature-b2g: --- → 2.2+

    
    

    
  
Remove the feature b2g tag to match the new v2.2 scope.
feature-b2g: 2.2+ → ---

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: