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

RESOLVED INVALID

Status

()

Core
DOM: Events
RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
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
Assignee: nobody → dale
Blocks: 1029343
Flags: needinfo?(jonas)
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".
(Assignee)

Comment 3

4 years ago
Created attachment 8447542 [details] [diff] [review]
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
Attachment #8447542 - Flags: feedback?(ehsan)
(Assignee)

Comment 4

4 years ago
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
(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

4 years ago
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.
Attachment #8447542 - Flags: feedback?(ehsan) → feedback+

Comment 7

4 years ago
(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.
Flags: needinfo?(gavin.sharp)

Comment 8

4 years ago
Also, please send an intent to implement email about this.  Thanks!
(Assignee)

Comment 9

4 years ago
|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?
Flags: needinfo?(ehsan)

Comment 10

4 years ago
Please see <https://wiki.mozilla.org/WebAPI/ExposureGuidelines> for our policy for exposing new APIs.  The intent to implement emails are also explained there.
Flags: needinfo?(ehsan)
(Assignee)

Comment 11

4 years ago
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?
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.
Flags: needinfo?(gavin.sharp)

Comment 13

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

No.

Comment 14

4 years ago
(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">
Whiteboard: [systemsfe]
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).
Flags: needinfo?(jonas)
(Assignee)

Comment 16

4 years ago
For this specific implementation decided against it for now, we can revisit after a bit more discussion about it + alternative implementations
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.