Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 12 years ago

I had a problem with touch events on Google Maps when running with this patch, but I can't rule out gmaps just being wonky.  Touch handling is quite iffy there.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 1 • 12 years ago

roc, with this patch applied I don't see any difference in touch behavior with a (too-small) checkbox in the gaia UI.

Does your fluffing code work for internal gecko targets, like checkboxes, radio buttons, etc?  Or maybe I'm configuring this incorrectly?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 12 years ago

Comment on attachment 661025 [details] [diff] [review]
Enable, plus enlarge radii

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

The patch looks OK.

It does work with internal gecko targets. See http://dxr.mozilla.org/mozilla-central/layout/base/PositionedEventTargeting.cpp.html?string=IsElementClickable#l120. There's a testcase for that too. So I'm not sure why it's not working for you.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 3 • 12 years ago

Guys, do we have someone who can investigate this?  While I don't think we have enough compat testing to know whether it's blocking+ yet, given fennec's experience we almost certainly need this.

roc and I pretty overloaded atm.

	Dietrich Ayala (:dietrich)
	Comment 4 • 12 years ago

Maybe post to dev-b2g & dev-gaia? Is this a device-specific configuration tweak?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 12 years ago

No, this is general product feature. Just apply Chris' patch and try pressing near buttons etc.

	Andrew Overholt [:overholt]
	Comment 6 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Guys, do we have someone who can investigate this?

I'll see if I can find someone.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 7 • 12 years ago

Comment on attachment 661025 [details] [diff] [review]
Enable, plus enlarge radii

Inferring f+ based on previous comment; this patch doesn't appear to be doing anything wrong.

All the "20mm" numbers I just pulled out of my hat for testing.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 12 years ago

We just tried this patch on Chris Double's Otoro phone, in the browser, and it worked fine. The testcase was a Web page containing nothing but <input type="checkbox">.

cjones, can you give specific steps to reproduce your problem?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 9 • 12 years ago

Sure.
 (1) Open "Settings" app
 (2) Go to "Wi-Fi"
 (3) Try to connect to secured network
 (4) In the popup dialog, try to tap the "show password" checkbox

The box is *way* too small (that's a gaia bug), but it wasn't any easier to hit with the patch here applied.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 10 • 12 years ago

Tried again with a build from today, and this patch still doesn't make a difference in that scenario.

Maybe some of the b2g/chrome/content JS is interfering with things?

	Wesley Johnston (:wesj)
	Comment 11 • 12 years ago

This fluffing range is huge. 2cm on each side of the finger? We use something like 3mm on Fennec. Everything inside the rect is ~equal weight (except visited links). Maybe reducing those huge rects will help? I don't have gaia around to check, but the fluffing only looks for role="button", certain element types, or elements with mouse listeners attached (not touch). Just some things to check.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 12 • 12 years ago

The fluffing size is intentionally too-large, for testing purposes.

	cajbir (:cajbir)
	Comment 13 • 12 years ago

I applied the fluffing patch. A couple of days ago checkboxes wouldn't click. Today with a "repo sync" of B2G and latest mozilla-central, checkboxes do click correctly. Did something land recently to fix things?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 14 • 12 years ago

Not to my knowledge.  You're following the STR in comment 9?

	cajbir (:cajbir)
	Comment 15 • 12 years ago

Yes, I followed the STR in comment 9.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 16 • 12 years ago

I've always been able to check the checkbox, but if I tapped very close but not quite on the hit target (for example the bottom-right corner of the [ ]), then it wouldn't check.  The patch here didn't change that.

If something has changed to make that work, \o/ (sort of).

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 17 • 12 years ago

Boy howdy, this patch is sure working now!

We should pick values based on UX input, but I'm guessing the default values in all.js were taken from fennec experience, so we can land with just the .enabled prefs flipped for now.

(Aside: does the fluffing take presshell resolution into account?)

	Wesley Johnston (:wesj)
	Comment 19 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> but I'm guessing the default values in all.js were taken from fennec experience, 
They weren't. Fennec currently uses about 3mm on the left and right sides of the point, 5 on top and 2 on bottom (with some rounding).

http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#406

> (Aside: does the fluffing take presshell resolution into account?)
Yes.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 20 • 12 years ago

Comment on attachment 664423 [details] [diff] [review]
Enable hit-target fluffing for b2g

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

Easy review!

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 21 • 12 years ago

The default params here match my right index finger pretty well (~19cm wide), but not the fennec defaults.  Let's overshoot for a bit and then let UX correct.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3688310dce70

	Patryk Adamczyk [:patryk] UX
	Comment 22 • 12 years ago

From my experience working on other mobile OSs and what most of our competitors do. We want to have touch targets fall in the following ranges:
Square Items: 7x7mm minimum with an ideal 9x9mm, anything beyond size doesn't really benefit the user much.
Row Items: 5.5mm

On our Otoro device this would translate to:
Square Items: 45 - 58 px range
Row Items: 36 px

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/3688310dce70

	Dietrich Ayala (:dietrich)
	Comment 24 • 12 years ago

did this cause? https://github.com/mozilla-b2g/gaia/issues/5290

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 25 • 12 years ago

Could be.  We need to tune the params here.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 26 • 12 years ago

The too-large tap targets have been causing issues for me, especially in the new maps app (settings after that).  Bumped them down to the fennec params in

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f04c4993a2

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/c7f04c4993a2

	Matt Brubeck (:mbrubeck)
	Comment 28 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #26)
> The too-large tap targets have been causing issues for me, especially in the
> new maps app (settings after that).  Bumped them down to the fennec params in
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f04c4993a2

This not only changed the radius prefs, but actually disabled "fluffing" entirely for Firefox OS.  It looks like this was unintentional, right?

By the way, I think you might want to enable fluffing for mouse events only, not for touch events.  This may resolve some of the issues you are seeing with touch apps.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 30 • 12 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #28)
> By the way, I think you might want to enable fluffing for mouse events only,
> not for touch events.  This may resolve some of the issues you are seeing
> with touch apps.

Why is that?

	Matt Brubeck (:mbrubeck)
	Comment 31 • 12 years ago

I find the re-targeting most useful for clicking form elements or text links on pages that were designed for desktop, and so have very small, non-touch-friendly click targets.  Pages that actually use touch events are generally designed with touch friendliness in mind.

(Perhaps a different way to address that difference would be to use retargeting only on pages with "desktop"-sized viewports.)

That said, we currently have both prefs enabled in Metro, and it sounds like that also worked for you in B2G with the decreased radii.  So if you'd like I can update my patch to re-enable both.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 32 • 12 years ago

That makes sense, but it seems that pages designed to be touch-friendly would either (i) succeed at having large enough hit targets, in which case the target fluffing wouldn't make a difference; or (ii) not succeed, in which case this patch could help.  So unless there's a significant perf difference (we didn't see one), I think I'd prefer to leave both enabled.

	Matt Brubeck (:mbrubeck)
	Comment 34 • 12 years ago

https://hg.mozilla.org/projects/elm/rev/10d969deb148

I'll land this on m-c shortly.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 35 • 12 years ago

Comment on attachment 705718 [details] [diff] [review]
re-enable event target fluffing

Patch with \epsilon risk that improves touch responsiveness and brings b2g closer in line with fennec.

	Matt Brubeck (:mbrubeck)
	Comment 36 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2692ef57acaa

	Matt Brubeck (:mbrubeck)
	Comment 37 • 12 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b348cb87392b

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 38 • 12 years ago

Backed out for bug 834883, which I have no idea about :(

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c8d3180cc593

	Matt Brubeck (:mbrubeck)
	Comment 39 • 12 years ago

Backed out on inbound too for bug 834883:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6baaa5cff198

Unassigning; this needs someone with a b2g device to do some testing.

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/2692ef57acaa

	Brendan Eich [:brendan]
	Comment 41 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #35)
> Comment on attachment 705718 [details] [diff] [review]
> re-enable event target fluffing
> 
> Patch with \epsilon risk that improves touch responsiveness and brings b2g
> closer in line with fennec.

Alas epsilon was 1 not 0 this time :-P.

No manual test coverage prior to landing?

Automated regression test coverage wanted.

/be

	cmtalbert
	Comment 42 • 12 years ago

We do have automated coverage that should have caught this.  These are the Gaia UI tests which use touch events. However, they are currently being hidden due to fall out from several touch and system issues that we're trying to fix. The last one (we hope) is this intermittent issue: https://bugzilla.mozilla.org/show_bug.cgi?id=834266  Once we get them green again, we'll unhide them.

The tracking bug to stand up the gaia UI tests per changeset is: https://bugzilla.mozilla.org/show_bug.cgi?id=802317 if you want to follow along.

You can see these by adding a noignore=1 flag to your tbpl URL.  However, until we get them green they're not too useful.  Here they are for this push: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2692ef57acaa&noignore=1 (They are the "G" elements on the B2G Panda Opt and B2g Panda Opt g-c (gaia central) lines).

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 43 • 12 years ago

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 45 • 12 years ago

Borderline tech-bug (we've managed to work around this in core UI), definitely major content issue and needed for competitive parity.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 46 • 11 years ago

Let's re-enable it again. With one more pref this time in order to make sure mouse event fluffing is enabled since it seems like it is disabled by default http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#104 which can have been the source of some errors (I could be wrong).

	Matt Brubeck (:mbrubeck)
	Comment 47 • 11 years ago

Comment on attachment 748834 [details] [diff] [review]
Patch - Enable it again.

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

::: b2g/app/b2g.js
@@ +634,5 @@
>  
> +// Touch events are sometimes converted to mouse events if a click event
> +// should be generated. So this is important to enable fluffing for mouse
> +// events as well.
> +pref("ui.mouse.radius.inputSource.touchOnly", false);

This pref is meant for testing only; see bug 832101 for details.  It should not be necessary to change this pref on actual devices, as long as you are setting inputSource correctly for simulated mouse events (bug 833663).

	Matt Brubeck (:mbrubeck)
	Comment 48 • 11 years ago

By the way, Metro Firefox is currently the only product with click fluffing enabled, and we have found that bug 837242 can cause some serious problems.  We should make sure to fix that bug before shipping click fluffing in either Metro or Firefox OS.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 49 • 11 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #47)
> Comment on attachment 748834 [details] [diff] [review]
> Patch - Enable it again.
> 
> Review of attachment 748834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/app/b2g.js
> @@ +634,5 @@
> >  
> > +// Touch events are sometimes converted to mouse events if a click event
> > +// should be generated. So this is important to enable fluffing for mouse
> > +// events as well.
> > +pref("ui.mouse.radius.inputSource.touchOnly", false);
> 
> This pref is meant for testing only; see bug 832101 for details.  It should
> not be necessary to change this pref on actual devices, as long as you are
> setting inputSource correctly for simulated mouse events (bug 833663).

Oh I was developing against b2g-18 that does not contains the change from bug 832101! I will backport the patch from bug 832101 for testing and remove the pref change. Thanks for the pointer.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 50 • 11 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #48)
> By the way, Metro Firefox is currently the only product with click fluffing
> enabled, and we have found that bug 837242 can cause some serious problems. 
> We should make sure to fix that bug before shipping click fluffing in either
> Metro or Firefox OS.

This patch does not target 1.0.1 or 1.1 (except if product decide that it worth it?). So it is safe to land it but I agree that the underlying bug should be fixed. I wish I can find some cycles to look at it. Again thanks for the pointer!

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 51 • 11 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #50)
> (In reply to Matt Brubeck (:mbrubeck) from comment #48)
> > By the way, Metro Firefox is currently the only product with click fluffing
> > enabled, and we have found that bug 837242 can cause some serious problems. 
> > We should make sure to fix that bug before shipping click fluffing in either
> > Metro or Firefox OS.
> 
> This patch does not target 1.0.1 or 1.1 (except if product decide that it
> worth it?). So it is safe to land it but I agree that the underlying bug
> should be fixed. I wish I can find some cycles to look at it. Again thanks
> for the pointer!

Sounds like we need to fix it before landing this patch. With this patch I can't answer phone calls anymore. I'm not sure why exactly but it seems like I can unlock the phone trying to answer a phone call while the lockscreen is behind the call screen :/

	Matt Brubeck (:mbrubeck)
	Comment 52 • 11 years ago

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #51)
> Sounds like we need to fix it before landing this patch. With this patch I
> can't answer phone calls anymore. I'm not sure why exactly but it seems like
> I can unlock the phone trying to answer a phone call while the lockscreen is
> behind the call screen :/

Yes, in Metro we had to work around bug 837242 by adding no-op "onclick" handlers to any non-clickable element that covers a clickable element, for example:
http://hg.mozilla.org/mozilla-central/file/81dd97739fa1/browser/metro/base/content/browser.xul#l392

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 54 • 11 years ago

Re-enable event target fluffing

	Gregor Wagner [:gwagner]
	Comment 55 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/fb2ba6ff6fb2

	Ryan VanderMeulen [:RyanVM]
	Comment 56 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/fb2ba6ff6fb2

	Ryan VanderMeulen [:RyanVM]
	Comment 57 • 11 years ago

This is showing up in my uplift queries due to the old approval. Do we still want this on b2g18? If so, please nom for leo blocking.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 58 • 11 years ago

I don't think we do.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 59 • 11 years ago

That's right this is way too risky now.