835710 - Homescreen not preventing BrowserElementScrolling code from running

Status: RESOLVED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

+++ This bug was initially created as a clone of Bug #834996 +++

Behavior seen in profile of Homescreen app

 - user puts finger down
 - touchstart handled by grid.js
   - homescreen *does not* and call evt.stopPropagation()
 - BrowserElementScrolling processes onTouchStart() and does 20-25ms unnecessary work

We shouldn't run BES code when we're panning the homescreen.  I'm not sure yet if this 20-25ms is on the critical startup path yet, but at any rate it's wasted CPU time.

It looks like bug 831656 removed the stopPropagation() call that bug 834996 was filed about not working correctly.  Intentional?

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Added stopPropagation

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 2

[Cristian Rodriguez (:crdlc)]

sorry for the mistake!

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 707523 [details]
Added stopPropagation

No more BrowserElementScrolling in the profile.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 707523 [details]
Added stopPropagation

Approved for v1.0.0

Comment 5

[David Flanagan [:djf]]

It sounds like this fix works. I wonder, though... would preventDefault() also work? It seems like that would be the more appropriate thing to prevent this kind of default action

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No, that would incidentally block the contextmenu event.

Comment 7

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/commit/f9328375d52fee9b87d345cd2bc56fe89fecdefe

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jonas Sicking (:sicking) from comment #4)
> Comment on attachment 707523 [details]
> Added stopPropagation
> 
> Approved for v1.0.0

Heh, was just about to request this.

For posterity: this is a zero-risk patch that gets samples off the critical app startup path, and seems to alleviate a bit of lag on touchstart when panning.

Comment 9

[Cristian Rodriguez (:crdlc)]

I guess that this patch breaks the 'active' pseudo class in the list of items

Comment 10

[Cristian Rodriguez (:crdlc)]

I HAVE to eliminate this commit from master because there two regressions:

1) No active pseudo class on icons
2) No tap on everything.me categories

Comment 11

[Cristian Rodriguez (:crdlc)]

How can we back out the commit form master?

Comment 12

[Cristian Rodriguez (:crdlc)]

Attachment: Revert change due to regressions

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 13

[David Flanagan [:djf]]

Does calling stopPropagation() on the touchmove event solve the bug without introducing new ones?

If not, does preventDefault() work to solve the bug? Chris says that will break the contextmenu event. But you could simulate that easily by starting a timer on touchstart if you had to.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 708530 [details]
Revert change due to regressions

Just back out the earlier commit.

It's kind of bogus to rely on :active with touch events.  It works without the patch here because gecko is trying hard to preserve compat, but the stopPropagation() will stop that compat code from running.  The best thing to do here is for the homescreen to set a CSS class manually.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to David Flanagan [:djf] from comment #13)
> Does calling stopPropagation() on the touchmove event solve the bug without
> introducing new ones?
> 

It doesn't, sadly: the vast majority of the BES overhead here is on touchstart.

> If not, does preventDefault() work to solve the bug? Chris says that will
> break the contextmenu event. But you could simulate that easily by starting
> a timer on touchstart if you had to.

That's an option but (i) less trivial; (ii) homescreen doesn't know the platform contextmenu timeout so has to invent its own.  That's not necessarily a bad thing; UX wanted that, I think.

Comment 16

[Cristian Rodriguez (:crdlc)]

ok with active, but ev.me is listen to mouse events and with this commit ev.me is broken

Comment 17

[Cristian Rodriguez (:crdlc)]

ev.me categories are broken with this commit, any idea Ran? Are you using click event? thanks

just in case I have the revert here https://github.com/mozilla-b2g/gaia/pull/7898

Comment 18

[Cristian Rodriguez (:crdlc)]

Hi Chiris,

      Here [1] we have a simple solution to stop the propagation of the touchstart event in all pages except ev.me due to event collisions very hard to solve according to ranbena.

      The other problem about active pseudo class is more complicated to solve. 

1) We could only active this class when the tap is performed [2] (different behavior) or...

2) If we want to simulate the same behavior til now, we should implement it on touchstart and touchend handlers checking if the touchstart target is an icon . So we set or remove a CSS class manually.

       But my concern here: is it (check if the target is an icon ) faster than do not stop propagation?

[1] https://github.com/crdlc/gaia/commit/3313801cfee954f4f14e5cf65c0518b004d9a16c
[2] https://github.com/crdlc/gaia/blob/bug-835710-v2/apps/homescreen/js/page.js#L641

Comment 19

[Cristian Rodriguez (:crdlc)]

upps sorry Chiris = Chris

Comment 20

[Cristian Rodriguez (:crdlc)]

Active simulation manually in this commit 

https://github.com/crdlc/gaia/commit/60f55578bf69957e8ab1d0a7ab2e45f6d52c5c6a

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The big win here comes from stopPropagation() on touchstart events where the target is an icon (to prevent relatively expensive BrowserElementScrolling code from running).  It's cheap to check whether the event target is an icon.

Comment 22

[Cristian Rodriguez (:crdlc)]

OK I have a patch. IMHO we can ask for approval v1 because it is interesting to have on v1, are you ok? thanks Chris

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes please, the homescreen lagging on pans and creating overhead on taps is a big complaint right now.

Comment 24

[Cristian Rodriguez (:crdlc)]

OK, thanks, today I have a meeting during all day. I will try to create the patch today......

Comment 25

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

This patch implements:

* Stop propagations on all pages except ev.me (collisions)
* Active class is defined manually

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: more lag on panning
Testing completed: crdlc tested as developer, maybe the reviewer could test too
Risk to taking this patch (and alternatives if risky): low

Comment 26

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

Comment on attachment 711213 [details]
Patch v1

But please do not declare too many variables at the top of the function. That ends up in ugly files :)

Removing approval? request and let's see if this get a tef+ since this is a perf bug on the homescreen panning.

Triagers the risk for this bug is to have a regression in the active state of icons (the blue glow stuff over icons). It is buggy right now and this patch should help with that too so that's not a big risk imho.

Comment 27

[Cristian Rodriguez (:crdlc)]

Vivien, all comments have been addressed. Thanks

Comment 28

[Alex Keybl [:akeybl]]

Not a blocker, but definitely tracking. We'll approve for uplift to v1.0.1.

Comment 29

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/commit/562921c3e76521f6073316634450571e2e35b11f

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 18a5933140d843872683d1c5c983c6e0c9b17679

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1