1100792 - Re-organize the view hierarchy of keyboard

Status: RESOLVED FIXED

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

Description

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

We should separate the CandidatePanel rendering part out of IMERender for better structure.

Comment 1

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

Attachment: Patch V1

Hi Tim,

This patch took much more time than I thought, since it has to cover multiple layouts, like zhuyin, pinyin, and latin layouts, and Vietnamese related layouts.
 - I tried to handle this with 2 classes of CandidatePanelView.

The tests are not complete yet, however, please help give an early feedback, if you can help.

Thanks.

Comment 2

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

Comment on attachment 8541155 [details] [review]
Patch V1

I think we need to sort out the view hierarchy here. I would imagine we should do

Keyboard
- Layout
-- Candidate Panel
-- Page
--- AltMenu
--- Key

But I don't see Layout in this patch. Please tell me your intention here...

Comment 3

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

Right now the draw() in render.js would accept a page as an parameter, so we would have the following structures so far,
 Keyboard
   - LayoutPageView
   -- CandidatePanel (should be moved out)
   -- Key
   - Alt Menu (since we got only one alt menu for now)

And I would like to make it look like this in a follow-up bug,
 Keyboard
   - Candidate Panel (Moved out)
   - LayoutPageView
   -- Key
   - Alt Menu (since we got only one alt menu for now)

It seems to make the hierarchy clearer by adding "Layout" layer to aggregate the candidate panel and pages, though I am not sure if there are other reasons it is a must-have.

Comment 4

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

I'll change the summary of this bug, since we may want to re-org the view hierarchy, instead of just moving candidate panel out.

Comment 5

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

Comment on attachment 8541155 [details] [review]
Patch V1

As our offline discussion, this patch is aiming at making the code sync with the DOM structure, so the LayoutPageView would contain candidatePanelView.

Tim, could you please help to give a second look on this?
Thanks!

Comment 6

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

Comment on attachment 8541155 [details] [review]
Patch V1

We are on the right direction here.

The main problem here is that you are trying to pass ad-hoc objects into registerView() but you should really pass a View class instance. Please correct there calls.

The other issues are cosmetic.

Comment 7

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

Comment on attachment 8541155 [details] [review]
Patch V1

The patch has been updated to address the review comments.
 - Create BaseView and let other simple views to inherit from it.
 - Move fitText() and getScale() to ViewUtils since they should be a general util functions.

Tim, could you please help review this change?
Thank you.

Comment 8

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

Comment on attachment 8541155 [details] [review]
Patch V1

Getting close! We should be able to land this in the final review :)

Comment 9

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

Comment on attachment 8541155 [details] [review]
Patch V1

Tim, 

Patch updated.

Could you please review again?
Thanks for your advice and patience.

Comment 10

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

Comment on attachment 8541155 [details] [review]
Patch V1

Nice! Thanks for the hard work.

Comment 11

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

With this change, I am facing a test failure here,
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=deff4a411790

For latin candidate panel,

The original DOM structure is:
  <div role="presentation">
   <span role="option" class="autocorrect"> candidate 1 </span>
  </div>

  <div role="presentation">
   <span role="option"> candidate 2 </span>
  </div>


And I want to change it to: 
 <div role="presentation" class="autocorrect">
   <span role="option"> candidate 1 </span>
  </div>

  <div role="presentation">
   <span role="option"> candidate 2 </span>
  </div>

i.e. move the "autocorrect" to the parent div element.
Note that I also made the span "pointer-events: none" so that in the code we would get the <div> as the touch event target.

Eitan, since you're the original author, could you help shed some light on how we're going to get the tests right?
Thanks.

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to Rudy Lu [:rudyl] from comment #11)
> With this change, I am facing a test failure here,
> https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=deff4a411790
> 
> For latin candidate panel,
> 
> The original DOM structure is:
>   <div role="presentation">
>    <span role="option" class="autocorrect"> candidate 1 </span>
>   </div>
> 
>   <div role="presentation">
>    <span role="option"> candidate 2 </span>
>   </div>
> 
> 
> And I want to change it to: 
>  <div role="presentation" class="autocorrect">
>    <span role="option"> candidate 1 </span>
>   </div>
> 
>   <div role="presentation">
>    <span role="option"> candidate 2 </span>
>   </div>
> 
> i.e. move the "autocorrect" to the parent div element.
> Note that I also made the span "pointer-events: none" so that in the code we
> would get the <div> as the touch event target.
> 
> Eitan, since you're the original author, could you help shed some light on
> how we're going to get the tests right?
> Thanks.

Hi Rudy,

I'm not 100% sure I understand how the test failure is related. But I could definitely give some input on how to update that markup in a way that keeps it accessible: Swap the roles between the div and span. 
 <div role="option" class="autocorrect">
   <span role="presentation"> candidate 1 </span>
  </div>

  <div role="option">
   <span role="presentation"> candidate 2 </span>
  </div>

This will keep it in sync with your intended touch target, and generally things should be good. The presentation role on the span might even be redundant.

Comment 13

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

Eitan, thanks for the valuable advice, now I can get the tests to pass!


Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/d6232cd6d099e53c90f4dd581e8963d65b8fec80