789358 - Enable event-target fluffing for b2g

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Attachment: Flip the prefs

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.

Comment 1

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

Attachment: Enable, plus enlarge radii

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?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

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

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.

Comment 4

[Dietrich Ayala (:dietrich)]

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 6

[Andrew Overholt [:overholt]]

(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.

Comment 7

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

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 9

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

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.

Comment 10

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

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?

Comment 11

[Wesley Johnston (:wesj)]

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.

Comment 12

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

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

Comment 13

[cajbir (:cajbir)]

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?

Comment 14

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

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

Comment 15

[cajbir (:cajbir)]

Yes, I followed the STR in comment 9.

Comment 16

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

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).

Comment 17

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

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?)

Comment 18

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

Attachment: Enable hit-target fluffing for b2g

Comment 19

[Wesley Johnston (:wesj)]

(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.

Comment 20

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

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

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

Easy review!

Comment 21

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

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

Comment 22

[Patryk Adamczyk [:patryk] UX]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Dietrich Ayala (:dietrich)]

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

Comment 25

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

Could be.  We need to tune the params here.

Comment 26

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

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Matt Brubeck (:mbrubeck)]

(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.

Comment 29

[Matt Brubeck (:mbrubeck)]

Attachment: re-enable mouse event fluffing (but not touch)

Comment 30

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

(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?

Comment 31

[Matt Brubeck (:mbrubeck)]

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.

Comment 32

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

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.

Comment 33

[Matt Brubeck (:mbrubeck)]

Attachment: re-enable event target fluffing

Comment 34

[Matt Brubeck (:mbrubeck)]

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

I'll land this on m-c shortly.

Comment 35

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

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.

Comment 36

[Matt Brubeck (:mbrubeck)]

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

Comment 37

[Matt Brubeck (:mbrubeck)]

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

Comment 38

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

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

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

Comment 39

[Matt Brubeck (:mbrubeck)]

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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

[Brendan Eich [:brendan]]

(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

Comment 42

[cmtalbert]

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).

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 45

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

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

Comment 46

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

Attachment: Patch - Enable it again.

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).

Comment 47

[Matt Brubeck (:mbrubeck)]

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).

Comment 48

[Matt Brubeck (:mbrubeck)]

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.

Comment 49

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

(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.

Comment 50

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

(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!

Comment 51

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

(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 :/

Comment 52

[Matt Brubeck (:mbrubeck)]

(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

Comment 54

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

Attachment: bug789358.patch

Re-enable event target fluffing

Comment 55

[Gregor Wagner [:gwagner]]

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

Comment 56

[Ryan VanderMeulen [:RyanVM]]

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

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we do.

Comment 59

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

That's right this is way too risky now.