563759 - Incorrect arguments to nodesFromRect

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

We're passing the "touch radius" components in BRTL order instead of TRBL.  See http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#625

Comment 1

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

(In reply to comment #0)
> Created an attachment (id=443424) [details]
> patch
> 
> We're passing the "touch radius" components in BRTL order instead of TRBL.  See
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#625

I would be happy to have these lines in the correct order. But it looks like the arguments are swapped somewhere in the implementation of nodesFromRect which result into the wrong behavior if we do that (enlarging the top area instead of the bottom area).

As Mark suggested on IRC we could change the pref value to reflect this change but I think the resulting pref name/value pair will confuse the user who want to customize the values.

I'm not sure where is the source of inversion in the code.
(Also I can be the only one confused by all that! :) )

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Felipe - Does any of this make sense to you? Currently, Fennec is swapping the "top" and "bottom" args when calling nodesFromRect. However, we seem to be getting the desired affect - the touch area is larger below the touch point than above it. Vivien is afraid that if we fix the arguments, the top area will be larger.

Either way, I'd like to at least call nodesFromRect correctly, but it would be nice to have a logical explanation of the behavior.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: testcase

This is just effect of the twisted logic. Note that the parameters to nodesFromRect tells how much to expand the search rect around the touch point. So if the desired effect is "it's easier to click on the bottom of elements", we have to expand the search upwards the point.

See the attached testcase. With the latest trunk, I set touch.bottom to 30 and touch.up to 0. Since the arguments are swapped, this makes up = 30 and bottom = 0. You can see that no click very near the top border of the element will register (because it's not searching downwards), and it's very easy to click below the element.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Thanks for the explanation, Felipe.  That helps a lot.

This patch changes both the argument order and the pref values, so it keeps the current (desired) behavior.  It also adds a brief comment explaining what the values really mean.

Comment 5

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

Comment on attachment 444169 [details] [diff] [review]
patch v2

The patch is right and pass the parameters as expected by nodesFromRect actually.
I was confused because i was thinking as the parameters as a kind of margins around the point x,y.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed m-c:
http://hg.mozilla.org/mobile-browser/rev/9772d862d2da

pushed m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/6cc40511f45f