828509 - HTML5 Context menus don't work on links, in Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Yann Brelière]

It doesn’t work for https://www.gandi.net on mobile (it works in desktop Firefox : right-click on the top-left logo or tagline).

Maybe it’s because it contains a link, so the default context-menu is different?
Maybe it’s because the html is added after loading the page, in javascript?


:wesj told me it's because of the link:
> The problem is that we terminate looking for context menu items when we hit an anchor tag. But we probably shouldn’t do that! Thanks for testing!

But I can't open the menu by long-pressing between the 2 logos, it goes in text-selection mode instead of opening the context-menu (but that's likely another bug).

Comment 1

[Alexander Ploner]

Attachment: simple testcase

This simple testcase confirms that the menus are displayed in Firefox Nightly for Android (2013-01-09) if the <a> element has it's own contextmenu attribute, but not if the <a> element is inside a <div> that has a contextmenu attribute.

In Firefox 18 on Desktop it works in both cases.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I have a feeling this is an optimization I copied from XUL Fennec. I can't think why we need/want it, but we do have other options if you disagree (we could do two loops, one for html stuff and one for the api stuff, or we could try some querySelector["*[contextmenu] > *"] before we bail).

Comment 3

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

Comment on attachment 700078 [details] [diff] [review]
Patch v1

I'd like to see some manual testing on web content to make sure we don't break the built-in context menu. Make sure to try out a variety of edge cases.

We have robocop tests for the context menu, but I don't know how extensive they are.

Comment 4

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61eddbd33e8c

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 700078 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 827608
User impact if declined: Context menus don't work the same on Fennec as they don on desktop.
Testing completed (on m-c, etc.): Landed on central today
Risk to taking this patch (and alternatives if risky): Moderate risk. Will slightly change our behaviour for deciding which context menu items to show. I CAN'T think of and haven't found any situations where we will show something we shouldn't though.
String or UUID changes made by this patch: None.

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61eddbd33e8c

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61eddbd33e8c

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61eddbd33e8c

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 700078 [details] [diff] [review]
Patch v1

bug 827608 is going to be resolved for the first time in FF21, so no need to uplift this fix.

Comment 10

[Yann Brelière]

I've just tested with the latest Nightly on my Nexus 7 and it now forks fine. Thanks. :)