708048 - context menus sometimes do not show link location

Status: VERIFIED FIXED

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

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: screenshot

The link location info sometimes does not display.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

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.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Update patch

Unbitrotted.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Updated Patch

Updated and moving review.

Comment 4

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

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 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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 10

[Kartikaya Gupta (email:[e-mail])]

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

Obsoleting old patch

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1adde03021f9

Comment 12

[Wesley Johnston (:wesj)]

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

Comment 13

[Wesley Johnston (:wesj)]

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

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ce34fa2b2189

Comment 15

[Aaron Train [:aaronmt]]

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

Comment 16

[Cristian Nicolae (:xti)]

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

Comment 18

[Aaron Train [:aaronmt]]

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

Nominate?

Comment 19

[Wesley Johnston (:wesj)]

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.