If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

<menu> element type=context should be renamed to type=popup

RESOLVED INVALID

Status

()

Core
DOM: Core & HTML
RESOLVED INVALID
3 years ago
a year ago

People

(Reporter: sanjoy.pal, Unassigned, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

36 Branch
x86_64
Linux
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36

Steps to reproduce:

Specification: https://html.spec.whatwg.org/multipage/forms.html#attr-menu-type

As per spec. the correct type value for menu element in contextmenu is "popup", but firefox supports type="context".


Actual results:

http://jsbin.com/guzapu/2/edit?html,css,output does not show custom menu on firefox


Expected results:

Should render custom menu items in context menu. type=context should be renamed to "popup"

Comment 1

3 years ago
FYI, https://bugzilla.mozilla.org/show_bug.cgi?id=897102

Comment 2

3 years ago
I doubt we can just remove support for type="context", at least not immediately, but we could support also "popup" and warn about use of "context".

Note, the spec used to have type="context", but it was then changed to have type="popup"
Blocks: 897102

Updated

3 years ago
Mentor: bugs@pettay.fi

Updated

3 years ago
Whiteboard: [lang=c++]

Comment 3

3 years ago
I'd like to work on this bug.

Comment 4

3 years ago
Feel free to :)

You'll need to update http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLMenuElement.cpp#29
and then perhaps add a helper method HasPopupState() to HTMLMenuElement which checks if the
type is either context or popup and use that everywhere and not the current 
mType == MENU_TYPE_CONTEXT

HTMLMenuElement::ParseAttribute could perhaps warn about use of deprecated feature if "context" is used.
See http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDeprecatedOperationList.h
and http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2076 (WarnOnceAbout)

Comment 5

3 years ago
The MDN is in accordance with the specs but not the Firefox possibilities :
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu

Updated

3 years ago
Assignee: nobody → piyushwaradpande
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: dev-doc-needed
(Reporter)

Comment 6

2 years ago
Piyush, any update? are you working on this?

Comment 7

2 years ago
I am re-assigning it back to nobody@mozilla.org so that it doesn't remain blocked. If I get a patch ready, I'll put in a comment and take it back.
Assignee: piyushwaradpande → nobody
Status: ASSIGNED → NEW
Created attachment 8666155 [details] [diff] [review]
Add support for type="popup" in addition to type="context".

I think this works right. I haven't added the deprecation warning yet, but this at least gets the menu items to appear with type="popup" in addition to "context".
Assignee: nobody → wkocher
Status: NEW → ASSIGNED
Attachment #8666155 - Flags: feedback?(bugs)

Comment 9

2 years ago
Comment on attachment 8666155 [details] [diff] [review]
Add support for type="popup" in addition to type="context".

You need to also change kMenuDefaultType to point index 3 so that we don't change the default type.
And we need some test here. Bug 617528 added some tests which could be modified, at least the stuff in subtst_contextmenu.html.
	
(Looks like we don't handle the missing attribute value correctly per spec, since 'missing value default' depends on the parent element. The spec is weird here, but I don't care enough to fight against it. But no need to change the behavior in this bug.)

janv, who implemented this stuff, could review the updated patch.
Attachment #8666155 - Flags: feedback?(bugs) → feedback+
Created attachment 8667656 [details] [diff] [review]
Patch v2

Fixed the kMenuDefaultType issue, and copied subtst_contextmenu.html to subtst_popupmenu.html and switched the type tested to "popup".

Appears to pass locally, here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b2a4769db2a
Attachment #8666155 - Attachment is obsolete: true
Attachment #8667656 - Flags: review?(Jan.Varga)
Ooh, still haven't done any deprecation warning thing.

Comment 12

2 years ago
The spec has changed back to type=context. See https://github.com/whatwg/html/issues/235

Comment 13

2 years ago
(In reply to Simon Pieters from comment #12)
> The spec has changed back to type=context. See
> https://github.com/whatwg/html/issues/235

Hm, so the patch here is not needed anymore, right ?

Comment 14

2 years ago
Comment on attachment 8667656 [details] [diff] [review]
Patch v2

Please, request a new review if you want to land the new test.
Attachment #8667656 - Flags: review?(Jan.Varga)
Unassigning myself since it seems like this isn't needed anymore.
Assignee: wkocher → nobody
Status: ASSIGNED → NEW

Comment 16

2 years ago
This bug chould be closed, since the spec was updated to use type="context" and Firefox matches the spec.

Comment 17

a year ago
Is there a reason why this bug is still open, :smaug?

The spec has indeed changed to use type="context": https://html.spec.whatwg.org/multipage/forms.html#menus
Flags: needinfo?(bugs)
no reason. Thanks for reminding. And feel free to close this kind of bugs. Someone will the reopen if one accidentally closes valid bug :)
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(bugs)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.