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)
Tracking
()
RESOLVED
INVALID
People
(Reporter: sanjoy.pal, Unassigned, Mentored)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [lang=c++])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Comment 2•10 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•10 years ago
|
Mentor: bugs
Updated•10 years ago
|
Whiteboard: [lang=c++]
Comment 4•10 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)
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•10 years ago
|
Assignee: nobody → piyushwaradpande
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Updated•10 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 6•10 years ago
|
||
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".
Comment 9•9 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+
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•9 years ago
|
||
The spec has changed back to type=context. See https://github.com/whatwg/html/issues/235
Comment 13•9 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•9 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•9 years ago
|
||
This bug chould be closed, since the spec was updated to use type="context" and Firefox matches the spec.
Comment 17•8 years 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)
Comment 18•8 years ago
|
||
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.
Description
•