Add support for the <menuitem> element to the HTML parser

NEW
Assigned to

Status

()

8 years ago
2 years ago

People

(Reporter: janv, Assigned: hsivonen)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
We need this for bug 617528. We decided to replace <command> with <menuitem>

The new syntax is:
<menu type="context">
  <menuitem type="" label="" ...>
  ...
</menu>

The behavior of <menuitem> is similar to <command>. However there is no need to use <menuitem> in <head>

There are plans to upgrade the syntax in future to:
<head>
  <command id="foo" type="" label="" ...>
</head>
<body>
  <menu type="context">
    <menuitem command="foo">
  </menu>
</body>
(Reporter)

Comment 1

8 years ago
Posted patch patch v1Splinter Review
Attachment #550371 - Flags: review?(hsivonen)
(Reporter)

Updated

8 years ago
Blocks: 617528
(Assignee)

Comment 2

8 years ago
Comment on attachment 550371 [details] [diff] [review]
patch v1

(In reply to comment #0)
> We need this for bug 617528. We decided to replace <command> with <menuitem>

Decided? Why? Who is "we"? Why hasn't this been raised at the WHATWG or the W3C HTML WG?

The days of browser vendors adding HTML parsing behaviors without coordinating through the spec are supposed to be over. It's painful (see www.w3.org/Bugs/Public/show_bug.cgi?id=13113) but coordinating through the spec is the right thing to do.
Attachment #550371 - Flags: review?(hsivonen) → review-
Before the menuitem gets spec'ed, we should #ifdef/pref the code, IMO.
(Assignee)

Comment 5

8 years ago
Comment on attachment 550371 [details] [diff] [review]
patch v1

r=me on the condition that the lines
case MENUITEM:
are #ifdefed off by default when landing on trunk ahead of the spec making <menuitem> a void element.

(I'll figure out what to do about #ifdefs and the translator when I return from vacation.)
Attachment #550371 - Flags: review- → review+
(Reporter)

Comment 6

8 years ago
I'm updating and testing a patch with the ifdefs
(Reporter)

Comment 7

8 years ago
Assignee: nobody → Jan.Varga
(Assignee)

Comment 8

8 years ago
Comment on attachment 550678 [details] [diff] [review]
Patch with ifdef'ed code

r=me assuming that the define continues to be off by default until the spec changes to have menuitem as a void element.
Attachment #550678 - Flags: review+
(Assignee)

Comment 10

8 years ago
smaug told me that <menuitem> is no longer meant to be a void element. Can we back this out now?
Keywords: dev-doc-needed
Is this still in progress? I was under the impression we'd implemented this already, based on bug 677463. Can I get a clarification as to the status here before I document it? Thanks!
OK, when time comes to document this, I need to reference bug 677463 to be sure that behavior is covered.

Comment 15

6 years ago
According to [1] and the spec[2], menuitem is a void element. So the pref can be flipped now, I think. 

Anything else left to do in this bug? 

[1] http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-December/038472.html
[2] http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#the-menuitem-element
Flags: needinfo?(Jan.Varga)
(Reporter)

Comment 16

5 years ago
Well, yes, but it's not just the pref. All the tests need to be updated to use just <menuitem label="...">
Flags: needinfo?(Jan.Varga)
(Reporter)

Comment 17

5 years ago
Posted patch menuitemSplinter Review
initial patch (more work needs to be done)
(Reporter)

Comment 18

5 years ago
Hm, trying to clear the needinfo flag, sorry for the spam.
Flags: needinfo?(Jan.Varga)

Updated

5 years ago
Blocks: 897102

Updated

5 years ago
Duplicate of this bug: 891005

Comment 20

3 years ago
It would be good to get input on this spec issue: https://github.com/whatwg/html/issues/234#issuecomment-146335204
Based on the link in comment #20, the spec ended up changing such that <menuitem> is not void, but is <option>-like (and no longer in the special category).

smaug, I can't quite tell if that means Firefox now handles <menuitem> correctly, or if there are still differences which need to be addressed. Can you see anything obvious?

At any rate I'm guessing we might as well close this bug as INVALID, and file a follow-up to remove the few lines of code which add the build option for making <menuitem> a void element.
Flags: needinfo?(bugs)
https://github.com/whatwg/html/issues/234 is still open, so I don't really understand what has happened in the spec.
I'd be ok to make us work closer to <option> if that is what the spec and other browsers will have too.

If you want to do some parser changes, ask hsivonen to review ;)
Flags: needinfo?(bugs)
Henri, could you take a look at whether something needs to be done here.
The spec bug was closed but it is still a bit unclear to me what actually got changed.
(some times I hate github making it so hard to follow changes)
Flags: needinfo?(hsivonen)
(Assignee)

Comment 25

3 years ago
I'll take a look.
Do you think you'll have a chance to take a look at this anytime soon, hsivonen?
(Assignee)

Comment 28

2 years ago
Looking now. See the previous comment for a try push.
Flags: needinfo?(hsivonen)
(Assignee)

Updated

2 years ago
Assignee: jvarga → hsivonen
(Assignee)

Comment 32

2 years ago
Which other browsers have implemented this spec change?
Flags: needinfo?(zcorpan)

Comment 36

2 years ago
https://github.com/whatwg/html/pull/2319 made menuitem parse like an unknown element.

If there are ifdefs, I suppose they can be removed.
You need to log in before you can comment on or make changes to this bug.