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

Status: RESOLVED INVALID

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

Description

[sanjoy.pal]

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

[Eiji Kitamura]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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"

Comment 3

[Piyush W]

I'd like to work on this bug.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

[Zéfling]

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

Comment 6

[sanjoy.pal]

Piyush, any update? are you working on this?

Comment 7

[Piyush W]

I am re-assigning it back to [e-mail] so that it doesn't remain blocked. If I get a patch ready, I'll put in a comment and take it back.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: 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".

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: 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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Ooh, still haven't done any deprecation warning thing.

Comment 12

[Simon Pieters [:zcorpan]]

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

Comment 13

[Jan Varga [:janv]]

(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

[Jan Varga [:janv]]

Comment on attachment 8667656 [details] [diff] [review]
Patch v2

Please, request a new review if you want to land the new test.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Unassigning myself since it seems like this isn't needed anymore.

Comment 16

[Darryl Pogue]

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

Comment 17

[Thomas Wisniewski]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

no reason. Thanks for reminding. And feel free to close this kind of bugs. Someone will the reopen if one accidentally closes valid bug :)