The link location info sometimes does not display.

Yes, I know this ain't blocking. Digging into this to look at delays I saw these other problems (still looking into delays...) 1.) We're not necessarily showing the context menu for the element that's been highlighted by the click helper code. This uses _highlightElement if available, and also caches it to make sure some later title calculations match. 2.) We need to loop over parentNodes to find the title. Google results for instance have a <bold> section for the search term. If you long tap on that, our current code can't find a title for the dialog. 3.) We're hiding the tap highlight when we dispatch the touch event to the page, but that is ~50ms before we hear back and show our own dialog. The delay between the highlight disappearing and the dialog appearing is strange.

Unbitrotted.

Updated and moving review.

Comment on attachment 660258 [details] [diff] [review] Updated Patch >+ // spin through the tree looking for a title for this context menu >+ let node = popupNode; >+ while(node && !title) { while (

Comment on attachment 660258 [details] [diff] [review] Updated Patch Review of attachment 660258 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +1393,5 @@ > + // use the highlighted element if possible, otherwise look for nearby clickable elements > + // If we still don't find one we fall back to using anything > + this._element = BrowserEventHandler._highlightElement || ElementTouchHelper.elementFromPoint(BrowserApp.selectedBrowser.contentWindow, aX, aY); > + if (!this._element) > + this._element = ElementTouchHelper.anyElementFromPoint(BrowserApp.selectedBrowser.contentWindow, aX, aY) The elementFromPoint and anyElementFromPoint functions don't take a window parameter any more. @@ +1437,5 @@ > > Haptic.performSimpleAction(Haptic.LongPress); > > + let popupNode = this._element; > + this._element = null; What if this code never gets executed? That could happen if e.g. menuitemsSet ends up being false. Then we have a dangling/leaked _element reference which could cause problems later. @@ +1454,5 @@ > + title = node.currentURI.spec; > + } else if (node instanceof Ci.nsIDOMHTMLMediaElement) { > + title = (node.currentSrc || node.src); > + } > + console.log("Got title " + title + " from " + node.nodeName); Take out console.log @@ +1489,5 @@ > }, > > handleEvent: function(aEvent) { > aEvent.target.ownerDocument.defaultView.removeEventListener("contextmenu", this, false); > + BrowserEventHandler._cancelTapHighlight(); Ditto here. If we never show a context menu (because e.g. menuitemsSet is false) then we'll never cancel the tap highlight.

Doh. I shoulda checked that a bit more carefully when unbitrotting. Sorry. This is cleaned up and only stores a WeakRef so that if something crazy happens (the page kills itself during the context menu event or something?) we don't leak the document.

Comment on attachment 660503 [details] [diff] [review] Patch v2 This still doesn't handle the last comment on my previous review, where the item gets stuck with the tap highlighting. I made a test page to demonstrate: http://people.mozilla.org/~kgupta/tmp/onclick.html Long tap on the link. It's a div with onclick and :active style, so the "highlight" triggers the active style but there's no context menu. When you remove your finger the item remains in "highlight" because the code to disable the tap highlight never runs.

Ahh. Sorry I missed that. This clears the highlight if we're not showing a context menu as well.

Comment on attachment 661825 [details] [diff] [review] Patch Review of attachment 661825 [details] [diff] [review]: ----------------------------------------------------------------- This one can still fail if the page listens for and stops propagation of the "contextmenu" event. Then the handleEvent function will never get run. But it's a pretty edge case so I'm not sure it's worth dealing with. ::: mobile/android/chrome/content/browser.js @@ +1398,5 @@ > + > + set _target(aTarget) { > + if (aTarget) > + this._targetRef = Cu.getWeakReference(aTarget); > + else this._targetRef = null; nit: Move the body of the else down a line and indent.

Comment on attachment 660503 [details] [diff] [review] Patch v2 Obsoleting old patch

This depends on touch event fluffing, which I had to back out: https://hg.mozilla.org/integration/mozilla-inbound/rev/1adde03021f9

Sorry for the huge delay here. Thought I would be able to figure out the fluffing problems, but haven't had any time to look. Landed this independently because there are other bugs to do with these titles I want to look at: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce34fa2b2189

This broke context menu invocation for many things, see bug 824475.

I cannot reproduce this issue on the latest Nightly and Aurora. Closing bug as verified fixed on: Firefox for Android Version: 21.0a1 (2013-01-29) Device: Galaxy R OS: Android 2.3.4

As per duplicate above, firefox-19 is affected here. Nominate?

We've shipped this for a long time. This code is also pretty fragile. I don't think there's any hurry to uplift it.