Closed Bug 742540 Opened 11 years ago Closed 10 years ago

Clicks fired near a box with a link can trigger the link


(Firefox for Android Graveyard :: General, defect)

Not set


(blocking-fennec1.0 -)

Firefox 16
Tracking Status
blocking-fennec1.0 --- -


(Reporter: wesj, Assigned: wesj)




(1 file, 2 obsolete files)

Our clicking code looks first to see if you tapped on a link and fires the click there. If not, it looks if you tapped near a link clicks there. If neither of those happens, it fires a click on whatever you're pointing at.

We also have a bit of code sending mouse vents that moves the point if its outside the element. It also tries to correct for some fractional pixel stuff that may be going on (copied from XUL).

Sometimes, when we are not attempting to click on a link, we decide to move the point to the center of the element anyway (I think this must be due to the fractional pixel stuff here?), which can actually position the click over a child link.

We should probably just only move these points to the edge of the element, rather than moving them to the middle. Patch coming.
Attached patch Patch(es) (obsolete) — Splinter Review
So this is a product of me spending to much time looking at this. It does three things:

1.) Don't bother with this moving junk if the element isn't clickable
2.) Don't bother with this fractional pixel stuff? Java is sending us ints in page coordinates. I don't think we have the same worries XUL had, but I'm not sure... Hoping maybe mbrubeck will be smarter than I.
3.) Move the click to the edge of the element rather than its middle

I'm happy to remove any of these bits, but was curious if you'd have any opinions on them mbrubeck.
Assignee: nobody → wjohnston
Attachment #612354 - Flags: review?(mbrubeck)
Comment on attachment 612354 [details] [diff] [review]

The approach looks correct, but I think there are some bugs; the patch should probably be tested more.

>+          this._sendMouseEvent("mousemove", element, data.x, data.y, isClickable);
>+          this._sendMouseEvent("mousedown", element, data.x, data.y, isClickable);
>+          this._sendMouseEvent("mouseup",   element, data.x, data.y, isClickable);

>+  _sendMouseEvent: function _sendMouseEvent(aName, aElement, aX, aY, aButton, aCanMovePoint) {

You are passing aButton=isClickable in the calls above.  Did you mean to pass aCanMovePoint=isClickable instead?  (Is it time to move to named parameters passed in an object?)

>         let rect = {x: rects[0].left, y: rects[0].top, w: rects[0].width, h: rects[0].height};
>         if (rect.w == 0 && rect.h == 0)
>           return;
>+        aX = Math.min(rect[0].left + rect[0].width, Math.max(rect[0].left, aX));
>+        aY = Math.min(rect[0].left + rect[0].width, Math.max(rect[0].top,  aY));

The last line should use (top + height) instead of (left + width).

"rect[0]" should be "rects[0]" in the last two lines.

You can clean up this old code by changing the first line to "let rect = rects[0];" and adjusting the other lines accordingly.
Attachment #612354 - Flags: review?(mbrubeck) → review-
Possible soft blocker.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → -
Attached patch Patch v2 (obsolete) — Splinter Review
Rebased. Fixed up. And tested much harder this time.
Attachment #612354 - Attachment is obsolete: true
Attachment #633363 - Flags: review?(mbrubeck)
Attached patch PatchSplinter Review
Forgot to qref....
Attachment #633363 - Attachment is obsolete: true
Attachment #633363 - Flags: review?(mbrubeck)
Attachment #633364 - Flags: review?(mbrubeck)
Comment on attachment 633364 [details] [diff] [review]

I notice that aButton / aParams.button is not actually used anywhere in our code.  You could just remove it if you feel like it.
Attachment #633364 - Flags: review?(mbrubeck) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.