1163524 - [Window Management] Action menu hidden when installing an app from the Rocketbar

Status: VERIFIED FIXED

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

Description

[Alberto Pastor [:albertopq]]

STR:

1.- Go to the rocketbar and search an app, 'Telegram' for example
2.- Click on the 'Install Telegram' icon

Expected:

It shows the action menu in order to choose the marketplace

Actual:

Nothing happens, but if you close the search, the action menu gets shown.

Comment 1

[Alberto Pastor [:albertopq]]

[Blocking Requested - why for this release]: broken feature.

Comment 2

[Alberto Pastor [:albertopq]]

Sounds like a 1121316 regression. Taking.

Comment 3

[Alberto Pastor [:albertopq]]

Alive, you know the z-indexes more than I do.

The action menu was -> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/zindex.css#L184
But now, after being a system dialog -> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/zindex.css#L251

Should we change the dialog-overlay z-index to a higher value, and reduce the dialogs inside depending on the case? Is there any case in which something that is inside the dialog-overlay shouldn't be in top of the Rocketbar?

Comment 4

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

Oops.

Good to know this case I never thought of.
So this might be a good reason to make ActionMenu become parts of the AppWindow instead of the SystemDialog. But that might be complex because sometimes we want to put the ActionMenu inside AppWindow but sometimes maybe not.

If we were to make Rocketbar lower than ActionMenu, I will worry about that eventually we will make everything under ActionMenu, which is SystemDialog and SIM PIN dialog belongs to that. then SIM PIN dialog will show on top of lockscreen window sometimes.

In the very first design, system dialog is something between lockscreen and app window to display some system related messages. I didn't think Rocketbar will be able to invoke a SystemDialog as well.

To change the z-index of system dialog to upon rocketbar, we will need to make sure other system dialog will not be affected by rocketbar. For example, could we still click the search bar when a system dialog like FxA or SIM PIN is opened to show the rocketbar? If so, we need to hide the system dialog when rocketbar is about to open otherwise, rocketbar will show under the system dialog. Something like this

The change action menu to become part of the app window, we need to
1. Render the action menu to Service.query('getTopMostWindow')'s appWindow container.
* It's tricky here. How to make every new ActionMenu call automatically do that?
  Maybe in ActionMenu do Service.request('injectUI', this); and HierarchyManager will inject the DOM element into the top most window's container and tell the window "you have a actionMenu inside now".
2. Close/Kill the action menu when an appWindow is killed if it has an actionMenu inside. This depends on what we want - switch the app window which has an action menu to pick the activity out and switch it back we should keep the menu. But if the app is killed at background we will need to kill the actionMenu because it will never work.

I think both way works for me, so it's up to you to think and choose what to do. The first way is fast but not flexible and the second way is complex but most flexible (we don't need to care about z-index anymore)

BTW, are we only using ActionMenu to represent AcitivtyChoice only and so in the future?

Alos, I want to hear from Etienne and Michael what you think here.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #4)
> The change action menu to become part of the app window, we need to
> 1. Render the action menu to Service.query('getTopMostWindow')'s appWindow
> container.
> * It's tricky here. How to make every new ActionMenu call automatically do
> that?
>   Maybe in ActionMenu do Service.request('injectUI', this); and
> HierarchyManager will inject the DOM element into the top most window's
> container and tell the window "you have a actionMenu inside now".
> 2. Close/Kill the action menu when an appWindow is killed if it has an
> actionMenu inside. This depends on what we want - switch the app window
> which has an action menu to pick the activity out and switch it back we
> should keep the menu. But if the app is killed at background we will need to
> kill the actionMenu because it will never work.

I don't have as clear a picture as you do of the problems we are facing here, but my initial thought is that this is adding a lot of complexity to deal with a z-index issue on a somewhat edge case. It may seem less elegant, but I don't mind the idea of changing (promoting) the z-index of the system dialog in certain scenarios.

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Michael Henretty [:mhenretty] from comment #5)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #4)
> > The change action menu to become part of the app window, we need to
> > 1. Render the action menu to Service.query('getTopMostWindow')'s appWindow
> > container.
> > * It's tricky here. How to make every new ActionMenu call automatically do
> > that?
> >   Maybe in ActionMenu do Service.request('injectUI', this); and
> > HierarchyManager will inject the DOM element into the top most window's
> > container and tell the window "you have a actionMenu inside now".
> > 2. Close/Kill the action menu when an appWindow is killed if it has an
> > actionMenu inside. This depends on what we want - switch the app window
> > which has an action menu to pick the activity out and switch it back we
> > should keep the menu. But if the app is killed at background we will need to
> > kill the actionMenu because it will never work.
> 
> I don't have as clear a picture as you do of the problems we are facing
> here, but my initial thought is that this is adding a lot of complexity to
> deal with a z-index issue on a somewhat edge case. It may seem less elegant,
> but I don't mind the idea of changing (promoting) the z-index of the system
> dialog in certain scenarios.

I share the complexity concern but I'll suggest something other than a z-index change: correct me if I'm wrong but I think that, from a user perspective, selecting a result from the rocketbar (be it local app or search suggestion) will always close the rocketbar.
Usually because we get an `appopened` as a result but some providers call `Search.close()` themselves apparently (like the contacts provider).

I think the search app should request to close the rocketbar itself whenever one of those marketplace "Install App" icon is clicked.

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1163524-action-menu-rocketbar > mozilla-b2g:master

Comment 9

[Alberto Pastor [:albertopq]]

Comment on attachment 8610045 [details] [review]
[gaia] albertopq:1163524-action-menu-rocketbar > mozilla-b2g:master

I would like UX to review Etienne's idea. Rob, could you take a look to the interaction of installing an app from the rocketbar search? Thanks!

Comment 10

[Alberto Pastor [:albertopq]]

Adding keyword to see if somebody from UX can take a look.

Comment 11

[Rob MacDonald [:robmac]]

FYI - Francis is in meetings today but should be able to look at this tomorrow. Thanks!

Comment 12

[Alberto Pastor [:albertopq]]

Comment on attachment 8610045 [details] [review]
[gaia] albertopq:1163524-action-menu-rocketbar > mozilla-b2g:master

Etienne, could you please take a look? Thanks!

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8610045 [details] [review]
[gaia] albertopq:1163524-action-menu-rocketbar > mozilla-b2g:master

We should probably factor the code a bit in the test file at some point but looks good to me!

Just to double check: clicking the 'install' button in the search result will trigger an activity?

Comment 14

[Alberto Pastor [:albertopq]]

(In reply to Etienne Segonzac (:etienne) from comment #13)
> Comment on attachment 8610045 [details] [review]
> [gaia] albertopq:1163524-action-menu-rocketbar > mozilla-b2g:master
> 
> We should probably factor the code a bit in the test file at some point but
> looks good to me!
> 
> Just to double check: clicking the 'install' button in the search result
> will trigger an activity?

Thanks Etienne. That's correct -> https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/marketplace_app.js#L40

Comment 15

[Alberto Pastor [:albertopq]]

Please, reopen if this behavior wasn't the expected. Thanks.

master: https://github.com/mozilla-b2g/gaia/commit/5df334619e9d11ca4c55bf930e198b5829d026f7

Comment 16

[Verson Xiong (Leave from Mozilla)]

Attachment: verify_master.3gp

This bug has been verified as "pass" on latest build of Flame master,Nexus5 master by the STR in comment 0.

Actually result: Device close rocketbar then shows the action menu.
Reproduce rate: 0/5
See attachment: verify_master.3gp

Device: Flame master build(pass)
Build ID               20150614010203
Gaia Revision          1bf2da102560481748ff3f6202fbed5c4daa5832
Gaia Date              2015-06-13 00:22:05
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c223b8844264
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150614.044513
Firmware Date          Sun Jun 14 04:45:25 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 master build(pass)
Build ID               20150614010203
Gaia Revision          1bf2da102560481748ff3f6202fbed5c4daa5832
Gaia Date              2015-06-13 00:22:05
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c223b8844264
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150614.044149
Firmware Date          Sun Jun 14 04:42:08 EDT 2015
Bootloader             HHZ12f

Comment 18

[Alberto Pastor [:albertopq]]

Attachment: v2.2 uplift Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30799

Comment 19

[Alberto Pastor [:albertopq]]

Comment on attachment 8628754 [details] [review]
v2.2 uplift Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30799

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: If the user has multiple marketplaces, the action menu will be shown in top of the rocketbar instead of closing it first. Not a big deal, but asking for approval as this fixes bug 1177354 as well.
[Testing completed]: Added unit tests. In master for a while without regressions.
[Risk to taking this patch] (and alternatives if risky): Low risk patch. Just adding a listener for an event.
[String changes made]: none

Comment 20

[Josh Cheng [:josh]]

Comment on attachment 8628754 [details] [review]
v2.2 uplift Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/30799

Approved as this break function in 2.2.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/2d6cbec3a4004bb770ecbadbba83f86b95365e56

Comment 22

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on latest build of Flame v2.2 and Nexus5 v2.2 by the STR in comment 0.

Actual results: Tapping an app icon from the Rocketbar, device will close the rocketbar and then show the action menu.
See attachment: verified_v2.2.3gp
Reproduce rate: 0/10


Device: Flame v2.2 (Verified) 
Build ID               20150705162505
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0214b4c1ea0
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150705.200108
Firmware Date          Sun Jul  5 20:01:20 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 v2.2 (Verified) 
Build ID               20150705002505
Gaia Revision          ea11f422b687a982f0a961c9aea7858066561707
Gaia Date              2015-07-02 23:37:50
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0214b4c1ea0
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150705.041046
Firmware Date          Sun Jul  5 04:11:05 EDT 2015
Bootloader             HHZ12f

Comment 23

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.3gp

Comment 24

[Alberto Pastor [:albertopq]]

Comment on attachment 8610045 [details] [review]
[gaia] albertopq:1163524-action-menu-rocketbar > mozilla-b2g:master

Clearing flags