Skip to content

Change <menuitem> from void to like-<option> #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 8, 2016
Merged

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Mar 21, 2016

Allow <menuitem> to have children giving the label

  • Change the content model
  • Change the processing to use trimmed child text content
    (unlike <option> but matches Firefox)
  • Change label IDL attribute to match <option>

Fix #234: Change <menuitem> from void to like-<option>

  • Add menuitem to "Generate implied end tags"
  • <menuitem>, <hr> and <menu> pop a <menuitem>

cc @smaug---- @tkent-google

@zcorpan
Copy link
Member Author

zcorpan commented Mar 21, 2016

Should maybe remove menuitem from "Special"; #909

@zcorpan
Copy link
Member Author

zcorpan commented Mar 22, 2016

Need to add menuitem to the lists in "in body" An end-of-file token, An end tag whose tag name is "body" and An end tag whose tag name is "html", so that <!doctype html><menuitem> is not a parse error.

@domenic
Copy link
Member

domenic commented Mar 22, 2016

LGTM for what that's worth. Will leave it up to you when you feel comfortable merging.

@zcorpan
Copy link
Member Author

zcorpan commented Mar 22, 2016

Thanks! I want to finish writing some tests first, at least. The parser is tricky. If someone else also want to review, that would be appreciated. :-)

@zcorpan zcorpan added the do not merge yet Pull request must not be merged per rationale in comment label Mar 22, 2016
zcorpan added a commit to zcorpan/html5lib-tests that referenced this pull request Mar 23, 2016
@zcorpan
Copy link
Member Author

zcorpan commented Mar 23, 2016

Tests PR at html5lib/html5lib-tests#72

@zcorpan
Copy link
Member Author

zcorpan commented Mar 30, 2016

@hsivonen do you think you can you review this (and the tests)?

@zcorpan
Copy link
Member Author

zcorpan commented Apr 1, 2016

Since this PR makes <menu> close menuitem, the HealthKit page is still broken, it seems. Check http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4028 in Firefox (I moved the </menuitem> tags to right before <menu>).

@zcorpan
Copy link
Member Author

zcorpan commented Apr 8, 2016

@zcorpan zcorpan removed the do not merge yet Pull request must not be merged per rationale in comment label Apr 8, 2016
@domenic
Copy link
Member

domenic commented Apr 8, 2016

Cool. Can you rebase? Then I will look for editorial issues and merge.

zcorpan added 2 commits April 8, 2016 17:54
- Change the content model
- Change the processing to use trimmed child text content
  (unlike <option> but matches Firefox)
- Change label IDL attribute to match <option>
- Add menuitem to "Generate implied end tags"
- <menuitem>, <hr> and <menu> pop a <menuitem>
- <menuitem> is not void and is not "special"
- An open menuitem at </body>, </html> or EOF is not a parse error.
@zcorpan zcorpan force-pushed the menuitem-not-void branch from 99a93cf to 5e49a20 Compare April 8, 2016 15:59
@zcorpan
Copy link
Member Author

zcorpan commented Apr 8, 2016

Yep, rebased. (I'd prefer to have this merged as two commits to separate the parser changes.)

@domenic domenic merged commit 5e49a20 into master Apr 8, 2016
@domenic domenic deleted the menuitem-not-void branch April 8, 2016 16:09
zcorpan added a commit to html5lib/html5lib-tests that referenced this pull request Apr 28, 2016
@sanjoypal
Copy link

sanjoypal commented May 12, 2016

The specification says
"Allow menuitem to have endtag and menuitem element's end tag can be omitted if
the menuitem element is immediately followed by a menuitem, hr, or menu element,
or if there is no more content in the parent element."

"foo

bar"

What should be parser output for above html as "" is not immediately followed by "

" but a ""?

@Hixie
Copy link
Member

Hixie commented May 12, 2016

The section you quoted is talking about conformance rules for writing HTML, and has nothing to do with how HTML is parsed. How HTML is parsed is discussed in the subsequent section.

@sanjoypal
Copy link

Can the menuitem element have menuitem/hr/menu as children? and this is what is done in https://developer.apple.com/library/ios/documentation/HealthKit/Reference/HealthKit_Constants/index.html#//apple_ref/c/econst/HKBodyTemperatureSensorLocationRectum, is the html in this page conforms this spec?

@zcorpan
Copy link
Member Author

zcorpan commented May 13, 2016

No, it cannot. That page is still broken with the spec change, see #907 (comment) above.

<menuitem><span>foo</span><menu>bar</menu></menuitem>

->

  • menuitem
    • span
      • "foo"
  • menu
    • "bar"

@zcorpan
Copy link
Member Author

zcorpan commented May 13, 2016

@hober can you help trying to get the Apple docs pages fixed (e.g. change from <menuitem> to <menu-item>)?

iabudiab added a commit to iabudiab/HTMLKit that referenced this pull request May 18, 2016
The current spec (2016.05.15) is missing the instruction to "reconstruct
the active formatting elements" to match the handling of the <option>
element

See relevant discussions:
whatwg/html#907
whatwg/html#234
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/MkEDloT-yu8
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25325
@sanjoypal
Copy link

We are not able to ship "contextmenu" in blink because of apple site issue (https://developer.apple.com/library/ios/documentation/HealthKit/Reference/HealthKit_Constants/index.html#//apple_ref/c/econst/HKBodyTemperatureSensorLocationRectum). I filed a bug in apple bug tracker but no action is taken. Firefox does not comply with the spec in this case. So, Firefox does not break the apple site. Can the specification be modified to FF implementation?

@hober
Copy link
Contributor

hober commented Jul 15, 2016

I can't reproduce this problem in the updated docs: https://developer.apple.com/reference/healthkit/hkbodytemperaturesensorlocation

@zcorpan
Copy link
Member Author

zcorpan commented Jul 16, 2016

Thank you @hober!

The old URL is still broken though. Do you think a redirect could be set up?

inikulin added a commit to inikulin/parse5 that referenced this pull request Aug 2, 2016
<!-- fake </menuitem> (maybe) -->
<p>If the <span>current node</span> is a <code>menuitem</code> element, pop that node from the
<span>stack of open elements</span>.</p>
<!-- end of fake </menuitem> -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it reconstruct active formatting element list like <option> do? This is how currently all implementations behaves, even tests landed by html5lib/html5lib-tests@88b8ee9 commit expect this.
\cc @zcorpan

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you file an issue on that? @zcorpan is on vacation until August 9 so it's probably best to track that somewhere.

Copy link
Member

@inikulin inikulin Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Update: #1627

zcorpan added a commit that referenced this pull request Aug 9, 2016
domenic pushed a commit that referenced this pull request Aug 10, 2016
cscott added a commit to fgnass/domino that referenced this pull request Oct 18, 2016
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
sundouzis pushed a commit to sundouzis/parse5 that referenced this pull request Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants