1096133 - [System2] Get rid of stopPropagation for certain hierarchy related events

Status: RESOLVED FIXED

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

Description

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

Issues:
Currently we are relying on addEventListener to register home event; this is not a problem until some of the modules want to interrupt the home event. The interrupt is based on the evt.stopPropagation, but that depends on the ordering of addEventListener between different modules.
It's why some of the ordering of the modules in bootstrap cannot be changed. It's hard to maintain the ordering and it's impractical when we start to lazy load because we cannot control the loading ordering. Even we could, it doesn't make sense to.

A proposed solution is to re-use the same mechanism of hierarchy manager:
* Module should registerHome/unregisterHome when needed.
* Module should specify if it needs to interrupt the home event.
* Module should specify its priority if it wants to interrupt the home event.
* Module without 'interruptHome' property will get home event anyway.

Comment 1

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

Attachment: https://github.com/mozilla-b2g/gaia/pull/25989

https://github.com/mozilla-b2g/gaia/pull/25989

I end up re-using current hierarchy manager instead of creating a new module list watcher.
* If any module wants to compete:
  'home', 'holdhome', 'system-resize', 'inputmethod-contextchange', 'launchactivity', 'webapps-launch'
  it should join the competition of hierarchy because all of these are hierarchy-related events and will be processed by only top most.
* A module could still listen to those events if it does not want to interrupt others by stopImmediatePropagation. 

What's undetermined:
* Fix webapps-launch compete in this patch (HomeWM v.s. AppWM)
* Do we want to use _handle_ semantic? It's a little confusing since the module does not specify them in this.constructor.EVENTS; another proposal is using
module.broadcast(evt);

What will be done in next if f+:
* Integration tests


I think the code explain itself well but let me know if you have problems.

Comment 2

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

Comment on attachment 8519756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25989

v2: Ready for review.

What will not be fixed:
* webapps-launch/open-app is interrupted by HomescreenWindowManager before AppWindowFactory if the target app is homescreen. I don't have a good idea on how to deprecate stopImmediatePropagation in that case. But they are all belong to mozApps subsystem so I don't worry too much about their startup sequence. Maybe.

What is changed:
* Broadcast is used to handle the hierarchy event. Not sure if there is a better name: handle() or deal() or emit() ...
* TaskManager is demoted to bottom most because FtuLauncher(which belongs to AppWindowManager) needs to interrupt holdhome/home event.
* AppWindowManager publishes activated/deactivated but there is little difference from other module. It needs to know the state of the task manager to publish deactivated. Not good enough, to fix this we need another run of AppWindowManager destruction. I again don't worry about this too much because taskManager will become appWindowManager's submodule in the future.
* Integration test for home/holdhome/resize/value-selector/ftu.

Comment 3

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

Comment on attachment 8519756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25989

FTGM. Nice clean up.

Comment 4

[Etienne Segonzac (:etienne)]

Comment on attachment 8519756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25989

Impressive work! (made one comment on github)

2 things:
* I'd like to push a little and try landing with all the integration tests (even those currently skipped) (I'm willing to help!)
* I thought about naming a lot. HierarchyManager#broadcast is perfectly fine but while you're reading the code of a module, the broadcast method looks really weird: it does not broadcast anything and the return value is hard to grasp.

Some suggestions by order of preference:
- respondToHierarchyEvent()
- handleAndContinue()
- handleHierarchyEvent()
- hierachyHandler()
- ...

The goal is: somebody reading rocketbar.js should easily understand that this is for events from the hierarchy manager and that the propagation can the stopped.

Cheers!

Comment 5

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

Comment on attachment 8519756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25989

Revised

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 8519756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25989

Very cool thanks!
(I think we can get the last test to pass, feel free to flag for review again if this requires big changes).

Comment 7

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

I had overcomed the next button issue, but after rebasing, in keyboard resize test, the input in attention window makes keyboard hide right away. Still investigating.

Comment 8

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

I decided to skip the keyboard test on attention window (and the fact is we have no this kind of use case now). Opening followup to enable it.

Comment 9

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

https://github.com/mozilla-b2g/gaia/commit/eca5c0478c4969b8790b2df9f702855cdd617414