Closed Bug 628012 Opened 9 years ago Closed 9 years ago

link highlighting and link targeting are inconsistent


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




Tracking Status
fennec 2.0+ ---


(Reporter: blassey, Assigned: wesj)





(1 file, 2 obsolete files)

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

1) load;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
tracking-fennec: --- → ?
OS: Linux → Android
Hardware: x86_64 → ARM
Assignee: nobody → wjohnston
tracking-fennec: ? → 2.0+
Severity: normal → minor
Priority: -- → P1
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #510859 - Flags: feedback?(mark.finkle)
Attached patch Patch v2 (obsolete) — Splinter Review
We started actually sending whether or not a tap might be a click with TapUp, so we can use that now and simplify this.
Attachment #510859 - Attachment is obsolete: true
Attachment #512237 - Flags: review?(mark.finkle)
Attachment #510859 - Flags: feedback?(mark.finkle)
Comment on attachment 512237 [details] [diff] [review]
Patch v2

Waiting for post b5 to review
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);
} else {
  this.tapUp(aEvent.clientX, aEvent.clientY);

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

r+ with nit fixed
Attachment #512237 - Flags: review?(mark.finkle) → review+
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?
Attachment #512237 - Attachment is obsolete: true
Attachment #513572 - Flags: review?(mark.finkle)
Attachment #513572 - Flags: review?(mark.finkle) → review+
Closed: 9 years ago
Resolution: --- → FIXED

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)
You need to log in before you can comment on or make changes to this bug.