1138956 - [search] edge gestures keep working when clicking on the rockerbar

Status: VERIFIED FIXED

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

Description

[Alberto Pastor [:albertopq]]

STR:

1.- Open 2 apps
2.- Go to one of them
3.- Click on the rocketbar and quickly start switching between apps with an edge gesture

Not sure what's the expected behavior here. Probably that the edge gesture doesn't work at all after clicking on the search.

Comment 1

[Alberto Pastor [:albertopq]]

 Rob, could you please confirm what's the expected behavior here?

Comment 2

[Rob MacDonald [:robmac]]

Hi Alberto...

Sorry for the delay. I went back through the specs and this isn't referenced so there's a bit of a gap here. Generally, though, as I was working with Etienne on sheets we tried to minimize disabling the edge gesture to make it universally available and predictable. So in apps where the keyboard is displayed for an inline form edge gestures work and the keyboard is dismissed as the user activates the edge gesture.

I would apply the same logic to rocketbar and dismiss the keyboard and search in a similar fashion to the inline keyboard scenarios. However, from what I recall there may have been some constraints on this so I'll flag Etienne as he likely has a better recollection on this and any proposed solution.

- Rob

Comment 3

[Etienne Segonzac (:etienne)]

For the inline keyboard case what's happening is really clear, you can see the 2 app windows moving below your finger while the keyboard is hiding, and you clearly "grabbed" one of the window.

I'm afraid it might be a bit weird in the case of the rocketbar, because of the rocketbar overlay hiding the app windows below.

So I don't have a strong opinion. But if disabling the rocketbar when a sheet-gesture-begins result in jankiness/weirdness I'm fine with just disabling the edge gestures while the rocketbar is focused.

Comment 4

[Gregor Wagner [:gwagner]]

Rob, can you make a decision if we should fix this or close the bug?

Comment 5

[Rob MacDonald [:robmac]]

Thanks for flagging me, Gregor. I propose we disable the edge gesture for now to maintain the polish. We can then revisit the transition in an upcoming release when we have more time to test it. Let me know if you have any concerns. - Rob

Comment 6

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

alberto, can you look into doing what rob suggested in commment 5?

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1138956-edge-rocketbar > mozilla-b2g:master

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8591568 [details] [review]
[gaia] albertopq:1138956-edge-rocketbar > mozilla-b2g:master

Etienne - could you review this?

I think it also might be better to change the approach to listen to rocketbar events so we don't have to introduce coupling to the rocketbar object.

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8591568 [details] [review]
[gaia] albertopq:1138956-edge-rocketbar > mozilla-b2g:master

(In reply to Kevin Grandon :kgrandon from comment #8)
> Comment on attachment 8591568 [details] [review]
> [gaia] albertopq:1138956-edge-rocketbar > mozilla-b2g:master
> 
> Etienne - could you review this?
> 
> I think it also might be better to change the approach to listen to
> rocketbar events so we don't have to introduce coupling to the rocketbar
> object.

Definitely!

We should disable the edge zones when entering the rocketbar, like [1] and re-enable it when it closes (if we're not on the homescreen), like [2].

We don't want to forward at all here since we would be forwarding to an app window the user can't see.


[1] https://github.com/mozilla-b2g/gaia/blob/43f67fb074c326286bdb15b31749b0905d427558/apps/system/js/edge_swipe_detector.js#L127
[2] https://github.com/mozilla-b2g/gaia/blob/43f67fb074c326286bdb15b31749b0905d427558/apps/system/js/edge_swipe_detector.js#L154-156

Comment 10

[Alberto Pastor [:albertopq]]

The edge gestures are already disabled via css on [1]. The patch is trying to solve the race condition between clicking on the rocketbar and start swiping. @Etienne, I'm sure you are right about forwarding, but I cannot see how 'this.lifecycleEnabled = false;' would solve the problem shown in the video. Touch events will still move the cards, won't they?

@Kevin, regarding the events, we receive 'touchstart' and 'touchmove' events before receiving the 'rocketbar-activating' one, so I'm not sure if we can use them.

[1] https://github.com/mozilla-b2g/gaia/blob/43f67fb074c326286bdb15b31749b0905d427558/apps/system/style/statusbar/statusbar.css#L9

Comment 11

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #10)
> The edge gestures are already disabled via css on [1]. The patch is trying
> to solve the race condition between clicking on the rocketbar and start
> swiping. @Etienne, I'm sure you are right about forwarding, but I cannot see
> how 'this.lifecycleEnabled = false;' would solve the problem shown in the
> video. Touch events will still move the cards, won't they?

If it comes to this we can chose to ignore those touch events without forwarding them, but:

Isn't rocketbar-activating [1] being dispatched in the same tick where we set |rocketbar.active| (that you use in your initial patch)?

[1] https://github.com/mozilla-b2g/gaia/blob/00130552f1913dab822762ba85d47ba2825bf80e/apps/system/js/rocketbar.js#L132

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Alberto Pastor [:albertopq] from comment #10)
> @Kevin, regarding the events, we receive 'touchstart' and 'touchmove' events
> before receiving the 'rocketbar-activating' one, so I'm not sure if we can
> use them.

We won't start to move the sheets at the first touchmove, the delta has to be > to kSignificant.

Comment 13

[Alberto Pastor [:albertopq]]

Etienne, it seems you are the right person to take this bug. Could you take a look? Thanks!

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] etiennesegonzac:bug-1138956 > mozilla-b2g:master

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8592737 [details] [review]
[gaia] etiennesegonzac:bug-1138956 > mozilla-b2g:master

(In reply to Alberto Pastor [:albertopq] from comment #13)
> Etienne, it seems you are the right person to take this bug. 

And you the right person to review it :)

Comment 16

[Alberto Pastor [:albertopq]]

Comment on attachment 8592737 [details] [review]
[gaia] etiennesegonzac:bug-1138956 > mozilla-b2g:master

LGTM! SheetsTransition.snapInPlace(); was what I was missing :)

Thanks!

Comment 17

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/d7a4735522fc80b6ea1496c0979c59318e29462a

Comment 18

[(PTO June 19-July 7) Ryan VanderMeulen]

Please request Gaia v2.2 approval on this patch when you get a chance.

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8592737 [details] [review]
[gaia] etiennesegonzac:bug-1138956 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): rocketbar + edge gesture
[User impact] if declined: See Description, note: the user has to be really quick for the bug to happen it's not a super common scenario
[Testing completed]: STR from the bug and checking that we don't end up wrongly enabling edge gestures when we shouldn't (ie. on the homescreen)
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 20

[(PTO June 19-July 7) Ryan VanderMeulen]

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

Comment 21

[Hermes Cheng (inactive after July 27, 2015)]

NI myself for following verification.

Comment 22

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master and 2.2.

Result: Edge gesture is not active after the user taps on the rocketbar. It is no longer accessible during the rocketbar expansion animation.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150421010201
Gaia: a8e4f95dce9db727dda5d408b038f97fb4296557
Gecko: 7b823253d9f2
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150421002501
Gaia: 828dd03a0e3b140d74b2e49355197df4d91d9227
Gecko: 36f72a3efb9b
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0