1031475 - Add 'nodefault' attribute to specify that default contextmenu items shouldnt be shown

Status: RESOLVED INVALID

(Core :: DOM: Events, defect)

Description

[Dale Harvey (:daleharvey)]

I split this from https://bugzilla.mozilla.org/show_bug.cgi?id=1029343 to handle the gecko side, the other bug can be used for the gaia implementation

Comment 1

[Dale Harvey (:daleharvey)]

Jonas, is there anyone on fx desktop that may want to chime in on this? Since the <menu> referred to can be a nested element, wasnt sure if this should be on the menu or the target |<a href="test" contextmenu="menu1" nodefault>...| but on the menu does seem cleaner. Otherwise this shouldnt take a bunch to implement, are we happy with 

<a contextmenu="menu1">test</a>

<menu id="menu1" nodefault>
 <menuitem> ....

|nodefault| is only checked on the topmost menu

Comment 2

[Alex Vincent [:WeirdAl]]

Nit:  The naming of this attribute is ever so slightly confusing.  When I first saw the bug report, I thought it was "node fault", not "no default".

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: 1031475.patch

I wasnt sure whether to add the flag to the global ctx event or attach it individually when we hit it in the menu elements

I dont think we should / need to change the behaviour of system events, I think its more flexible to just have gaia check for the flag and ignore

Comment 4

[Dale Harvey (:daleharvey)]

also +1 on being confused and reading it as 'node fault', would be nice to bikeshed a better name, I couldnt think of one though

Comment 5

[Alex Vincent [:WeirdAl]]

(In reply to Dale Harvey (:daleharvey) from comment #4)
> also +1 on being confused and reading it as 'node fault', would be nice to
> bikeshed a better name, I couldnt think of one though

no-default, noDefault, no_default...  But these names are so generic, it's not helpful.

context-chrome="disabled" actually describes what you're trying to do.

Comment 6

[(no longer active)]

Comment on attachment 8447542 [details] [diff] [review]
1031475.patch

Review of attachment 8447542 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test for nodefault on a nested <menu> as well.  Otherwise looks good.

Comment 7

[(no longer active)]

(In reply to Dale Harvey (:daleharvey) from comment #1)
> Jonas, is there anyone on fx desktop that may want to chime in on this?

Gavin probably knows.

Comment 8

[(no longer active)]

Also, please send an intent to implement email about this.  Thanks!

Comment 9

[Dale Harvey (:daleharvey)]

|chrome="disabled"| on the menu element seems more descriptive and flexible to me (I dont think we need context-chrome, we already do type="context" on the menu element

Ehsan could you explain how to send an intent to implement email, do you mean on the b2g list or elsewhere?

Comment 10

[(no longer active)]

Please see <https://wiki.mozilla.org/WebAPI/ExposureGuidelines> for our policy for exposing new APIs.  The intent to implement emails are also explained there.

Comment 11

[Dale Harvey (:daleharvey)]

I emailed the intent to dev-platform, hixie mentioned some reservations and to be honest I kinda agree, jumped on implementing this but not entirely sure we should, applications shouldnt be breaking the context menu functionality (links should still be links etc), it seems like having the user be given both menus would be the best option every time, and if our default ones are getting in the way then we should rethink how many of them there needs to be, and possible options giving the web content ones preference

Jonas, whats the use case?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Does the attached patch actually affect Desktop? Didn't think BrowserElementChildPreload.js was actually used there in any significant way.

In any case I agree that we shouldn't let webpage authors disable context menus with a simple attribute, it's far too prone to abuse.

Comment 13

[(no longer active)]

(In reply to :Gavin Sharp [away until July 14th] from comment #12)
> Does the attached patch actually affect Desktop?

No.

Comment 14

[(no longer active)]

(Err, accidentally deleted part of my comment!)

(In reply to :Gavin Sharp [away until July 14th] from comment #12)
> In any case I agree that we shouldn't let webpage authors disable context
> menus with a simple attribute, it's far too prone to abuse.

That ship has sailed.  :-)  Try <html oncontextmenu="return false">

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Indeed. As long as we support cancelling the contextmenu event we are only forcing authors to do even worse things.

Essentially we are currently telling them "you can create your own context menu UI, but in order to do so you need to create less accessible pages". Currently authors seem quite happy to sacrifice accessibility in order to get control over the context menus that they render.

Pages currently cancel the contextmenu event and then render their own context menu using HTML. Or they change their links into <span onclick=... style=...>. Or they overlay transparent <div>s.

This situation is even worse because if the user installs an addon which renders a context menu even if the contextmenu event has been cancelled, then that will break many applications since the built-in context menu will render over the page-provided one implemented in HTML which means that those actions can't be accessed.

It's much better that we get people onto a path of writing pages that the browser can semantically understand. If we understand that the page is asking to render it's own context menu rather than simply add to the existing one, then we can do things like enable addons to show the built-in context menu items, or even have a pref for this (we used to have one, but I can't find it any more).

Comment 16

[Dale Harvey (:daleharvey)]

For this specific implementation decided against it for now, we can revisit after a bit more discussion about it + alternative implementations