Closed Bug 676236 Opened 13 years ago Closed 2 years ago

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

Categories

(Core :: DOM: HTML Parser, enhancement)

Product:
Core
Component:
DOM: HTML Parser
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: janv, Assigned: hsivonen)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files)

patch v1
32.77 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
Patch with ifdef'ed code
27.14 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
menuitem
10.70 KB, patch
Details | Diff | Splinter Review 
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>
Attached patch patch v1 — — Splinter 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 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.

    
    

    
  
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+

    
    

    
  
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? 

[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)

    
    

    
  
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
          
          
        menuitem
        — 
          — Splinter 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

    
    

    
  
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)

    
    

    
  
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 https://github.com/whatwg/html/issues/2730 this should probably be wontfixed.
Depends on: 1376917

    
    

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

If there are ifdefs, I suppose they can be removed.
Type: defect → enhancement

    
  
Severity: normal → S3

    
    

    
  
Feature was removed in bug 1372276


Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: