Closed Bug 1100749 Opened 10 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

36 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: sanjoy.pal, Unassigned, Mentored)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

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"
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
Mentor: bugs
Whiteboard: [lang=c++]
I'd like to work on this bug.
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)
The MDN is in accordance with the specs but not the Firefox possibilities : https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu
Assignee: nobody → piyushwaradpande
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Piyush, any update? are you working on this?
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
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 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+
Attached patch Patch v2 (deleted) — Splinter Review
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.
The spec has changed back to type=context. See https://github.com/whatwg/html/issues/235
(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 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
This bug chould be closed, since the spec was updated to use type="context" and Firefox matches the spec.
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
Closed: 8 years ago
Flags: needinfo?(bugs)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: