Closed Bug 1096133 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:+, b2g-v2.2 fixed)

RESOLVED FIXED
tracking-b2g +
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: alive, Assigned: alive)

References

Details

Attachments

(1 file)

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.
blocking-b2g: --- → backlog
tracking-b2g: --- → +
Assignee: nobody → alive
Component: Gaia::System::Window Mgmt → Gaia::System
Summary: [System2] Implement HomeEventDispatcher → [System2] Get rid of stopPropagation for certain hierarchy related events
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.
Attachment #8519756 - Flags: feedback?(etienne)
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.
Attachment #8519756 - Flags: review?(timdream)
Attachment #8519756 - Flags: review?(etienne)
Attachment #8519756 - Flags: feedback?(etienne)
Depends on: 1094550
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!
Attachment #8519756 - Flags: review?(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).
Attachment #8519756 - Flags: review?(etienne) → review+
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.
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.
https://github.com/mozilla-b2g/gaia/commit/eca5c0478c4969b8790b2df9f702855cdd617414
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1106666
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.