Closed Bug 1079748 Opened 6 years ago Closed 6 years ago

[System2] Implement HierarchyManager

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: alive, Assigned: alive)

References

Details

User Story

The hierarchyManager will manage the competition between modules with "top level" UI elements.

hierarchyManager.registerHierarchy(module);
hierarchyManager.unregisterHierarchy(module);

The module responsible for displaying "top level" UI elements to the user should:
* Implement the isActive function
* Dispatch activated/deactivated events when active state changes
* (Optional) Implement getActiveWindow if the module is a window manager
* (Optional) Implement setHierarchy which will be invoked by hierarchy manager when the top most module is changed.

Any module who wants to get focus should do:

System.request('focus', module);
If you are the top most module, HierarchyManager will then invoke module.setHierarchy(true). Otherwise the request will just be ignored.

The hierarchyManager will replace the visibilityManager, including the a11y and audio-channel support.

== List of competing modules ==
* AttentionWindowManager
* SecureWindowManager
* LockScreenWindowManager
* AppWindowManager
* UtilityTray
* SIM PIN Dialog
* Dev tools Dialog
* Rocketbar / Search
* Cards View
* Bluetooth Pairing Dialog ?

Attachments

(2 files)

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.
Assignee: nobody → alive
Lemme know how you feel with it.
Attachment #8501629 - Flags: feedback?(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 :)
Attachment #8501629 - Flags: feedback?(etienne)
(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?
Flags: needinfo?(etienne)
(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.
(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.
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?
Flags: needinfo?(yzenevich)
Flags: needinfo?(eitan)
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.
(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.
Flags: needinfo?(yzenevich)
(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.
Flags: needinfo?(eitan)
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.
User Story: (updated)
Flags: needinfo?(etienne)
(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.
(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 :|
User Story: (updated)
Attached file HierarchyManager, v2
Lemme know what you think to this one.
Attachment #8509277 - Flags: feedback?(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)
Attachment #8509277 - Flags: feedback?(etienne) → feedback+
On it right now!
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
Status update: working on unit tests and almost done.
Need to check bug 1088513 is resolved with the patch.
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.
Attachment #8509277 - Flags: review?(etienne)
Duplicate of this bug: 1088513
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.
User Story: (updated)
Depends on: 1092549
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!
Attachment #8509277 - Flags: review?(etienne)
blocking-b2g: --- → backlog
Comment on attachment 8509277 [details] [review]
HierarchyManager, v2

Hope you are happy with this!
Attachment #8509277 - Flags: review?(etienne)
Duplicate of this bug: 943188
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)
Attachment #8509277 - Flags: review?(etienne) → review+
Travis is happy; I found we have more to do for fxa account to enhance it, will open followup.
https://github.com/mozilla-b2g/gaia/commit/8ca903fe7b463a6974186e19d5ba667a1d59f02d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.