1028708 - statusbar not opaque when opening the contextMenu in verticalhome

Status: VERIFIED FIXED

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

Description

[inscription]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428194215

Steps to reproduce:

Unlock the screen of the phone, get on the vertical homescreen, long touch an empty space on the screen to open the context menu. 


Actual results:

The context menu opens, but the status bar remains transparent.


Expected results:

the status bar should become opaque.

Comment 1

[inscription]

Attachment: statusbar not opaque when opening the contextMenu in verticalhome.

Comment 2

[inscription]

Attachment: statusbar not opaque when opening the contextMenu in verticalhome.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8444122 [details] [diff] [review]
statusbar not opaque when opening the contextMenu in verticalhome.

Review of attachment 8444122 [details] [diff] [review]:
-----------------------------------------------------------------

Cristian - Could you review this one for me? Thanks!

Comment 4

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8444122 [details] [diff] [review]
statusbar not opaque when opening the contextMenu in verticalhome.

Review of attachment 8444122 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM although where is the context-menu-open dispatched? I haven't seen it, please let me know if you missed it or I couldn't see it in the patch. Thanks

Comment 5

[Kevin Grandon :kgrandon]

Ah right, I guess we added this for some smart collections work: https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/contextmenu_ui.js#L30

(Needed to know the position to insert smart collections)

Comment 6

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8444122 [details] [diff] [review]
statusbar not opaque when opening the contextMenu in verticalhome.

Review of attachment 8444122 [details] [diff] [review]:
-----------------------------------------------------------------

ok, so r+

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1028708.patch (to check-in)

The patch to land (with Kevin Canévet) as an author).
Fixed a small nit as there was a bug when the homescreen was scrolled to the top, the edit mode was on, and a contextmenu was popped up.

The bug was that in edit mode you can have a transparent statusbar.

Comment 8

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8444373 [details] [diff] [review]
bug1028708.patch (to check-in)

Review of attachment 8444373 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM although I guess that we should do it how the old homescreen worked. I mean we should disable the contexmenu feature while we are in edit mode here https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/contextmenu_handler.js#L21

if (this.canceled || app.grid._grid.dragdrop.inEditMode) {
  return;
}

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Cristian Rodriguez (:crdlc) from comment #8)
> Comment on attachment 8444373 [details] [diff] [review]
> bug1028708.patch (to check-in)
> 
> Review of attachment 8444373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM although I guess that we should do it how the old homescreen worked. I
> mean we should disable the contexmenu feature while we are in edit mode here
> https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/
> contextmenu_handler.js#L21
> 
> if (this.canceled || app.grid._grid.dragdrop.inEditMode) {
>   return;
> }

I tend to think like you but I don't really know if this behavior is expected or not.

Comment 10

[Cristian Rodriguez (:crdlc)]

or disable contexmenu handler when it receives the gaiagrid-editmode-start and restore its behavior when it receives gaiagrid-editmode-end instead of checking app.grid._grid.dragdrop.inEditMode

I think that it should work so because the context menu does not make sense in edit mode IMHO

I saw that jsavory added the interaction design document, do you know if this is the expected behavior?

Comment 11

[Jason Smith [:jsmith]]

In a separate bug UX clarified that all instances of the transparent status bar should block the visual refresh for 2.0, so marking this as a blocker.

Comment 12

[Jacqueline Savory [:jsavory] UX]

Sorry Cristian, I'm not sure I completely understand the question. Could you explain the behaviour you are referring to?

Comment 13

[Cristian Rodriguez (:crdlc)]

OK, you are in the vertical homescreen and you enter in edit mode: Does contexmenu feature make sense in this scenario? In the old homescreen when you were in edit mode the contextmenu to set wallpaper or add a new smart collection was disabled. IMHO we should follow the same approach thought. Contextmenu feature only for normal mode instead of edit mode too

Comment 14

[Rob MacDonald [:robmac]]

(In reply to Cristian Rodriguez (:crdlc) from comment #13)
> OK, you are in the vertical homescreen and you enter in edit mode: Does
> contexmenu feature make sense in this scenario? In the old homescreen when
> you were in edit mode the contextmenu to set wallpaper or add a new smart
> collection was disabled. IMHO we should follow the same approach thought.
> Contextmenu feature only for normal mode instead of edit mode too

I agree - Let's stick the the previous behaviour and disable the context menu while in edit mode.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I will file a followup to not display the contextmenu in edit mode and provide a patch there. I Would like to land this patch as if, as it comes from a contributor, that may not have the time to fix the followup.

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=8249787956debdd02514b5bf3c480125486b2a71

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/6d33e7207146e86586ed49434662b9c1899c0a79

Gaia-Try is green for all the tests related to this. Seems like we have a new perma-red unrelated to this patch though as I can see it on all Gaia-Try results for everybody...

I will open the followup soon.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/39eacde29df03789ecfb10c58f15d821abd651c2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The original landing on master was pushed with Vivien as the author. At his request, I reverted the original landing and re-pushed it with Kevin as the author.

Master: https://github.com/mozilla-b2g/gaia/commit/1f0902c6165297d51883ca6f9fe300bfee1f4c08

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For what it worth, the followup I promised is bug 1031331.

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Gaia      b8f36518696f3191a56e4f33447ee9d6ec820da1
Gecko     https://hg.mozilla.org/mozilla-central/rev/9290d7995f98
BuildID   20140627040205
Version   33.0a1
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
Flame

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified on 

Gaia      8df02268fcd7e80c5fab8c1ec099772e37f8659d
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/731a5e8831e6
BuildID   20140627000201
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
Flame