Closed Bug 1083818 Opened 10 years ago Closed 10 years ago

Often tapping on icons on the homescreen just after a scroll fails to activate icon

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Homescreen
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

Github PR
46 bytes, text/x-github-pull-request
kgrandon
: review+
kgrandon
: feedback+
Details | Review
Don't wait 10ms if active state has no effect
6.33 KB, patch
botond
: review+
Details | Diff | Splinter Review 
The Gaia homescreen code has a 300ms delay after a scroll comes to a complete halt during which taps are ignored. I find this quite frustrating because often I tap on an icon after scrolling and it doesn't activate.

Some of this code was added as part of bug 1015000, and can be removed because Gecko now implements the same thing in the APZ code (bug 1022956). There is also other click-simulation code in Gaia which to me seems unnecessary, and I'm not sure why it's there. I think we should be able to rip that out entirely and just use the default Gecko/APZ click detection.

This has the additional advantage of making the homescreen behavior consistent with other apps, such as the settings app.
Attached file Github PR — 
As far as I can tell this behaves just fine on the vertical homescreen. I'm not sure if this component is used in other places and/or if there are tests that need adjusting too. Thoughts?
Attachment #8506158 - Flags: review?(kgrandon)
Hey Kats - I would absolutely *love* to rip this logic out if we can, but the tricky reason as to why we can't IMO is due to the reason this was implemented, which is bug 1043293 and bug 1038176. For reasons as to why this was added, please see bug 1038176 comment 39.

Application launch time is one of the most critical pieces of user-perceived performance, so we should make sure we don't regress this at all.

Kats - do you think that there would be some slowdown if we moved to click events instead of touchstart? If so, I'm not sure if we can accept that.
Flags: needinfo?(bugmail.mozilla)
See Also: → 1043293
Comment on attachment 8506158 [details] [review]
Github PR

Clearing review until we have an answer on comment 3. If this doesn't regress performance I'll definitely take another look. I'd want to measure the perf impact first though.
Attachment #8506158 - Flags: review?(kgrandon) → feedback+
Thanks for pointing me to those bugs, looks like there was a big effort to bring those app startup times down and I certainly don't want to regress that. Is there a way to measure the perf impact? Like a try push to datazilla or something?

From bug 1038176 comment 39 it sounds like the improvement this gave was about 20ms. As far as I can tell it should be even less than that. The only delay between touchend and click should be the time it takes to dispatch the other events in between (mousedown, mouseup, maybe a mousemove) and unless there are long-running listeners for those it shouldn't take 20ms.
Flags: needinfo?(bugmail.mozilla) → needinfo?(kgrandon)
It's currently a bit painful to test this. Datazilla doesn't test this because it launches apps directly with marionette I believe (instead of tapping on the icons).

We can test this by inserting some logging in both the homescreen and system apps. I'll leave the ni? on myself for now to create a patch to debug this and get timing.
On second thought, I am still thinking of the best way to actually test this and don't have a really good idea yet. Adding listeners might work, but is a bit wrong if we have to inject a touchstart listener to capture a timestamp in the click patch. I'd like to ni? a few people for information.

Julien - Do you remember how you got the timing information in bug 1038176 comment 39? At which point did you add logs?

Mason - I recall that you suggested we switch over to touchstart events everywhere in gaia because they were faster. Is this still the recommended strategy?
Flags: needinfo?(mchang)
Flags: needinfo?(kgrandon)
Flags: needinfo?(felash)
If it's just a matter of doing local testing I can add printfs in the c++ code when the touchstart is received and a log inside clickIcon and compare the timestamps. Would that be sufficient?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> If it's just a matter of doing local testing I can add printfs in the c++
> code when the touchstart is received and a log inside clickIcon and compare
> the timestamps. Would that be sufficient?

Yeah, I think that should work. Better would probably be to add some logging inside of the system app when it actually gets the webapps-launch event from Gecko I suppose?

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window_factory.js#L113
(In reply to Kevin Grandon :kgrandon from comment #7)
> Mason - I recall that you suggested we switch over to touchstart events
> everywhere in gaia because they were faster. Is this still the recommended
> strategy?

TouchStarts compared to clicks should be faster. The flame device sends touch events every 13ms. So in the best case, I would expect to get a TouchStart, wait 13 ms, then send touchmove/end if it's fast enough. That has to be processed and converted to a 'click'. The problem is that you'd probably still have to wait a bit to ensure that the touch start wasn't part of an actual scroll and was actually a tap. I'm not sure how you would get around that.
Flags: needinfo?(mchang)
I agree that touch*start*s probably arrive significantly (~a few ms) before clicks, but the code as it is right now uses the touch*end* to trigger starting the app. AIUI the difference between touchend and click should be minimal.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> I agree that touch*start*s probably arrive significantly (~a few ms) before
> clicks, but the code as it is right now uses the touch*end* to trigger
> starting the app. AIUI the difference between touchend and click should be
> minimal.

Yes I agree. The time between a touch end / click event should be pretty small.
Hm, so I added a couple of print statements and tapped on a homescreen icon 10 times with and without the patch. My patch does seem to regress time by about 25ms on average. Not really sure why but if I have some time I'll try to dig into this.

Without patch:
Samples: 10
Average:     173.80
Stddev:      23.54
Max: 203.167
Min: 117.429

With patch:
Samples: 10
Average:     198.15
Stddev:      28.19
Max: 246.313
Min: 137.629
My data above reported the time from touchstart being received in gonk to the webapps-launch event, which is mostly meaningless because it includes some variable amount of time that my finger was down.

If I measure from the touchend being received in gonk to the webapps-launch event, my patch still causes a regression, but the regression is from ~19ms to ~34ms which is around ~15ms. 10ms of that is actually intentional because of the code at [1], and another 3ms is the time taken between TabChild getting the touch-up event and the single-tap event from the APZ (more than I was expecting).

It seems like the largest chunk of the delay here is intentional (for :active pseudeoelement triggering) but since the homescreen icons don't have :active pseudoelements, as far as I can tell, it's a wasted delay. Maybe we should have some way for content to tell gecko not to include that delay?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=3a53ee57c736#110
Nice investigation Kats. What are the cases when we need active, is it only for :active CSS selectors? I wonder if having "no" :active selectors on the content would be enough of a hit? It seems like an opt-in instead of opt-out strategy would be useful here?
Yeah I think it really only affects :active CSS selectors. Maybe also :focus selectors, I'm not sure. Vivien might be able to provide further insight since he was the one who filed bug 972081 which added this code.
Flags: needinfo?(21)
(In reply to Kevin Grandon :kgrandon from comment #15)
> It seems like an opt-in instead of opt-out
> strategy would be useful here?

But yeah, it does seem like we should be able to skip this delay if the target element doesn't have any :focus or :active CSS selectors. I don't know how easy it is to check that for that sort of thing.
(In reply to Kevin Grandon :kgrandon from comment #7)
> On second thought, I am still thinking of the best way to actually test this
> and don't have a really good idea yet. Adding listeners might work, but is a
> bit wrong if we have to inject a touchstart listener to capture a timestamp
> in the click patch. I'd like to ni? a few people for information.
> 
> Julien - Do you remember how you got the timing information in bug 1038176
> comment 39? At which point did you add logs?

I don't really remember, sorry :(

> 
> Mason - I recall that you suggested we switch over to touchstart events
> everywhere in gaia because they were faster. Is this still the recommended
> strategy?

yeah, I didn't use "touchstart" here because we want to be able to distinguish longpress, for example.

Note that I started some work in bug 1043834 (not sure the patch still works in current master) because this is bugging me too. This is still using "touchend" and the timeout, but should launch the app if the homescreen is scrolling slowly.

If we can get rid of all this and simply use "click" instead, I'd be glad :) But I'm not sure this is possible without regressing the launch time. 20ms is not much, but I think we need it. 5 times 20 gives 100 :)

If you see it regresses by less than this, then I'm not against removing this behavior.
Flags: needinfo?(felash)
Also, I think Vivien has a plan to prelaunch (in the "prerender" way) the app on "touchstart"; in that case, we can definitely use "click" instead of "touchend" because this would not impact the app launch time anymore.
I wrote a patch to skip the 10ms delay in the case where the :active style has no effect to see if that would help, and it did. Now the regression to startup time is down to ~3-5ms. We gain some time by not running touch listeners, but we lose some time because (a) it takes time for the single-tap notification to come from the APZ and (b) we have to dispatch the mouse events before we can dispatch the click event. In the end it's a net loss for app startup time but because we're not listening to touch events you start scrolling more responsively (and of course it fixes this bug).
This is the gecko-side patch. It might be worth landing this regardless of whether we do the gaia side thing.
Attachment #8529945 - Flags: review?(botond)
Comment on attachment 8529945 [details] [diff] [review]
Don't wait 10ms if active state has no effect

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

::: gfx/layers/apz/util/ActiveElementManager.h
@@ +54,5 @@
>     */
>    void HandleTouchEnd(bool aWasClick);
> +  /**
> +   * @return true iff the currently active element (or one of its ancestors)
> +   * actually had a style for the :active pseudeo-class. The currently active

pseudo
Attachment #8529945 - Flags: review?(botond) → review+
Comment on attachment 8506158 [details] [review]
Github PR

With the gecko-side patch the regression is only a few milliseconds (~3-5). Is that acceptable? I rebased the gaia PR on master.
Flags: needinfo?(21)
Attachment #8506158 - Flags: review?(kgrandon)
Whiteboard: [leave open]
Comment on attachment 8506158 [details] [review]
Github PR

I feel that a ~3-5 ms delay is acceptable for the much-cleaned up code, and the maintainability improvements. Thank you for the work, deleting code never felt so good.
Attachment #8506158 - Flags: review?(kgrandon) → review+
checkin-needed to merge the gaia PR. thanks!
Keywords: checkin-needed
Assignee: nobody → bugmail.mozilla
Whiteboard: [leave open]
The failures on gaia-try seem unrelated and to be harness issues. Landed in master: https://github.com/mozilla-b2g/gaia/commit/0a48c64d152fac024679c74672ceb7f9fd956535
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: