628012 - link highlighting and link targeting are inconsistent

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Sometimes when clicking links that are close together, we'll highlight one link and then load another. This is very confusing when it happens.

STR:
1) load http://lassey.us/?C=M;O=D
2) click the top link towards the bottom of its area
3) notice that the top link gets hightlighted blue

Expected results:
top link is loaded

Actual results:
the link immediately below the top link gets a blue outline then loads

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This works by holding a reference to the tapped element and making sure that it is the same one before we fire the click event on it. I wanted to save the element when we do tapHighlight and destroy it when we kill the highlight.

Unfortunately, we fire both TapUp events, and TapSingle in the parent process when you do this, and as such we send both MouseCancel and MouseUp events in the child process. cancelTapHighlight() actually gets called twice, and in most cases the cached element is destroyed before the MouseUp event occurs.

At this point I got a little frustrated and hacked around the problem. This only deletes the element if you pass the same element into cancelTapHighlight.

A better alternative might be to create a distinction in the parent process between a TapUp which is part of a click and a TapUp which isn't. Actually, if we do that, we could potentially kill the MouseUp event entirely and only send one message. Then we can selectively hide the tapHighlight in the child process. That shouldn't be too hard, but is a lot more risky than this simple approach.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v2

We started actually sending whether or not a tap might be a click with TapUp, so we can use that now and simplify this.

Comment 3

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

Comment on attachment 512237 [details] [diff] [review]
Patch v2

Waiting for post b5 to review

Comment 4

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

Comment on attachment 512237 [details] [diff] [review]
Patch v2

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>           case "TapUp":
>             if (aEvent.isClick && !Browser.selectedTab.allowZoom) {
>               this.tapSingle(aEvent.clientX, aEvent.clientY, aEvent.modifiers);
>               aEvent.preventDefault();
>-            } else {
>+            } else if (!aEvent.isClick) {
>               this.tapUp(aEvent.clientX, aEvent.clientY);
>             }

I am a fan of making the logic a bit easier to read


if (aEvent.isClick) {
  if (!Browser.selectedTab.allowZoom) {
    this.tapSingle(aEvent.clientX, aEvent.clientY, aEvent.modifiers);
    aEvent.preventDefault();
  }
} else {
  this.tapUp(aEvent.clientX, aEvent.clientY);
}

>diff --git a/chrome/content/content.js b/chrome/content/content.js

r+ with nit fixed

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Cleaning this up I noticed that the Browser._contextMenu attribute isn't really used anymore, so I just deleted it. An mxr search showed it showing up three times, always being set to null. Just checking to make sure I didn't miss something?

Comment 6

[Wesley Johnston (:wesj)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/646c82353ef2

Comment 7

[Anna (Waverley)]

VERIFIED FIXED on:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110317 Firefox/4.0b13pre Fennec /4.0b6pre 

Device: Motorola Droid 2 (Android 2.2)