1043293 - [Verticalhome] use "touchend" instead of "click" to launch applications

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

blocking a blocker.

Comment 1

[Kevin Grandon :kgrandon]

Should just be a fact of changing references to clickIcon everywhere it's mentioned[1]. The search and collection apps will also benefit from this fix which is nice. Cristian - I know you're going on PTO soon, any interest in taking a look before you go?

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/grid_view.js#L124

Comment 2

[Kevin Grandon :kgrandon]

Oh, actually I didn't see that this was assigned to Julien, nevermind then :) Julien - let us know if you need help, thanks!

Comment 3

[Cristian Rodriguez (:crdlc)]

Attachment: 1043293.patch

I have this code in my local just in case you like the approach

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: github PR

So, it was more challenging that I expected ;) I actually thought that merely changing the 'click' to 'touchend' would just work, but it turns out the Web platform does clever things with mouse events and scrolling using touch, that we don't get for free with touch events. So I had to replicate it.

Do you have any integration test covering launching an app? Otherwise I can try to do a unit test, but if it's already covered I don't want to waste time :)

Tell me what you think!

Comment 6

[Kevin Grandon :kgrandon]

Just making a note that this appears to regress bug 1015000. I don't really care about that functionality, but if we can't fix it we should loop in product/QA. The launch improvements are much better than the stop functionality.

We have several tests for launching apps, but we do not yet have tests for things like bug 1015000 unfortunately - we don't have the proper APZ implementation on desktop to test this.

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Kevin Grandon :kgrandon from comment #6)
> Just making a note that this appears to regress bug 1015000. I don't really
> care about that functionality, but if we can't fix it we should loop in
> product/QA. The launch improvements are much better than the stop
> functionality.
> 
> We have several tests for launching apps, but we do not yet have tests for
> things like bug 1015000 unfortunately - we don't have the proper APZ
> implementation on desktop to test this.

Ok, I tested it before and it appeared to work. I just tried it again and it doesn't, so sorry about this.

I think a solution is to put a proper scroll event back (the existing one didn't work, but since the 'click' event was not sent, it was working). I'll try this now.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8461507 [details] [review]
github PR

I added 2 commits:

* the first commit restores the previous behavior
* the second commit makes it different: if we scroll slowly, we can tap an app. When I try this it feels really better, but tell me what you think.

It's also probably better to move that second commit in a separate bug given this one is a blocker.

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8461507 [details] [review]
github PR

I don't really think that we can split them up, they probably need to go together, and I don't think we should remove the scroll handler part in the first patch.

I'm going to clear feedback for now and keep experimenting. I'm noticing that I'm getting a lot of unwanted app launches scrolling up and down, which I don't think I should see. Not sure what the cause is yet.

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Kevin Grandon :kgrandon from comment #9)
> Comment on attachment 8461507 [details] [review]
> github PR
> 
> I don't really think that we can split them up, they probably need to go
> together, and I don't think we should remove the scroll handler part in the
> first patch.

I meant, there are 3 commits all in all:
* commit 1 is my first commit, which has the issue you mentionned earlier
* commit 2 fixes this issue and it should have no other issue from my testing
* commit 3 introduces a different behavior: when the scroll is slow, then we can launch an app

I was saying that commit 3 makes sense but is probably out of this bug. Commit 1 + commit 2 are definitely going in together.

Comment 11

[Jason Smith [:jsmith]]

Blocking for helping resolve the blocking bug.

Comment 12

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8461507 [details] [review]
github PR

LGTM, r+ for the first and second patches. I am a bit concerned about the third one because why don't we cancel the touch behavior when we receive a scroll event although this would be slow instead of asking for scrollTop? This operation implies reflows while scrolling

Comment 13

[Cristian Rodriguez (:crdlc)]

As i said the code looks good although there are some comments on github that I would like you will address (if you are agree with them of course)

Comment 14

[Cristian Rodriguez (:crdlc)]

I am wondering if we need to support mouse events for the simulator or is it legacy and we are focused only in touch events? This question comes to main because AFAIR we had to support it in the old home. Perhaps it is not needed right now

var isTouch = 'ontouchstart' in window;
var touchstart = isTouch ? 'touchstart' : 'mousedown';
var touchend = isTouch ? 'touchend' : 'mouseup';

Comment 15

[Kevin Grandon :kgrandon]

We no longer need to support mouse events in the simulator - it has touch support.

I am still concerned about launching apps during scrolling, but perhaps that is only because I tested with the third commit.

Comment 16

[Julien Wajsberg [:julienw]]

I'll separate the commits shortly so that you can give it another try Kevin.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> I'll separate the commits shortly so that you can give it another try Kevin.

done and moved the 3rd commit to bug 1043834 :)

Now looking at Cristian's comments :)

Comment 18

[Julien Wajsberg [:julienw]]

master: 82654e607a579374052124499d40135868da9a72

Comment 19

[Julien Wajsberg [:julienw]]

reverted: ddee66d01a2907d3300421e3200901b436bbca50

I haven't checked before merging: the unit tests seem to fail.
Will look at it shortly.

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8461507 [details] [review]
github PR

Hey Kevin,

Now, I tried with search and collection and it seems to work. Unit tests pass locally, we'll see for Gaia Try.

I'm now unconditionally attach the listener to "window" with useCapture = true. This works in all cases I tried, but maybe I lost some. Also we'll get all "scroll" events but in our current cases this is probably good enough.

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8461507 [details] [review]
github PR

Didn't find any problems and looks good to me. Thanks!

Comment 22

[Julien Wajsberg [:julienw]]

Kevin, I see we have marionette test issues. I tried to replace "click()" calls by "tap()"  calls, this seems to fix some tests, but not all of them.

I admit I'm quite stuck now, especially that I don't have much experience writing marionette tests.

I've pushed the last version of the patch on the pull request, if you want to have a look at it.

Thanks !

Comment 23

[Kevin Grandon :kgrandon]

Attachment: Pull request - Test support, simplify launch icon utility

Hey Julien, yeah the tests can be a bit tricky. I think what's probably happening is the launch_icon.js utility is not catching the event.

I'm opening up this standalone PR to see what it does on master, and if it looks good we can land it, and hopefully your tests will pass.

Comment 24

[Julien Wajsberg [:julienw]]

I think this won't be enough because:
* some failing tests are in collection and search
* even in verticalhome, there is at least one test where we don't launch, but we cancel a download. (this is one of the tests I struggled with btw).

Comment 25

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> I think this won't be enough because:
> * some failing tests are in collection and search
> * even in verticalhome, there is at least one test where we don't launch,
> but we cancel a download. (this is one of the tests I struggled with btw).

That should be ok - I think you can still "launch" an icon to cancel the download. I will try to look more at these tests early next week.

Comment 26

[Julien Wajsberg [:julienw]]

(In reply to Kevin Grandon :kgrandon from comment #25)
> (In reply to Julien Wajsberg [:julienw] from comment #24)
> > I think this won't be enough because:
> > * some failing tests are in collection and search
> > * even in verticalhome, there is at least one test where we don't launch,
> > but we cancel a download. (this is one of the tests I struggled with btw).
> 
> That should be ok - I think you can still "launch" an icon to cancel the
> download. I will try to look more at these tests early next week.

Yes, but I mean, you won't have the "launching" class, right?

Comment 27

[Kevin Grandon :kgrandon]

Hey Julien,

I finally got some passing tests, but I wasn't having much luck with the touch events. I think keeping the tests in the same format as they are in now, and keeping the click listener is fine until we figure out the tests.

You can fold this commit into your code it helps: https://github.com/KevinGrandon/gaia/commit/bf44c5df14497685ab77a719c2a3271dbb55bde3

Comment 28

[Kevin Grandon :kgrandon]

FYI - I have that commit in this pull request: https://github.com/mozilla-b2g/gaia/pull/22193

Tests passing here: https://tbpl.mozilla.org/?rev=dba039ae84f4ad8503d7b4f99ed6a8252c88b8df&tree=Gaia-Try

Comment 29

[Julien Wajsberg [:julienw]]

I'm moving forward with fixing the tests one by one. WIP is [1], but not finished yet.

[1] https://github.com/julienw/gaia/compare/1043293-use-touchend-fix-tests

Comment 30

[Julien Wajsberg [:julienw]]

Attachment: new github PR

Did the additional work on a separate branch so I created a new PR for convenience.

Waiting for a Gaia Try run.

Comment 31

[Julien Wajsberg [:julienw]]

Attachment: new github PR

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8465401 [details] [review]
new github PR

Look ma, it's green !
https://tbpl.mozilla.org/?rev=5149be2f766bc47097c8e0dcba023ee2df31ca4f&tree=Gaia-Try

There are 2 additional commits:
* the 3rd commit contains new code for an issue I found while fixing the tests. Basically it prevents launching an app if a contextmenu event has been sent
* the 4th commit is only test fixing

What do you think?

Comment 33

[Kevin Grandon :kgrandon]

Comment on attachment 8465401 [details] [review]
new github PR

Makes sense to me, very nice work!

Comment 34

[Julien Wajsberg [:julienw]]

master: 87ffc8423953bf052ed2b953c7f5e0133cbf1384

Thanks ! Was much more complex than I expected ;)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.0 uplift.

Comment 36

[Julien Wajsberg [:julienw]]

Attachment: v2.0 PR

PR with the test-only patches from bug 1034657 and bug 1036053

Let's see what TBPL says.

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8466181 [details] [review]
v2.0 PR

Did some more fixing and pushed to the PR.

What's puzzling is that these changes should have been needed on master too... For example, the app_search_test.js test (in the "search" app) is using "click" to launch an app, but this should not work now! Yet it works on master (checked locally) and not on v2.0 (checked locally too).

Same goes for app_packaged_fail_test.js, the fail is identical on master, it should have failed there too. For this file, I can't make it fail locally on v2.0 though, yet it failed on TBPL. I've fixed the failure like I did in similar cases in other files, but it's still puzzling. I have a b2g-desktop from central too, so that might explain the difference.

Comment 38

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #37)

> Same goes for app_packaged_fail_test.js, the fail is identical on master, it
> should have failed there too. For this file, I can't make it fail locally on
> v2.0 though, yet it failed on TBPL. I've fixed the failure like I did in
> similar cases in other files, but it's still puzzling. I have a b2g-desktop
> from central too, so that might explain the difference.

Slight correction: I couldn't make app_unrecoverable_error_test.js fail locally, app_packaged_fail_test.js was failing.

Comment 39

[Julien Wajsberg [:julienw]]

I actually reverted from master in faf9fa28400f56227f4781e0786bc451a83d67e5
There are too many intermittents coming from this.

Comment 40

[Julien Wajsberg [:julienw]]

Attachment: master github PR

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 8466289 [details] [review]
master github PR

hey Kevin,

some more fixes, TBPL looks fine:
https://tbpl.mozilla.org/?rev=89dc90e7dfffe25198279c47754d07bc0b493339&tree=Gaia-Try

I can reproduce the failures on master too.

Comment 42

[Kevin Grandon :kgrandon]

Comment on attachment 8466289 [details] [review]
master github PR

Makes me sad that we need the workarounds in tests, but seems fine to me. Thank you so much for taking care of the tests!

Comment 43

[Julien Wajsberg [:julienw]]

master: 0e3c4c270d3b84e88aa8652da1807b6ff7f77b06


(In reply to Kevin Grandon :kgrandon from comment #42)
> 
> Makes me sad that we need the workarounds in tests, but seems fine to me.
> Thank you so much for taking care of the tests!

I hope the "tap" issue will be resolved soon :)

Comment 44

[Julien Wajsberg [:julienw]]

A branch patch should not be necessary now, as I uplifted separately the test patches that produced the issue.

Comment 45

[Kevin Grandon :kgrandon]

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

Comment 46

[Julien Wajsberg [:julienw]]

This is causing issues in Gip, but in Travis only because these tests are disabled in TBPL:
https://travis-ci.org/mozilla-b2g/gaia/jobs/31630929

Comment 47

[Zac C (:zac)]

Addressed comment #46 with timing/tap location changes:
https://github.com/mozilla-b2g/gaia/commit/84af4f7a84d449165a2618fd5cfe4431bf156bfe

There was a little race between the scrolling, the tap and the 'app installed' banner.

Comment 48

[Viorela Ioia [:viorela]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/22554

Address failures on master

Comment 49

[[:AndreiH]]

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/738831ec864946f856032f1203e3fcc2ad25b568