Closed Bug 1043293 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S2 (15aug)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Keywords: perf, regression, Whiteboard: [systemsfe][c=progress p= s= u=2.0][sms-sprint-2.1S1])

Attachments

(2 files, 6 obsolete files)

blocking a blocker.
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
Flags: needinfo?(crdlc)
Oh, actually I didn't see that this was assigned to Julien, nevermind then :) Julien - let us know if you need help, thanks!
Flags: needinfo?(crdlc)
Attached patch 1043293.patch (obsolete) — Splinter Review
I have this code in my local just in case you like the approach
Flags: needinfo?(felash)
Duplicate of this bug: 1043321
Attached file github PR (obsolete) —
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!
Attachment #8461507 - Flags: review?(crdlc)
Flags: needinfo?(felash)
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.
(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 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.
Attachment #8461507 - Flags: feedback?(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.
Attachment #8461507 - Flags: feedback?(kgrandon)
(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.
Blocking for helping resolve the blocking bug.
blocking-b2g: 2.0? → 2.0+
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
Attachment #8461507 - Flags: review?(crdlc) → review+
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)
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';
Whiteboard: [systemsfe]
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.
I'll separate the commits shortly so that you can give it another try Kevin.
Blocks: 1043834
(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 :)
master: 82654e607a579374052124499d40135868da9a72
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8461434 - Attachment is obsolete: true
reverted: ddee66d01a2907d3300421e3200901b436bbca50

I haven't checked before merging: the unit tests seem to fail.
Will look at it shortly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8461507 - Flags: review?(kgrandon)
Comment on attachment 8461507 [details] [review]
github PR

Didn't find any problems and looks good to me. Thanks!
Attachment #8461507 - Flags: review?(kgrandon) → review+
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 !
Flags: needinfo?(kgrandon)
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.
Flags: needinfo?(kgrandon)
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).
(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.
Flags: needinfo?(kgrandon)
(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?
Attachment #8462715 - Attachment is obsolete: true
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
Flags: needinfo?(kgrandon)
Target Milestone: --- → 2.0 S1 (9may)
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
Target Milestone: 2.0 S1 (9may) → 2.1 S1 (1aug)
Whiteboard: [systemsfe] → [systemsfe][c=progress p= s= u=2.0]
Blocks: 1046254
No longer blocks: 1046254
Attached file new github PR (obsolete) —
Did the additional work on a separate branch so I created a new PR for convenience.

Waiting for a Gaia Try run.
Attachment #8461507 - Attachment is obsolete: true
Attached file new github PR (obsolete) —
Attachment #8465400 - Attachment is obsolete: true
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?
Attachment #8465401 - Flags: review?(kgrandon)
Comment on attachment 8465401 [details] [review]
new github PR

Makes sense to me, very nice work!
Attachment #8465401 - Flags: review?(kgrandon) → review+
master: 87ffc8423953bf052ed2b953c7f5e0133cbf1384

Thanks ! Was much more complex than I expected ;)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe][c=progress p= s= u=2.0] → [systemsfe][c=progress p= s= u=2.0][sms-sprint-2.1S1]
Needs rebasing for v2.0 uplift.
Flags: needinfo?(felash)
Attached file v2.0 PR (obsolete) —
PR with the test-only patches from bug 1034657 and bug 1036053

Let's see what TBPL says.
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.
(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.
Flags: needinfo?(felash)
Flags: needinfo?(felash)
I actually reverted from master in faf9fa28400f56227f4781e0786bc451a83d67e5
There are too many intermittents coming from this.
Flags: needinfo?(felash)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file master github PR
Attachment #8465401 - Attachment is obsolete: true
Attachment #8466181 - Attachment is obsolete: true
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.
Attachment #8466289 - Flags: review?(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!
Attachment #8466289 - Flags: review?(kgrandon) → review+
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 :)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
A branch patch should not be necessary now, as I uplifted separately the test patches that produced the issue.
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
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.
Address failures on master
Attachment #8468292 - Flags: review?(zcampbell)
Attachment #8468292 - Flags: review?(andrei.hutusoru)
Attachment #8468292 - Flags: review?(zcampbell)
Attachment #8468292 - Flags: review?(andrei.hutusoru)
Attachment #8468292 - Flags: review+
Depends on: 1049014
Depends on: 1050470
See Also: → 1083818
You need to log in before you can comment on or make changes to this bug.