Closed Bug 708048 Opened 8 years ago Closed 7 years ago

context menus sometimes do not show link location

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox19 --- affected
firefox20 --- verified
firefox21 --- verified

People

(Reporter: kbrosnan, Assigned: wesj)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached image screenshot
The link location info sometimes does not display.
Priority: -- → P4
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #612389 - Flags: review?(mark.finkle)
Attached patch Update patch (obsolete) — Splinter Review
Unbitrotted.
Attachment #612389 - Attachment is obsolete: true
Attachment #612389 - Flags: review?(mark.finkle)
Attachment #643940 - Flags: review?(mark.finkle)
Attached patch Updated Patch (obsolete) — Splinter Review
Updated and moving review.
Assignee: nobody → wjohnston
Attachment #643940 - Attachment is obsolete: true
Attachment #643940 - Flags: review?(mark.finkle)
Attachment #660258 - Flags: review?(bugmail.mozilla)
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.
Attachment #660258 - Flags: review?(bugmail.mozilla) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #660258 - Attachment is obsolete: true
Attachment #660503 - Flags: review?(bugmail.mozilla)
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.
Attachment #660503 - Flags: review?(bugmail.mozilla) → review-
Attached patch PatchSplinter Review
Ahh. Sorry I missed that. This clears the highlight if we're not showing a context menu as well.
Attachment #661825 - Flags: review?(bugmail.mozilla)
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.
Attachment #661825 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 660503 [details] [diff] [review]
Patch v2

Obsoleting old patch
Attachment #660503 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/ce34fa2b2189
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 824475
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 837698
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.
You need to log in before you can comment on or make changes to this bug.