Closed Bug 789358 Opened 7 years ago Closed 6 years ago

Enable event-target fluffing for b2g

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- wontfix
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- wontfix

People

(Reporter: cjones, Assigned: vingtetun)

References

(Depends on 1 open bug)

Details

(Whiteboard: [completed-elm][tech-p1])

Attachments

(3 files, 4 obsolete files)

Attached patch Flip the prefs (obsolete) — Splinter Review
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.
Attached patch Enable, plus enlarge radii (obsolete) — Splinter Review
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?
Attachment #659073 - Attachment is obsolete: true
Attachment #661025 - Flags: feedback?(roc)
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.
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.
Maybe post to dev-b2g & dev-gaia? Is this a device-specific configuration tweak?
No, this is general product feature. Just apply Chris' patch and try pressing near buttons etc.
(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 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.
Attachment #661025 - Flags: feedback?(roc) → feedback+
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?
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.
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?
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.
The fluffing size is intentionally too-large, for testing purposes.
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?
Not to my knowledge.  You're following the STR in comment 9?
Yes, I followed the STR in comment 9.
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).
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?)
Assignee: nobody → jones.chris.g
Attachment #661025 - Attachment is obsolete: true
Attachment #664423 - Flags: review?(21)
(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 on attachment 664423 [details] [diff] [review]
Enable hit-target fluffing for b2g

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

Easy review!
Attachment #664423 - Flags: review?(21) → review+
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
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
https://hg.mozilla.org/mozilla-central/rev/3688310dce70
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Could be.  We need to tune the params here.
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
(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.
Assignee: jones.chris.g → mbrubeck
Status: RESOLVED → REOPENED
Attachment #705209 - Flags: review?(jones.chris.g)
Resolution: FIXED → ---
(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?
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.
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.
Attachment #705209 - Attachment is obsolete: true
Attachment #705209 - Flags: review?(jones.chris.g)
Attachment #705718 - Flags: review?(jones.chris.g)
Attachment #705718 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/projects/elm/rev/10d969deb148

I'll land this on m-c shortly.
Status: REOPENED → ASSIGNED
Whiteboard: [completed-elm]
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.
Attachment #705718 - Flags: approval-mozilla-b2g18?
Attachment #705718 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
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.
Assignee: mbrubeck → nobody
(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
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).
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Duplicate of this bug: 731006
Borderline tech-bug (we've managed to work around this in core UI), definitely major content issue and needed for competitive parity.
Whiteboard: [completed-elm] → [completed-elm][tech-p1]
Assignee: nobody → 21
Whiteboard: [completed-elm][tech-p1] → [completed-elm][tech-p1], u=fx-os-user c=may-6-17 p=0
Whiteboard: [completed-elm][tech-p1], u=fx-os-user c=may-6-17 p=0 → [completed-elm][tech-p1], u=fx-os-user c=may-6-17 p=1
Attached patch Patch - Enable it again. (obsolete) — Splinter Review
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).
Attachment #748834 - Flags: review?(fabrice)
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).
Attachment #748834 - Flags: feedback-
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.
Attachment #748834 - Flags: review?(fabrice)
(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.
(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!
Depends on: 837242
(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 :/
(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
Duplicate of this bug: 825672
Whiteboard: [completed-elm][tech-p1], u=fx-os-user c=may-6-17 p=1 → [completed-elm][tech-p1]
Attached patch bug789358.patchSplinter Review
Re-enable event target fluffing
Attachment #748834 - Attachment is obsolete: true
Attachment #784630 - Flags: review?(mbrubeck)
Attachment #784630 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/fb2ba6ff6fb2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
That's right this is way too risky now.
Depends on: 921928
Depends on: 926389
Depends on: 945727
Depends on: 946731
You need to log in before you can comment on or make changes to this bug.