Closed Bug 961800 Opened 6 years ago Closed 6 years ago

[haida] Enable apps to open new sheets

Categories

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

x86
macOS
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: etienne, Assigned: alive)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [systemsfe][p=8][in-bubble-tea])

Attachments

(1 file, 1 obsolete file)

In order to prototype the "haidification" of apps further [1], we'll need to be able to spawn new "sheets" from an app.

Currently, if an app calls window.open it goes through the PopupManager [2] and opens a modal window with chrome (it's used for facebook auth...).

One exception is the AttentionWindow [3] which uses a special feature keyword to open a window on top of everything else (like the dialer's call screen).

We probably want to implement something similar the the AttentionWindowFactory submodule to handle new sheets.

ie.
* Listen for |mozbrowseropenwindow| event on the app iframe directly
* Intercept (prevent default) requests for windows _on the same origin_ than the app (so opening a facebook URL for auth would still spawn a window with some chrome)
* Publish an open-sheet event

-> The AppWindowManager will listen for the open-sheet events, take over, instantiate the AppWindow (which will be properly inserted in the StackManager), transition the new sheet from the right...

PS: Not sure how to handle the case where an app window.open on the exact same URL it's currently displaying, pretty sure some stuff will break, we might want to prevent this.

[1] see bug 946750
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/popup_manager.js#L33
[3] see bug 927862
feedback? on the proposition in the Description.
Blocks: haida, 946750
Flags: needinfo?(alive)
Hey Gregor, not sure how SystemFE bugs get prioritized, but apparently this is a MWC blocker.
Assignee: nobody → alive
Flags: needinfo?(alive)
Etienne, IMO this should be done in PopupWindow rewrite. Attention is too strong to use and the animation is not the same.
We could do this in PopupWindowFactory:
1. Open a remote url: append browser chrome to PopupWindow/BrowserWindow and use popup animation
2. Open the same origin: do not append browser chrome to PopupWindow/BrowserWindow and use haida animation
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3)
> Etienne, IMO this should be done in PopupWindow rewrite. Attention is too
> strong to use and the animation is not the same.

Small misunderstanding, I meant doing it in a similar submodule, not as part of the AttentionWindows.

> We could do this in PopupWindowFactory:
> 1. Open a remote url: append browser chrome to PopupWindow/BrowserWindow and
> use popup animation
> 2. Open the same origin: do not append browser chrome to
> PopupWindow/BrowserWindow and use haida animation

But don't we want the new sheet the be an AppWindow? It should be part of the navigation stack, the cardview and should be able to open a new sheet too.
(In reply to Etienne Segonzac (:etienne) from comment #4)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3)
> > Etienne, IMO this should be done in PopupWindow rewrite. Attention is too
> > strong to use and the animation is not the same.
> 
> Small misunderstanding, I meant doing it in a similar submodule, not as part
> of the AttentionWindows.
> 
> > We could do this in PopupWindowFactory:
> > 1. Open a remote url: append browser chrome to PopupWindow/BrowserWindow and
> > use popup animation
> > 2. Open the same origin: do not append browser chrome to
> > PopupWindow/BrowserWindow and use haida animation
> 
> But don't we want the new sheet the be an AppWindow? It should be part of
> the navigation stack, the cardview and should be able to open a new sheet
> too.

Cardview should be another story :/
A submodule to help window.open features does make sense, and thank you for reading my code before I set review.
But I think I know what you mean now, we need a XXXXWindowFactory for app wants to open a new sheet.

So my question is what's the features keyword for app to open sheets in the long run?
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5)
> So my question is what's the features keyword for app to open sheets in the
> long run?

I _think_ all window.open on the app origin (minus the attention screens) could be new sheets by default.
But if we have a rationale to use another special feature instead I'm completely fine with that too.

Vivien, any thought?
Flags: needinfo?(21)
(In reply to Etienne Segonzac (:etienne) from comment #6)
> (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5)
> > So my question is what's the features keyword for app to open sheets in the
> > long run?
> 
> I _think_ all window.open on the app origin (minus the attention screens)
> could be new sheets by default.
> But if we have a rationale to use another special feature instead I'm
> completely fine with that too.
> 
> Vivien, any thought?

By comment 6 we would have
* submodule: PopupWindowFactory to open PopupWindow(has no browser chrome) from a non-app Window.
* submodule: AttentionWindowFactory to open AttentionWindow from an app window instance with features=attention
* submodule: SheetWindowFactory? to open AppWindow from an app window instance without features.

But I think this doesn't make sense if an installed app tries to open |http://google.com| but we make an AppWindow for it thus google.com as it looks like part of the app as a sheet?
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #7)
> (In reply to Etienne Segonzac (:etienne) from comment #6)
> > (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5)
> > > So my question is what's the features keyword for app to open sheets in the
> > > long run?
> > 
> > I _think_ all window.open on the app origin (minus the attention screens)
> > could be new sheets by default.
> > But if we have a rationale to use another special feature instead I'm
> > completely fine with that too.
> > 
> > Vivien, any thought?
> 
> By comment 6 we would have
> * submodule: PopupWindowFactory to open PopupWindow(has no browser chrome)
> from a non-app Window.
> * submodule: AttentionWindowFactory to open AttentionWindow from an app
> window instance with features=attention
> * submodule: SheetWindowFactory? to open AppWindow from an app window
> instance without features.
> 
> But I think this doesn't make sense if an installed app tries to open
> |http://google.com| but we make an AppWindow for it thus google.com as it
> looks like part of the app as a sheet?

I agree that an installed app that pops up a new AppWindow but we still want a sheet, pointing for example to |http://google.com| should not create an AppWindow but a Popup/BrowserWindow.
For example if there is an installed Twitter app and the user click on a link inside it, then it should popup as a new independent sheet.

Part of my idea was that if the window is opened with the |dialog| keyword then the window will popup covering the current window/sheet by coming from the bottom and sliding to the top.

So this one sounds like a DialogWindow to me (if we want to split PopupWindow into multiples submodules), that should be dependent on the state of the sheet. So if the sheet is killed/closed then it should be removed.

If the window is not opened with this keyword, then it will pop up as a new sheet, either a new AppWindow or a BrowserWindow, sliding from the left to the right.

Hope it makes sense.
Flags: needinfo?(21)
On it right now. My first bug during this ww :)

Proposal: Implement submodule of AppWindow, ChildWindowFactory to deal with all window.open request.
I will update AttentionWindowFactory later if I have some time to finish that at this week.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #7)
> > (In reply to Etienne Segonzac (:etienne) from comment #6)
> > > (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5)
> > > > So my question is what's the features keyword for app to open sheets in the
> > > > long run?
> > > 
> > > I _think_ all window.open on the app origin (minus the attention screens)
> > > could be new sheets by default.
> > > But if we have a rationale to use another special feature instead I'm
> > > completely fine with that too.
> > > 
> > > Vivien, any thought?
> > 
> > By comment 6 we would have
> > * submodule: PopupWindowFactory to open PopupWindow(has no browser chrome)
> > from a non-app Window.
> > * submodule: AttentionWindowFactory to open AttentionWindow from an app
> > window instance with features=attention
> > * submodule: SheetWindowFactory? to open AppWindow from an app window
> > instance without features.
> > 
> > But I think this doesn't make sense if an installed app tries to open
> > |http://google.com| but we make an AppWindow for it thus google.com as it
> > looks like part of the app as a sheet?
> 
> I agree that an installed app that pops up a new AppWindow but we still want
> a sheet, pointing for example to |http://google.com| should not create an
> AppWindow but a Popup/BrowserWindow.
> For example if there is an installed Twitter app and the user click on a
> link inside it, then it should popup as a new independent sheet.
> 
> Part of my idea was that if the window is opened with the |dialog| keyword
> then the window will popup covering the current window/sheet by coming from
> the bottom and sliding to the top.
> 
> So this one sounds like a DialogWindow to me (if we want to split
> PopupWindow into multiples submodules), that should be dependent on the
> state of the sheet. So if the sheet is killed/closed then it should be
> removed.
> 
> If the window is not opened with this keyword, then it will pop up as a new
> sheet, either a new AppWindow or a BrowserWindow, sliding from the left to
> the right.
> 
> Hope it makes sense.

Let's clarify again:

### Without any features ###
* appWindow(installed twitter) uses window.open: an new AppWindow as child of current AppWindow. No dependency.
  (But if it's opening the same origin page, they will be in same process and OOM killer would make us kill both.)
  (So what do you mean by 'dependent'?)
* browserWindow(web page) uses window.open: an new BrowserWindow as child of current BrowserWindow. No dependency.

### With attention features ###
* Open AttentionWindow if it has permission, otherwise opens Whatever window its parent is.

### With dialog features ###
* Open DialogWindow, no permission check here. Dependency.
WIP
https://github.com/alivedise/gaia/compare/bugzilla;961800;child-window-factory?expand=1

This patch is 'just' working.
Issue revealed:
1. 2 or more sheets of same app in cardview.
   Idea: Patch cardview to show leaf window only if appWindow has 'childWindow' property.
2. Sheet manager is not smart enough.
   Idea: Count relationship in
3. AppWindowManager display root window but not leaf window when the app is opened again.
4. We may have some trouble if popup/activity/attetnion could open new sheet.
I wonder we should make window.open always open OOP mozbrowser iframe otherwise we would change current behavior.
One more problem is we may at the same enable 'multiple same origin' app window instances. But AppWindowManager still store instances by origin now...that's bad.
1. AppWindowManager fix: discard origin reference as best as I could!
2. Change StackManager to adopt in app sheets
3. AppWindow API change
4. window.open changes
5. Implement ChildWindowFactory.
Attachment #8370863 - Flags: review?(etienne)
I think I'd fixed almost the broken tests. Still watching..
Comment on attachment 8370863 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16045

Use a new pull request :)
Attachment #8370863 - Attachment description: https://github.com/mozilla-b2g/gaia/pull/15992 → https://github.com/mozilla-b2g/gaia/pull/16045
Whiteboard: [systemsfe][p=8]
Target Milestone: --- → 1.4 S1 (14feb)
Comment on attachment 8370863 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16045

Very impressive patch! Very cool to have the test apps to play with it :)

I did a solid first pass, mainly about testing.

PS: we should create a new attachment with the correct PR
Attachment #8370863 - Flags: review?(etienne)
Attached file ChildWindowFactory v4
Second run!
Attachment #8370863 - Attachment is obsolete: true
Attachment #8374735 - Flags: review?(etienne)
Zac, could you guide me what's wrong with the GaiaApps modification?
Seeing 'JavascriptException: JavascriptException: TypeError: appWindow is null
stacktrace:
	execute_async_script @gaia_test.py, line 82
	inline javascript, line 295
	src: "      let origin = appWindow.origin;"'
But not sure how to debug it...
Flags: needinfo?(zcampbell)
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #20)
> Zac, could you guide me what's wrong with the GaiaApps modification?
> Seeing 'JavascriptException: JavascriptException: TypeError: appWindow is
> null
> stacktrace:
> 	execute_async_script @gaia_test.py, line 82
> 	inline javascript, line 295
> 	src: "      let origin = appWindow.origin;"'
> But not sure how to debug it...

I am not hugely familiar with this part of the code (Dave and Jonathan did most of it) but it looks like the line prior to this one in gaia_apps.js is not returning anything, it is:
 let appWindow = GaiaApps.getAppByName(appName);

The appName the test passes in as per the manifest file. Why don't you put an exception if the loop in getAppByName finishes without finding the app?

To actually run the tests locally:
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/gaia-ui-tests/Gaia_UI_Tests_Run_Tests#Running_tests_using_the_Travis_scripts_%28for_Gecko_and_Gaia_developers%29

You can also ping Askeing for help!
Flags: needinfo?(zcampbell)
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

Second round...done :)

Mainly the missing ChildWindowFactory coverage and the broken python test, the third one should be the charm :)
Attachment #8374735 - Flags: review?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #22)
> Comment on attachment 8374735 [details] [review]
> ChildWindowFactory v2
> 
> Second round...done :)
> 
> Mainly the missing ChildWindowFactory coverage and the broken python test,
> the third one should be the charm :)

OH I just noticed I forget to git add child_window_factory_test....zombie ate my brain...
Dave could you help to debug gaia_apps change?
Flags: needinfo?(dave.hunt)
I'm not sure what I can add, other than I tend to use console.log for debugging the atoms.
Flags: needinfo?(dave.hunt)
I am getting this message on local run:

test_cold_launch (test_cold_launch.TestColdLaunch) ... ERROR

======================================================================
ERROR: None
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette_test.py", line 143, in run
    testMethod()
  File "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cold_launch.py", line 11, in test_cold_launch
    app = self.apps.launch('Clock')
  File "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 91, in launch
    self.switch_to_frame(app.frame_id, url)
  File "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 166, in switch_to_frame
    if check(self.marionette.get_url()):
  File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette.py", line 901, in get_url
    response = self._send_message("getCurrentUrl", "value")
  File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette.py", line 605, in _send_message
    self._handle_error(response)
  File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette.py", line 667, in _handle_error
    raise MarionetteException(message=response, status=500)
MarionetteException: MarionetteException: {u'message': u'Marionette does not recognize the packet type "getCurrentUrl"', u'error': u'unrecognizedPacketType'}
TEST-UNEXPECTED-FAIL | test_cold_launch.py test_cold_launch.TestColdLaunch.test_cold_launch |
----------------------------------------------------------------------
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

Only one python test is failing now so I think it's proper to set review now.
Attachment #8374735 - Flags: review?(etienne)
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #26)
> I am getting this message on local run:
> 
> test_cold_launch (test_cold_launch.TestColdLaunch) ... ERROR
> 
> ======================================================================
> ERROR: None
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
> "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/
> marionette/marionette_test.py", line 143, in run
>     testMethod()
>   File
> "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/
> test_cold_launch.py", line 11, in test_cold_launch
>     app = self.apps.launch('Clock')
>   File
> "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.
> py", line 91, in launch
>     self.switch_to_frame(app.frame_id, url)
>   File
> "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.
> py", line 166, in switch_to_frame
>     if check(self.marionette.get_url()):
>   File
> "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/
> marionette/marionette.py", line 901, in get_url
>     response = self._send_message("getCurrentUrl", "value")
>   File
> "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/
> marionette/marionette.py", line 605, in _send_message
>     self._handle_error(response)
>   File
> "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/
> marionette/marionette.py", line 667, in _handle_error
>     raise MarionetteException(message=response, status=500)
> MarionetteException: MarionetteException: {u'message': u'Marionette does not
> recognize the packet type "getCurrentUrl"', u'error':
> u'unrecognizedPacketType'}
> TEST-UNEXPECTED-FAIL | test_cold_launch.py
> test_cold_launch.TestColdLaunch.test_cold_launch |
> ----------------------------------------------------------------------

This is likely related to bug 941136. You will need to make sure you have a matching Marionette client/server versions.
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

And... third round.

Let's do a last round once the comments are addressed and travis is green with a contacts app, email app and a homescreen app peer in the loop.
Attachment #8374735 - Flags: review?(etienne)
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

V4
Attachment #8374735 - Attachment description: ChildWindowFactory v2 → ChildWindowFactory v4
Attachment #8374735 - Flags: review?(francisco.jordano)
Attachment #8374735 - Flags: review?(etienne)
Attachment #8374735 - Flags: review?(crdlc)
Attachment #8374735 - Flags: review?(bugmail)
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

LGTM, just a question on github, thx
Attachment #8374735 - Flags: review?(crdlc) → review+
Are we documenting the various window.open features we have? (attention, dialog...) Should we?

(sorry if this isn't the correct use of dev-doc-needed)
Keywords: dev-doc-needed
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

r=me with comments addressed, we should probably wait for the 1.4 branch to be created before landing this.

Huge patch, huge feature, huge potential :)
Attachment #8374735 - Flags: review?(etienne) → review+
Comment on attachment 8374735 [details] [review]
ChildWindowFactory v4

Great job, I've been trying the contacts and ftu modifications, working well, also the tricky activity cicle between contacts, sms and contacts again working well.

Thanks Alive!
Attachment #8374735 - Flags: review?(francisco.jordano) → review+
Whiteboard: [systemsfe][p=8] → [systemsfe][p=8][in-bubble-tea]
Attachment #8374735 - Flags: review?(bugmail)
blocking-b2g: --- → backlog
Target Milestone: 1.4 S2 (28feb) → ---
in master https://github.com/gasolin/gaia/commit/439b23a04c6dba788293f577b1efe8c5d8843b78
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Reverted in https://github.com/mozilla-b2g/gaia/commit/e11b3cd5c35a564075fe21a610604418975f0c00 because something in this or one of the six other patches that the b2g bumper bot included in a single push to b2g-inbound broke gaia-ui tests on TBPL:

bumper bot's push's contents: https://hg.mozilla.org/integration/b2g-inbound/rev/0c2938cd910b
The gaia-ui failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=36421229&tree=B2g-Inbound


In the future, can we please not merge seven patches in the course of 5 minutes? It would make tracking down regressions much simpler if they were spaced several minutes apart each.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://travis-ci.org/mozilla-b2g/gaia/builds/21163364 green
reland
https://github.com/mozilla-b2g/gaia/commit/a1b8093857a64538b7e6758d129dd4958fde003f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
hi Alive, sorry had to revert this changeset again, seems we run into a gaia ui test bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=36439850&tree=B2g-Inbound which seems the same like in comment #39 again
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://github.com/mozilla-b2g/gaia/commit/af57923e3aa0d2d204fd135bcf39ef7f91c9a238
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
See https://bugzilla.mozilla.org/show_bug.cgi?id=987014#c1. Patch landings are not allowed to blindly shut off tests to get a feature to land. This is a critical automated test that cannot be shut off in the build, so this needs to be backed out.
Keywords: checkin-needed
This will require a release of gaiatest and a dependency bump for all packages that depend on it, such as b2gperf, in order to restore the ability to test against master.
I'd fixed the problem by some tricks. Will backout and reland soon.
50b956066751019d4adcb5e8c36eeda2d82de826 reverted
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://github.com/mozilla-b2g/gaia/commit/5c0b52751c7cf9f73d44126916a822baf65e2547
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 919498
Depends on: 997290
Depends on: 987994
Depends on: 1024472
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.