1220770 - Reduce the top value for the PositionedEventTargeting logic

Status: RESOLVED WONTFIX

(Core :: Panning and Zooming, defect)

Description

[Julien Wajsberg [:julienw]]

Attachment: testcase-delegation.html

[Blocking Requested - why for this release]:

In bug 1210201 we discovered that the logic introduced in bug 1181763 could have bad side-effects in very specific situations, but that are completely legit.

In this bug I'd like to reduce the default value we use for the "top" preference because it seems too big compared to the "bottom" preference. While it's still not perfect it should at least allow the user to carry his actions, should the issue appear on the web.

As a STR you can:
1. load up the attachment.
2. try to tap right below the existing "+"
=> you'll see it's difficult.
3. activate 2 squares, skipping one, and then try to enable the one is in the middle.
=> It's virtually impossible.

Because the issue happens on Fennec too, I'd suggest to change the values for fennec as well.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: 0001-Bug-1210201-Reduce-the-top-constants-for-the-Positio.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8682069 [details] [diff] [review]
0001-Bug-1210201-Reduce-the-top-constants-for-the-Positio.patch

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

I have no objection to this, but I can't r+ it either - it should be up to the UX/product owners to make this call.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8682069 [details] [diff] [review]
0001-Bug-1210201-Reduce-the-top-constants-for-the-Positio.patch

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

I'm in the same position as Kats.

Comment 4

[Julien Wajsberg [:julienw]]

Who should I ask and how can we easily explain them the problem ?

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8682069 [details] [diff] [review]
0001-Bug-1210201-Reduce-the-top-constants-for-the-Positio.patch

Chiming in from UX, I'm not sure what context I'm missing but I'm having trouble following what's happening here. Could you help me understand a bit of what's going on here?

Comment 6

[Mahendra Potharaju [:mahe]]

Hey Julien, 

UX needs your input on this one. 

Thanks

Comment 7

[Julien Wajsberg [:julienw]]

Let me try to explain.

We have a feature, which I could call "Position Event Retargeting" that works like this:
when the user touch a location on the screen, and that location is not something "tappable" (or "clickable"), then we try to "look around" and find something that is actually tappable. We could call it "fat finger fix".

So the radius we use to "look around" is different on the 4 directions. Currently it's:
* look in the top direction up to 5 mm
* look in the bottom direction up to 2mm
* look in the left and directions up to 3 mm

We use the same values for B2G/B2GDroid/Fennec.

We have one difference between these platforms currently: B2G uses this for both the mouse events (click, mousedown, mouseup, mousemove) and the touch events (touchstart, touchmove, touchend), while other platforms use it only for the mouse events. We want to sync B2G with the other platforms in bug 1091889 but it seems there are issues we need to investigate.


So in Bug 1181763 we extended this a bit, by "looking around" and trying to find a child even if the tap targeted a tappable target:


-----------------------
| parent              |
|      ---------      |
|      | child |      |
|      --------       |
|                     |
-----------------------


The idea is that it's usual in the web to use event delegation and put the "click" event handler on the parent instead of the child (for example if we have several children that would share the same behavior). Because it's usual the previous "retargeting" behavior was too restrictive and was missing a lot of real-life cases.


But we found in Calendar at least one case (bug 1210201) where this new behavior provoked an issue: getting the event on the proper position was something we wanted, even if there is a nearby child. The testcase in 8682068 tries to show this. See also comment 0 for more information about how to use it.
You can also try it on Desktop by enabling the pref "ui.mouse.radius.enabled" and setting "ui.mouse.radius.inputSource.touchOnly" to false (but the radius values for settings are bigger, you can change them too, search for "radius").


The patch here is trying to reduce the likeliness of the issue appearing while still keeping the new behavior, and for all 3 platforms. The impact is that we'll "look around" less far to the top direction, actually as far as we already do for the bottom direction (all other values are unchanged).


I also proposed a more future-proof way to deal with the issue in bug 1220774 (although kats would rather just do bug 1091889 -- to be further discussed :) ).


Hope this clears up the issue here. Please NI me in case you have more questions !

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Ah, thanks for clearing this up Julien, but I don't work on B2G. I wouldn't be comfortable making this call on their behalf since they work with much different devices.

SOrry!

Comment 9

[Julien Wajsberg [:julienw]]

Hey Anthony, the goal here is to change B2G, B2GDroid AND Firefox/Android. So the NI to you was only for Firefox/Android.

NI Fxos-ux for FxOS.

Comment 10

[Tiffanie Shakespeare [:tif]]

From UX triage:

Is the issue that the touch target isn't matching the visual target? Sorry, we aren't totally following the issue or what you need from us.

Thanks

Comment 11

[Julien Wajsberg [:julienw]]

Tiffanie, what I want to change is the amount of distance we use to look around (to the top direction) when the user taps somewhere.

I was told that I need a UX feedback on this change before doing anything. The easiest is likely to load attachment 8682068 [details] on B2G and/or Firefox/Android and try to do what I say in comment 0.

Please tell me if this is better like this.

Comment 12

[Tiffanie Shakespeare [:tif]]

From UX triage:

This seems like this would solve the issue. Is it possible to try out a patch before landing?

Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

Tiffanie, I don't know how to give you something to test. The patch is attached to this bug and merely changes some default settings. You can directly change it on your own phone using WebIDE:

0. Enable ADB and Devtools in Settings > Developer
1. connect to your phone using WebIDE
2. open "Device Settings" (the button is at the bottom right of WebIDE's window)
3. search for "ui.touch.radius.topmm"
4. Change the value to 2, press enter.
5. reboot your phone.

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Julien Wajsberg [:julienw] from comment #0)
> Created attachment 8682068 [details]
> testcase-delegation.html
> 
> [Blocking Requested - why for this release]:
> 
> In bug 1210201 we discovered that the logic introduced in bug 1181763 could
> have bad side-effects in very specific situations, but that are completely
> legit.
> 
> In this bug I'd like to reduce the default value we use for the "top"
> preference because it seems too big compared to the "bottom" preference.
> While it's still not perfect it should at least allow the user to carry his
> actions, should the issue appear on the web.
> 
> As a STR you can:
> 1. load up the attachment.
> 2. try to tap right below the existing "+"
> => you'll see it's difficult.
> 3. activate 2 squares, skipping one, and then try to enable the one is in
> the middle.
> => It's virtually impossible.
> 
> Because the issue happens on Fennec too, I'd suggest to change the values
> for fennec as well.

Looking at this some more, I'm not sure I fully understand the implications for Firefox on Android. 

The problem you're trying to solve here seems strictly for FFOS so I'm unclear what effects it will have on Android. Perhaps I'm mistaken? :)

Also, we recently worked on bug 1198463 that seems like it will be affected by this change. Are these related?

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Anthony Lam (:antlam) from comment #14)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > Created attachment 8682068 [details]
> > testcase-delegation.html
> > 
> > [Blocking Requested - why for this release]:
> > 
> > In bug 1210201 we discovered that the logic introduced in bug 1181763 could
> > have bad side-effects in very specific situations, but that are completely
> > legit.
> > 
> > In this bug I'd like to reduce the default value we use for the "top"
> > preference because it seems too big compared to the "bottom" preference.
> > While it's still not perfect it should at least allow the user to carry his
> > actions, should the issue appear on the web.
> > 
> > As a STR you can:
> > 1. load up the attachment.
> > 2. try to tap right below the existing "+"
> > => you'll see it's difficult.
> > 3. activate 2 squares, skipping one, and then try to enable the one is in
> > the middle.
> > => It's virtually impossible.
> > 
> > Because the issue happens on Fennec too, I'd suggest to change the values
> > for fennec as well.
> 
> Looking at this some more, I'm not sure I fully understand the implications
> for Firefox on Android. 
> 
> The problem you're trying to solve here seems strictly for FFOS so I'm
> unclear what effects it will have on Android. Perhaps I'm mistaken? :)

Well, the problem is in Android as well. Did you try the testcase with Firefox/Android ?

 
> Also, we recently worked on bug 1198463 that seems like it will be affected
> by this change. Are these related?

Not sure, let's ask Dominique.

Comment 16

[Dominique Vincent [:domivinc]]

Sorry, I cannot test the issue before the end of the week but here some comments (not verified):

1- the issue should be visible on Android too,

2- there is no reason to only change the pref values vertically, you just have to make a test case horizontally, and you will have the same issue horizontally.
To fix it, you will propose to set ui.touch.radius.leftmm = 2 and ui.touch.radius.rightmm=2
With this change it won't be easy to click on small links.

3- the visited links get a better place in the search process to return the closest link from the the touch point. The issue could come from that point?

4- on Android using Nightly, the test case should open the zoomed view to allow the user to select the correct button. In this case, it should not be an issue any more.

5- on Android using Nightly, the touch area is larger after Bug 1222234 (8mm).

Comment 17

[Julien Wajsberg [:julienw]]

Yeah, I agree that with the zoomed view implementation, we likely don't need it in Android. This should be what we want in the future for B2G too.

I didn't change the horizontal values because it was already low (3) compared to the top value (5) (the bottom value is 2).

Comment 18

[Tiffanie Shakespeare [:tif]]

Julien - would it be possible to meet up in Orlando for like 5 minutes to get this closed? Ping me on IRC and we'll set up a time.

Thanks!

Comment 19

[Julien Wajsberg [:julienw]]

I think we totally forgot about it while in Orlando :/ I'll capture a video.

Comment 20

[Julien Wajsberg [:julienw]]

Hey Tiffanie, see https://youtu.be/9xk5pI0NgdE for more information.

The phone on the left has the pref change. The phone on the right has the current pref.

You can see that for apps that are coded in a specific way (as described in comment 7) some actions are really difficult; in this case, it's very difficult to click between 2 existing links.

With the smaller value for the pref, we can still occasionally miss it, but it's a lot easier.

Hope this helps.

Comment 21

[Tiffanie Shakespeare [:tif]]

From UX triage:

Thanks Julien for posting the video! :) Definitely seems easier to tap on the items - looks good. Thanks for your work Julien!

Comment 22

[Julien Wajsberg [:julienw]]

Attachment: patch v2 - only B2G & B2GDroid

Because Firefox/Android uses the zoomed view, I excluded it from the patch. (it doesn't work for me on the attachment, but well, I guess this is a separate bug).

So here is the patch for only B2G and B2G Droid. How does that look ?

Comment 23

[Julien Wajsberg [:julienw]]

(In reply to Dominique Vincent [:domivinc] from comment #16)
> Sorry, I cannot test the issue before the end of the week but here some
> comments (not verified):
> 
> 1- the issue should be visible on Android too,

Just checked, it is :)

> 2- there is no reason to only change the pref values vertically, you just
> have to make a test case horizontally, and you will have the same issue
> horizontally.
> To fix it, you will propose to set ui.touch.radius.leftmm = 2 and
> ui.touch.radius.rightmm=2
> With this change it won't be easy to click on small links.

Even if the link is a 1x1 pixel, it would yield a 6x4 surface. Not that bad for something that's obviously bad at the start.

> 
> 3- the visited links get a better place in the search process to return the
> closest link from the the touch point. The issue could come from that point?

No I don't think this is the issue here.

> 
> 4- on Android using Nightly, the test case should open the zoomed view to
> allow the user to select the correct button. In this case, it should not be
> an issue any more.

It doesn't on attachment 8682068 [details] :/ Try to reproduce what I do in https://youtu.be/9xk5pI0NgdE
Should you file a separate bug ?

> 
> 5- on Android using Nightly, the touch area is larger after Bug 1222234
> (8mm).

Indeed, and the issue is even more visible now on Android :/

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8700021 [details] [diff] [review]
patch v2 - only B2G & B2GDroid

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

The patch looks ok to me, but as it's a product decision rather than platform code I'll leave the official review to fabrice.

Comment 25

[Dominique Vincent [:domivinc]]

(In reply to Julien Wajsberg [:julienw] (PTO -> 2016) from comment #23)

> It doesn't on attachment 8682068 [details] :/ Try to reproduce what I do in
> https://youtu.be/9xk5pI0NgdE
> Should you file a separate bug ?
> 
> > 

I spent more time on your test page, and I don't think it's a bug in the Android/B2G version of Firefox. 
The html structure of your test page is really weird: the list of "<a>" links are not present when the page is loaded. You just added the squares using css and the user think that there are some buttons. In fact some buttons are created after a first click using a script!
When the page is loaded, there is no reason to open the zoomed view in this case, and the closest link behavior works without any issue.

I really think that we should not take this type of weird page as a "standard" mobile web page. With your changes on "ui.touch.radius**", we will "deactivate" the closest link behavior and the zoomed view behavior only for a very specific case. 
You should try to rebuild your test page using a list of "<a>" links and it should work!

Comment 26

[Julien Wajsberg [:julienw]]

Dominique, the page is weird because it's a simpler testcase for a real-world case that makes sense. That's the Calendar application in Firefox OS. You can have a look at this app and see that this actually makes more sense.
(in the Calendar app I've added a workaround in bug 1210201 so this works as expected by merely ignoring the retargeting algorithm)

Yes, we could build the app differently, but this is a real-world usage. I'm not saying this is _right_, only that this happens on the web, and that when this happens the user is blocked.

I haven't checked what Chrome/Android does, I'll look at it before landing.

I disagree that we deactivate the closest link behavior with the change. The value '2' was already used for the 'bottom' value anyway... I think it works just as fine and that '5' was too big.

Comment 27

[Dominique Vincent [:domivinc]]

(In reply to Julien Wajsberg [:julienw] (PTO -> 2016) from comment #26)
> 
> I disagree that we deactivate the closest link behavior with the change. The
> value '2' was already used for the 'bottom' value anyway... I think it works
> just as fine and that '5' was too big.

Two things can be used to define the correct size of the touch area:
1- the size of a human fingertip,
2- the parallax (there is a good graphic about parallax here [1])

In bug 1208370 comment 29, Anthony found that 14 mm (7mmx7mm) is a good value for the size of the touch area to simulate a finger. The original value was a little bit smaller (6mm x 7mm). 
After some tests, Anthony decided to use a larger symmetrical area (8mm x 8mm). In this last proposal, the parallax is not taken into account.

The original value was near the size of the fingertip and was designed to take into account the parallax ( bottom = 2mm, top = 5mm).

Your proposal is (6mm x 4mm) and is based on the design of your application.
It doesn't take into account the parallax and the size of a fingertip. Your values are based on the size of your simulated buttons.
Your proposal doesn't fix the issue for all the applications. I can create the same application with some horizontal buttons, and I will have to change the size of the touch area to (4mm x 4mm).
The Calendar application will never work correctly with the closest link behavior. It's better with small values for the touch area but the issue is still visible.

One way, to correctly handle all the cases, could be to add some mozilla prefixed properties (for instance: -moz-touch-radius-top, -moz-touch-radius-bottom, ...) in the css to be able to change the size of the touch area dynamically when the page is loaded. With this solution, if the default values don't fit the page design (your case in the Calendar application), the values can be updated by the page.


[1] http://bridgedesign.com/touch-screen-ergonomics/

Comment 28

[Julien Wajsberg [:julienw]]

I definitely think like you that a correct way to handle all the cases is to improve the platform. The properties you propose is a solution (CSS-based), I proposed another one in bug 1220774 (JS/DOM-based).

I'm closing this for now. I'll come back if I find the issue in a real-world use case. In the Calendar app I worked around this differently.

Thanks for the article, it's very informative. I knew about the parallax effect already but the article explains this very well. (Note: my personal behavior is I unconsciously compensate for the parallax when tapping on a touch screen, likely because most of the app didn't account for it in the past and I had issues with some devices (could be train ticket machines, for example) that accounts for parallax).