Closed Bug 563759 Opened 10 years ago Closed 10 years ago

Incorrect arguments to nodesFromRect

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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
Attachment #443424 - Flags: review?(mark.finkle)
Attachment #443424 - Flags: review?(mark.finkle) → review+
(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! :) )
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.
Attached file 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.
Attached patch patch v2Splinter Review
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.
Attachment #443424 - Attachment is obsolete: true
Attachment #444169 - Flags: review?(21)
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.
Attachment #444169 - Flags: review?(21) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.