Bug 916709 (popup-window)

[Window Management] Implement PopupWindow

RESOLVED FIXED in 1.4 S6 (25apr)

Status

Firefox OS
Gaia::System::Window Mgmt
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: alive, Assigned: alive)

Tracking

(Blocks: 2 bugs)

unspecified
1.4 S6 (25apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Need Rework PopupManager.
This blocks v1.3 Browser app to System integration.

Updated

4 years ago
Component: Gaia::System → Gaia::System::Window Mgmt
Blocks: 935260
No longer blocks: 935260
Alias: popup-window
Assignee: nobody → alive
Blocks: 809559
on it.
Created attachment 8382932 [details] [review]
PopupWindow WIP v1

This is WIP patch for PopupWindow:
* Remove PopupManager
* Add PopupWindow
* Layout strategy changed: layout any PopupWindow instance inside it's parent Window. We will do the same thing for ActivityWindow later.
* Enable PopupWindow to have modal dialog/authentication dialog/context menu...
* Enable nested PopupWindow (We could turn this off if we dislike.)

Next to do:
* Use childWindow/parentWindow to represent ActivityWindow, too.(another bug might be.)
* More manual testing.
* Add Unit tests.


Note:
Sorry I haven't finished the testing part but I don't want to frustrate etienne by the length of the patch every time so let's get some feedback at first.
Attachment #8382932 - Flags: feedback?(etienne)
Is it me or is the patch missing |popup_window.js| ? :)
Created attachment 8384494 [details] [review]
v2: Added popupwindow.js

I messed up my branch so here's another one.
Attachment #8382932 - Attachment is obsolete: true
Attachment #8382932 - Flags: feedback?(etienne)
Attachment #8384494 - Flags: feedback?(etienne)
Note: comment 6 is not implemented in v2 yet.
I expected to use a new term for vertical window (including popup and inline activity) and a term for horizontal window (including current child window and window activity).
Comment on attachment 8384494 [details] [review]
v2: Added popupwindow.js

preemptive feedback+ on the parent/child, caller/callee, top/bottom version :)
Attachment #8384494 - Flags: feedback?(etienne) → feedback+
Progress today:
Rename parentWindow/childWindow to previousWindow/nextWindow.
Use rearWindow/frontWindow pair to represent popup.
Enhancing sheet apps to have popup/activity ability to do more tests.
Blocks: 927862
Target Milestone: --- → 1.4 S3 (14mar)
Created attachment 8394040 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/17284

PopupWindow v3
* Implement PopupWindow
* Remove ActivityWindowFactory and let AppWindowManager to do the launch work
* Add ActivityWindowManager
* Remove some duplicated killing app logic
* Enhance ChildWindowFactory

Note this is based on bubble-tea branch because bug 961800 has not been landed to master (running tests) and I will make a master patch later when it's landed.

I know this won't be last round but lots of things has been changed so I ask for review at first. Sorry for taking so long time because I was stuck in blockers all the day.
Attachment #8384494 - Attachment is obsolete: true
Attachment #8394040 - Flags: review?(etienne)
Comment on attachment 8394040 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/17284

Made some comments on github.
Wish I spent more time on the patch but I played with the tests apps too much :)

On PTO next week but I'm sure Vivien will like the multidimensional window manager too.
Attachment #8394040 - Flags: review?(etienne)
Working in progress v4 based on master's 961800 is here:
https://github.com/mozilla-b2g/gaia/pull/17567

Will request a new review round tomorrow.
Created attachment 8397028 [details] [review]
PopupWindow v5

PopupWindow v4
* More tests.
* Change orientation logic.
Attachment #8397028 - Flags: review?(etienne)
Comment on attachment 8397028 [details] [review]
PopupWindow v5

Transfer to vivien.
Attachment #8397028 - Flags: review?(etienne) → review?(21)
Duplicate of this bug: 902784
Comment on attachment 8397028 [details] [review]
PopupWindow v5

While I really would like to be able to review it, I already know that I will be hard for me to find the necessary time to review this patch next week. I will likely be focused on the l10n refactor, vertical homescreen, widgets, rild.js, webRTC, and Web Components things :/

As Etienne as already started to look at it, he will likely be much faster than myself trying to review it.

For what it worth your windows schema makes total sense to me.

Do you consider the keyboard to be part of the vertical window in the future too ?

Does the horizontal window term will represent the app boundary that will be used by the edge gesture code to determine which types of transitions to apply ?

I guess the visibility delegated to top most window means also that the screenshot will be the top most window only ?

It scares me a little bit to delegate the orientation to the top most window only. For example if a landscape game opens an activity to a portrait only inline activity, the game may react and resize incorrectly, resulting in visual glitches when you close the activity.
Attachment #8397028 - Flags: review?(21) → review?(etienne)
Comment on attachment 8397028 [details] [review]
PopupWindow v5

Comments on github (not much) and needs a post bubble-tea rebase.

I think this can/should land this week :)
Attachment #8397028 - Flags: review?(etienne)
Target Milestone: 1.4 S3 (14mar) → 1.4 S5 (11apr)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #16)
> Comment on attachment 8397028 [details] [review]
> PopupWindow v4
> 
> While I really would like to be able to review it, I already know that I
> will be hard for me to find the necessary time to review this patch next
> week. I will likely be focused on the l10n refactor, vertical homescreen,
> widgets, rild.js, webRTC, and Web Components things :/
> 
> As Etienne as already started to look at it, he will likely be much faster
> than myself trying to review it.
> 
> For what it worth your windows schema makes total sense to me.
> 
> Do you consider the keyboard to be part of the vertical window in the future
> too ?

Hmmm...interesting proposal, KeyboardWindow should be somewhat different because it doesn't cover the caller but co-exist. So I think keyboard is part of the baseWindow(bottom most window).

> 
> Does the horizontal window term will represent the app boundary that will be
> used by the edge gesture code to determine which types of transitions to
> apply ?

About in-app / app-to-app transition: We could determine by relationship of currentWindow & nextWindow/previousWindow.
> 
> I guess the visibility delegated to top most window means also that the
> screenshot will be the top most window only ?

Yes, only take screenshot of topMostWindow in taskManager. But for edge gesture all window should have its screenshot layer.

> 
> It scares me a little bit to delegate the orientation to the top most window
> only. For example if a landscape game opens an activity to a portrait only
> inline activity, the game may react and resize incorrectly, resulting in
> visual glitches when you close the activity.

Not all. Because resize is also delegated to top most window so the caller won't be resized.
But yes, we have bad orientation control right now because orientation is global but we will see 2 apps during transition and if they have different orientation we will have bad look.....I am still thinking how to overcome it.
Comment on attachment 8397028 [details] [review]
PopupWindow v5

Rebased v5.
Attachment #8397028 - Attachment description: PopupWindow v4 → PopupWindow v5
Attachment #8397028 - Flags: review?(etienne)
Attachment #8394040 - Attachment is obsolete: true
Comment on attachment 8397028 [details] [review]
PopupWindow v5

Bad news: made a few comments on github that are behavior related so I can't r+ before we fix them.

Good news: I'm getting comfortable around this patch so hopefully we won't have to go to double digit version number ;)
Attachment #8397028 - Flags: review?(etienne)
Target Milestone: 1.4 S5 (11apr) → 1.4 S6 (25apr)
Comment on attachment 8397028 [details] [review]
PopupWindow v5

v6, took a long time cause of other blockers.
* Rebased.
* Fix comments in github as most as possible.
* Fix broken integration test.
Attachment #8397028 - Flags: review?(etienne)
Comment on attachment 8397028 [details] [review]
PopupWindow v5

Like they said, the sixth time's the charm :)

Some comments on github but nothing that requires another review round.
Congrats!
Attachment #8397028 - Flags: review?(etienne) → review+
https://github.com/mozilla-b2g/gaia/commit/f5b9126fb1b14066baf49775ffa2f54eee106be7

Hopefully we won't have too much backout ;)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Blocks: 1000800
Blocks: 1000812
Blocks: 1008928

Updated

4 years ago
Blocks: 1018487

Updated

4 years ago
No longer blocks: 1018487

Updated

4 years ago
Blocks: 1009271

Updated

4 years ago
Depends on: 1021658

Updated

4 years ago
Depends on: 1027401
Blocks: 1032068
You need to log in before you can comment on or make changes to this bug.