Open Bug 676236 Opened 10 years ago Updated 2 years ago

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


(Core :: DOM: HTML Parser, enhancement)

Not set




(Reporter: janv, Assigned: hsivonen)


(Blocks 1 open bug)


(Keywords: dev-doc-needed)


(3 files)

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

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

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:
  <command id="foo" type="" label="" ...>
  <menu type="context">
    <menuitem command="foo">
Attached patch patch v1Splinter Review
Attachment #550371 - Flags: review?(hsivonen)
Blocks: 617528
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 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.
Comment on attachment 550371 [details] [diff] [review]
patch v1

r=me on the condition that the lines
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+
I'm updating and testing a patch with the ifdefs
Assignee: nobody → Jan.Varga
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+
smaug told me that <menuitem> is no longer meant to be a void element. Can we back this out now?
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.
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? 

Flags: needinfo?(Jan.Varga)
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)
Attached patch menuitemSplinter Review
initial patch (more work needs to be done)
Hm, trying to clear the needinfo flag, sorry for the spam.
Flags: needinfo?(Jan.Varga)
Blocks: 897102
Duplicate of this bug: 891005
It would be good to get input on this spec issue:
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) 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)
I'll take a look.
Do you think you'll have a chance to take a look at this anytime soon, hsivonen?
Looking now. See the previous comment for a try push.
Flags: needinfo?(hsivonen)
Assignee: jvarga → hsivonen
Which other browsers have implemented this spec change?
Flags: needinfo?(zcorpan)
Per this should probably be wontfixed.
Depends on: 1376917 made menuitem parse like an unknown element.

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