1079748 - [System2] Implement HierarchyManager

Status: RESOLVED FIXED

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

Description

[(inactive after 6/18) Alive Kuo [:alive]]

The hierarchyManager will provide an interface for modules to compete for hierarchy.

hierarchyManager.addHierachy(module);
hierarchyManager.removeHierarchy(module);

The module who wants to join the competition should:
* Provide an unique priority
* Provide the isActive function
* Dispatch active/inactive events when active state is changed

Any module who wants to get focus should do:

System.request('focus', module);

In the future hierarchyManager will take place of visibilityManager.
Or visibilityManager should simply use the result of hierarchyManager.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: WIP before bug 1072757 landed

Lemme know how you feel with it.

Comment 2

[Etienne Segonzac (:etienne)]

Comment on attachment 8501629 [details] [review]
WIP before bug 1072757 landed

Not sure I'm a good reviewer for this since I know very little about the visibility manager and I don't know well the pain points you're trying to solve with this refactoring.

Happy to discuss and learn but it will take a fair amount of time so you might want to redirect :)

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #2)
> Comment on attachment 8501629 [details] [review]
> WIP before bug 1072757 landed
> 
> Not sure I'm a good reviewer for this since I know very little about the
> visibility manager and I don't know well the pain points you're trying to
> solve with this refactoring.
> 
> Happy to discuss and learn but it will take a fair amount of time so you
> might want to redirect :)

It's fine, let me try to convince you at first.

HierarchyManager is designed to
* Provide the information of current top most UI which includes any appWindow or systemDialog
* Notify the bottom UI with events when there is a higher priority UI takes the focus.
* Response requests from different UI which wants to know if it could be focused.
* Extendable interface to watch new modules.

The pain we want to resolve:
## Accessibility issue
Now if there is a module who has the higher z-order which is active, we need to "ally-hide" all the UI which has the lower z-order than the top most UI.
In the very beginning this was done by adding class to #screen element.
For example:

#screen.homescreenShow.taskManagerShown.utilityShown.attentionShown #systemDialog {
  visibility: hidden;
}

The way we were doing is non maintainable - any new z-ordering element added will make you rewrite these rules and easy to make mistakes. Also sometimes we don't want to "visually" hide the bottom UI but still show it with aria-hidden=true, and this needs some javascript manipulation
## Non-extendable VisibilityManager
Now we are talking about this. The javascript manipulation to turn on/off aria-hidden is here;
but again now it's in a bad shape to grow; every time a new module is added, we need to modify visibility manager again. That's what I said as 'non-extendable'.
## Focus issue
If AppWindow wants to be focused, it doesn't need to know which UI has more z-order then it but only request to focus to HierarchyManager.

AppWindow#requestFocus->AppWindowManager->HierarchyManager(check the hierarchy)=>grant or dismiss the request.

Does this convince you?

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3)
> (In reply to Etienne Segonzac (:etienne) from comment #2)
> > Comment on attachment 8501629 [details] [review]
> > WIP before bug 1072757 landed
> > 
> > Not sure I'm a good reviewer for this since I know very little about the
> > visibility manager and I don't know well the pain points you're trying to
> > solve with this refactoring.
> > 
> > Happy to discuss and learn but it will take a fair amount of time so you
> > might want to redirect :)
> 
> It's fine, let me try to convince you at first.
> 
> HierarchyManager is designed to
> * Provide the information of current top most UI which includes any
> appWindow or systemDialog

So in the future what I want to see is,

HierarchyManager ===> *WindowManager =====> *Window
                      *DialogManager =====> *Dialog

* HierarychyManager will watch a list of modules which wants to compete for the z-order.

* Each WindowManager/DialogManager is inherited from BaseModule which has
isActive: if there is an active child shown
focus: focus the active child
setAccessibility: set accessibility of the active child
HIERARCHY_PRIORITY: the pre-defined z-ordering of this module

* Each Window/Dialog is inherited from BaseUI which has
setAccessibility(true|false): being called by its manager to ally-hide/show the element
requestFocus(true|false): being called if it wants to get the focus like https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_transition_controller.js#L280-L293

Whenever there is a new module who wants to compete for the z-order is added to the system,
it should call
|System.request('addHierarchy', this);| with given hierary_priority which comes from z-index.

After that, hierarychyManager will automatically notify you if the top most UI is changed and you are/are not the top most UI which could gain the accessibility/focus so you could do something to hide/show your UI or blur/focus.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3)
> ## Non-extendable VisibilityManager
> Now we are talking about this. The javascript manipulation to turn on/off
> aria-hidden is here;
> but again now it's in a bad shape to grow; every time a new module is added,
> we need to modify visibility manager again. That's what I said as
> 'non-extendable'.

Lemme explain this by example.

Now we have taskManager/utilityTray/AttentionWindow/LockscreenWindow/Rocketbar/systemDialog.... to affect app accessibility/focus.

All of them are having different interface to know its state;
LockscreenWindowManager.states.active;
AttentionWindowManager.hasActiveWindow;
SystemDialogManager.states.active;
rocketbar.active....

Think about we have 20, 30, 40 this kind of new UI who wants to know its the top most or not. We will have codes like

// handle Module1 show/focus request
if (ModuleA.states.active || ModuleB.active || ModuleC.shown()) {
  Module1.hide();
  Module2.hide();
  Module3.hide();
  ....
} else {
  Module1.show();
  Module2.hide();
  Module3.hide();
}

This isn't a good pattern to go long way.
If all modules have the same interface then the manager could manage them easily.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Hey Eitan/Yura,
I forget who I had discussed the idea to.
Does this sound a good idea to you guys to deal with accessibility?

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

What's more:
The priority mechanism could be applied to home event issues.
We are now relying on the addEventListenr(start) order to make sure the home button could be stopped by somebody; but this will fail if the modules are loaded and started without specific order in the future.
And honestly it's hard to maintain the start order because you don't know if you insert a new module between old modules the home event will break or not.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #6)
> Hey Eitan/Yura,
> I forget who I had discussed the idea to.
> Does this sound a good idea to you guys to deal with accessibility?

I think if this is to become a central place of keeping track of the hierarchy stack it should help with our a11y visibility issues.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #8)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #6)
> > Hey Eitan/Yura,
> > I forget who I had discussed the idea to.
> > Does this sound a good idea to you guys to deal with accessibility?
> 
> I think if this is to become a central place of keeping track of the
> hierarchy stack it should help with our a11y visibility issues.

Agreed. Especially if this has good test coverage.

Comment 10

[Etienne Segonzac (:etienne)]

Thanks a lot, this is much clearer. I understand the pain points now and I even know some of them first hand from the AttentionWindows and Rocketbar work. So I should be able to do the review.
It might become even more useful with the current ongoing `mozbrowser.setActive` work...

A few points I'd like to discuss:
* for the "unique priority" to work we may need to maintain the priority list inside the hierarchy manager. I remember the days before your work on zindex.css and it wasn't pretty :) having everything in one file was a huge help!

* how about keeping the register/unregister semantic instead of addHierarchy/removeHierarchy? (bike-shedding a bit :))

* with more and more dialogs moving inside the AppWindow, I hope we actually won't need a DialogManager because instead we'll just have 2-3 "dialog modules" competing in the hierarchy individually

I also think we should hack the "User Story" field and use it to keep a comprehensive view of the project, especially if this ends up being a meta bug. I'll take a stab at it (to verify my understanding), feel free to edit it often :) My goal is to have all the current use-cases in mind while working on this.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Thanks a lot, this is much clearer. I understand the pain points now and I
> even know some of them first hand from the AttentionWindows and Rocketbar
> work. So I should be able to do the review.
> It might become even more useful with the current ongoing
> `mozbrowser.setActive` work...
> 
> A few points I'd like to discuss:
> * for the "unique priority" to work we may need to maintain the priority
> list inside the hierarchy manager. I remember the days before your work on
> zindex.css and it wasn't pretty :) having everything in one file was a huge
> help!

Ya, that was something I am not sure where to go;
if we put the priority in the hierarchy manager we should use a key to identify, maybe module.name.

> 
> * how about keeping the register/unregister semantic instead of
> addHierarchy/removeHierarchy? (bike-shedding a bit :))

Do you mean

System.request('HierarchyManager:register', this);
System.request('HierarchyManager:unregister', this);

?
> 
> * with more and more dialogs moving inside the AppWindow, I hope we actually
> won't need a DialogManager because instead we'll just have 2-3 "dialog
> modules" competing in the hierarchy individually
> 

Maybe, in the long run we will only have system-dialogs and windows.
But as you know, we won't know when UX would like to add one more new dialog which behaves differently from system dialog and in-app dialog. I designed this just for the flexity.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11)
> (In reply to Etienne Segonzac (:etienne) from comment #10)
> > Thanks a lot, this is much clearer. I understand the pain points now and I
> > even know some of them first hand from the AttentionWindows and Rocketbar
> > work. So I should be able to do the review.
> > It might become even more useful with the current ongoing
> > `mozbrowser.setActive` work...
> > 
> > A few points I'd like to discuss:
> > * for the "unique priority" to work we may need to maintain the priority
> > list inside the hierarchy manager. I remember the days before your work on
> > zindex.css and it wasn't pretty :) having everything in one file was a huge
> > help!
> 
> Ya, that was something I am not sure where to go;
> if we put the priority in the hierarchy manager we should use a key to
> identify, maybe module.name.
> 

I am a little worried about this scenario;
in the future if any new module wants to join the top-most competition,
we should modify hierarchy manager as well as the new module to register/unregister at the same time.
It is not that intuitive; however this is what we have for z-index right now :|

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: HierarchyManager, v2

Lemme know what you think to this one.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8509277 [details] [review]
HierarchyManager, v2

Almost too easy :) definite f+, we can move forward and add unit tests for the new code in each module (hopefully we'll find something good to extract in the system mock)

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

On it right now!

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

FYI,
I found that initlogo is not in a good shape and its coverage is poor;
we need to work it with system 2 format someday after this patch.

The bug tracking is bug 971505

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

Status update: working on unit tests and almost done.
Need to check bug 1088513 is resolved with the patch.

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8509277 [details] [review]
HierarchyManager, v2

V3
* Use setHierarchy instead of setAccessibility or focus in manager. It should be the responsibility of the module who competes hierarchy decides what to do when hierarchy is changed.
* Introduce getTopMostWindow() to HierarchyManager to integration all WindowManager information.

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

Note: With this patch when rocketbar + keyboard is on, and press close button of rocketbar, the keyboard will stay with foreground active app if it's having a input focused before opening rocketbar. And the problem is when type with the keyboard the key does not appear in the text input.

Not sure but very likely a gecko input method bug.

Tim said he will look when he has time. Opening a dependency bug.

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8509277 [details] [review]
HierarchyManager, v2

First, I'm really happy with the unit tests :)
Second, since the goal for this patch is to do arbitrage _between_ modules, it's begging for integration testing :)

Here's the marionette-js test I'd like to see:
* open an app
* assert on the topMostUI + aria-hidden of the app browser element
* open the utilitytray (we already have an helper)
* assert the new topMostUI + changed aria-hidden of the app browser element

and the same one with the rocketbar

Thanks!

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8509277 [details] [review]
HierarchyManager, v2

Hope you are happy with this!

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 8509277 [details] [review]
HierarchyManager, v2

Thanks for being so exhaustive with the marionette tests!

r=me with the (important) comments on github addressed.
Please re-trigger the Gij slice where this test ends up a few times before landing to make sure we don't have any intermittents.

(and feel free to ask for review again if you end up needing a fair amount of changes to make the systemdialog test work)

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

Travis is happy; I found we have more to do for fxa account to enhance it, will open followup.

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/commit/8ca903fe7b463a6974186e19d5ba667a1d59f02d