Closed
Bug 562008
Opened 14 years ago
Closed 14 years ago
HTML5 section DOM elements should be of type HTMLElement
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla2.0b1
People
(Reporter: bugzilla, Assigned: mounir)
References
()
Details
(Keywords: dev-doc-complete, html5)
Attachments
(1 file, 8 obsolete files)
42.05 KB,
patch
|
Details | Diff | Splinter Review |
Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100427 Minefield/3.7a5pre See bug 559284, where these elements default style is now defined as display:block This bug requests the section elements (article, aside, footer, header, hgroup, nav, section) to return these DOM elements type equals to HTMLElement and not as HTMLUnknownElement as they are today.
Reporter | ||
Updated•14 years ago
|
Summary: Support for defining HTML5 section elements as type = HTMLElement → HTML5 section DOM elements should be of type HTMLElement
Comment 1•14 years ago
|
||
Links to the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/sections.html#the-section-element "DOM interface: Uses HTMLElement." http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#htmlunknownelement "The HTMLElement interface holds methods and attributes related to a number of disparate features, and the members of this interface are therefore described in various different sections of this specification. The HTMLUnknownElement interface must be used for HTML elements that are not defined by this specification (or other applicable specifications)." To clarify the description what's expected is e.g. HTML <section>'s .constructor is HTMLElement, e.g.: document.createElementNS("http://www.w3.org/1999/xhtml", "section").constructor == HTMLElement
Keywords: html5
Comment 2•14 years ago
|
||
Relevant parts from bug 559284: I believe this needs parser changes, actually... nsElementTable and all that mess. Unless I'm missing something? Henri, what's needed to make this work with the HTML5 parser? Blake, can the nsElementTable entries for these match <div>? And even then, I'm not sure we have a sane way for things to report as HTMLElement as opposed to something whose proto is HTMLElement....
(In reply to comment #2) > Henri, what's needed to make this work with the HTML5 parser? The HTML5 parser uses NS_NewElement(), so the interfaces come from http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/parser/htmlparser/public/nsHTMLTagList.h > Blake, can the nsElementTable entries for these match <div>? Commenting here even though the question was for Blake: Since the old parser will still be used for feed sanitizing after the HTML5 parser has been turned on until bug 482909 is fixed, I think the old parser should not be allowed to regress just quite yet. <section>, etc., are not supposed to parse the same way as <div>. <div> is scoping. <section>, etc., are not.
Comment 4•14 years ago
|
||
OK. Is there a simple way to express the way they _should_ parse with the old parser? Ideally via copy/paste of an existing nsElementTable chunk?
(In reply to comment #4) > OK. Is there a simple way to express the way they _should_ parse with the old > parser? Ideally via copy/paste of an existing nsElementTable chunk? I am not aware of a completely correct nsElementTable chunk, but I believe bug 311366 got close enough so that fixing the old parser further isn't probably worthwhile.
Comment 6•14 years ago
|
||
Well, the issue is that we'd need to add entries for the nsHTMLTagList bits we add. The question is what those entries should say...
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Component: DOM → DOM: Core & HTML
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mounir.lamouri
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #446932 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #446933 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•14 years ago
|
||
Comment on attachment 446933 [details] [diff] [review] Patch v1 Blake, could you review the old parser changes ?
Attachment #446933 -
Flags: review?(mrbkap)
Comment 10•14 years ago
|
||
Comment on attachment 446933 [details] [diff] [review] Patch v1 The construction |kInlineEntity | kFlowEntity| is redundant, just use |kFlowEntity|. Also, nix the useless additional empty line you added after script. r=mrbkap with that.
Attachment #446933 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 11•14 years ago
|
||
r=mrbkap I've added the elements to the editor and nsGkAtomLists.h (forget them in the previous patch).
Attachment #446933 -
Attachment is obsolete: true
Attachment #447237 -
Flags: review?(bzbarsky)
Attachment #446933 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #447237 -
Flags: review?(timeless)
Assignee | ||
Comment 12•14 years ago
|
||
Josh, could you review the editor changes ?
Attachment #447237 -
Flags: review?(timeless)
Attachment #447237 -
Flags: review?(brade)
Attachment #447237 -
Flags: review-
Comment 13•14 years ago
|
||
Comment on attachment 447237 [details] [diff] [review] Patch v2 the additional class and the repetition in the name kinda bother me... +#define NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_HTMLELEMENT(_elementName)\ +NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo, \ + return NS_NewHTMLElement(aNodeInfo, aFromParser); \ NS_DECLARE_NS_NEW_HTML_ELEMENT(Area) +NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_HTMLELEMENT(Article) NS_DECLARE_NS_NEW_HTML_ELEMENT(Script) +NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_HTMLELEMENT(Section) +++ b/editor/libeditor/base/nsEditPropertyAtomList.h @@ -92,33 +92,40 @@ EDITOR_ATOM(span, "span") // block tags +EDITOR_ATOM(article, "article") +EDITOR_ATOM(aside, "aside") EDITOR_ATOM(p, "p") i need to figure out why p was listed early, it might be a priority order thing, in which case article+aside should probably be later (after pre?) EDITOR_ATOM(div, "div") EDITOR_ATOM(center, "center") EDITOR_ATOM(blockquote, "blockquote") things are somewhat semantically grouped, which argues that footer should be w/ header: +EDITOR_ATOM(footer, "footer") EDITOR_ATOM(h1, "h1") .. EDITOR_ATOM(h6, "h6") +EDITOR_ATOM(header, "header") +EDITOR_ATOM(hgroup, "hgroup") EDITOR_ATOM(ul, "ul") EDITOR_ATOM(ol, "ol") EDITOR_ATOM(dl, "dl") EDITOR_ATOM(pre, "pre") i suspect that nav should come later, perhaps near section... +EDITOR_ATOM(nav, "nav") EDITOR_ATOM(noscript, "noscript") EDITOR_ATOM(form, "form") EDITOR_ATOM(hr, "hr") +EDITOR_ATOM(section, "section") EDITOR_ATOM(table, "table") EDITOR_ATOM(fieldset, "fieldset") EDITOR_ATOM(address, "address") diff --git a/editor/libeditor/html/nsHTMLEditUtils.cpp b/editor/libeditor/html/nsHTMLEditUtils.cpp this list seems to be sorted alphabetically, please put section between script and select: ELEM(samp, PR_TRUE, PR_TRUE, GROUP_PHRASE, GROUP_INLINE_ELEMENT), + ELEM(section, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), ELEM(script, PR_TRUE, PR_FALSE, GROUP_HEAD_CONTENT | GROUP_SPECIAL, GROUP_LEAF), ELEM(select, PR_TRUE, PR_FALSE, GROUP_FORMCONTROL, GROUP_SELECT_CONTENT), diff --git a/parser/htmlparser/public/nsHTMLTagList.h b/parser/htmlparser/public/nsHTMLTagList.h this list seems to be sorted alphabetically, please put section between script and select: HTML_TAG(samp, Span) +HTML_TAG(section, Section) // HTMLElement HTML_TAG(script, Script) HTML_TAG(select, Select) this list seems to be sorted alphabetically, please put section between script and select: { + /*tag*/ eHTMLTag_section, + /*req-parent excl-parent*/ eHTMLTag_unknown,eHTMLTag_unknown, + /*rootnodes,endrootnodes*/ &gRootTags,&gRootTags, + /*autoclose starttags and endtags*/ 0,0,0,0, + /*parent,incl,exclgroups*/ kBlock, (kSelf|kFlowEntity), kNone, + /*special props, prop-range*/ 0,kDefaultPropRange, + /*special parents,kids*/ 0,0, + }, + { /*tag*/ eHTMLTag_script, /*req-parent excl-parent*/ eHTMLTag_unknown,eHTMLTag_unknown, /*rootnodes,endrootnodes*/ &gRootTags,&gRootTags, /*autoclose starttags and endtags*/ 0,0,0,0, /*parent,incl,exclgroups*/ (kSpecial|kHeadContent), kCDATA, kNone, // note: this is kHeadContent since shipping this breaks things. /*special props, prop-range*/ kNoStyleLeaksIn|kLegalOpen, kNoPropRange, /*special parents,kids*/ 0,&gContainsText, }, this list seems to be sorted alphabetically, please put section between script and select: +static const PRUnichar sHTMLTagUnicodeName_section[] = + {'s', 'e', 'c', 't', 'i', 'o', 'n', '\0'}; static const PRUnichar sHTMLTagUnicodeName_script[] = {'s', 'c', 'r', 'i', 'p', 't', '\0'}; static const PRUnichar sHTMLTagUnicodeName_select[] = {'s', 'e', 'l', 'e', 'c', 't', '\0'}; the - is because i need to figure out what nsEditPropertyAtomList is thinking.
Comment 14•14 years ago
|
||
Comment on attachment 447237 [details] [diff] [review] Patch v2 r- Please fix items timeless noted. Minus is because the tests seem insufficient to me. Please add a test or two to the editor to know that your changes are having the desired effect within the editor.
Attachment #447237 -
Flags: review?(brade) → review-
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #13) > (From update of attachment 447237 [details] [diff] [review]) > the additional class and the repetition in the name kinda bother me... > +#define NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_HTMLELEMENT(_elementName)\ > +NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo, \ > + return NS_NewHTMLElement(aNodeInfo, aFromParser); \ > > NS_DECLARE_NS_NEW_HTML_ELEMENT(Area) > +NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_HTMLELEMENT(Article) > > NS_DECLARE_NS_NEW_HTML_ELEMENT(Script) > +NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_HTMLELEMENT(Section) I didn't find another way to do that except with the new nsHTMLElement class. If you have any idea..? For the name of the macro, i tried to keep the same kind of name but it could be changed. > +++ b/editor/libeditor/base/nsEditPropertyAtomList.h > @@ -92,33 +92,40 @@ EDITOR_ATOM(span, "span") > // block tags > +EDITOR_ATOM(article, "article") > +EDITOR_ATOM(aside, "aside") > EDITOR_ATOM(p, "p") > > i need to figure out why p was listed early, it might be a priority order > thing, in which case article+aside should probably be later (after pre?) > > EDITOR_ATOM(div, "div") > EDITOR_ATOM(center, "center") > EDITOR_ATOM(blockquote, "blockquote") > > things are somewhat semantically grouped, which argues that footer should be w/ > header: > +EDITOR_ATOM(footer, "footer") > EDITOR_ATOM(h1, "h1") > .. > EDITOR_ATOM(h6, "h6") > +EDITOR_ATOM(header, "header") > +EDITOR_ATOM(hgroup, "hgroup") > EDITOR_ATOM(ul, "ul") > EDITOR_ATOM(ol, "ol") > EDITOR_ATOM(dl, "dl") > EDITOR_ATOM(pre, "pre") > > i suspect that nav should come later, perhaps near section... > +EDITOR_ATOM(nav, "nav") > EDITOR_ATOM(noscript, "noscript") > EDITOR_ATOM(form, "form") > EDITOR_ATOM(hr, "hr") > +EDITOR_ATOM(section, "section") > EDITOR_ATOM(table, "table") > EDITOR_ATOM(fieldset, "fieldset") > EDITOR_ATOM(address, "address") Ok. That are interesting clearly-not-explicit-rules examples. (In reply to comment #14) > (From update of attachment 447237 [details] [diff] [review]) > r- Please fix items timeless noted. Minus is because the tests seem > insufficient to me. Please add a test or two to the editor to know that your > changes are having the desired effect within the editor. Actually, I've added the code into the editor to let some editors tests pass. TBH, there is no other "desired effect" and I've no idea what I should test.
Assignee | ||
Comment 16•14 years ago
|
||
I've updated the patch regarding timeless comments. I've added all the new elements at the end of the block list and I've fixed the section misplacement.
Attachment #447237 -
Attachment is obsolete: true
Attachment #447441 -
Flags: review?(bzbarsky)
Attachment #447237 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #447441 -
Flags: review?(timeless)
Assignee | ||
Updated•14 years ago
|
Attachment #447441 -
Flags: review?(brade)
Why do you need the new NS_NewHTML*Element constructors? I don't see a reason why someone would want to create a <footer> element directly rather than go through the DOM.
One way to do it would be to define: NS_NewHTMLBasicElement(nsINodeInfo *aNodeInfo, \ PRBool aFromParser = PR_FALSE) \ { \ return NS_NewHTMLElement(aNodeInfo, aFromParser); \ } And then in nsHTMLTagList.h do HTML_TAG(article, Basic) FWIW, there's probably more elements that should use "basic". Look in nsHTMLTagList.h for 'Span'.
Assignee | ||
Comment 19•14 years ago
|
||
This patch is adding "Basic" to create HTMLElement. I am going to open a follow-up to stop using HTMLSpanElement for elements that should only implement HTMLElement.
Attachment #446932 -
Attachment is obsolete: true
Attachment #447441 -
Attachment is obsolete: true
Attachment #447770 -
Flags: review?(jonas)
Attachment #446932 -
Flags: review?(bzbarsky)
Attachment #447441 -
Flags: review?(timeless)
Attachment #447441 -
Flags: review?(bzbarsky)
Attachment #447441 -
Flags: review?(brade)
Assignee | ||
Updated•14 years ago
|
Attachment #447770 -
Flags: review?(timeless)
Assignee | ||
Updated•14 years ago
|
Attachment #447770 -
Flags: review?(brade)
Comment on attachment 447770 [details] [diff] [review] Patch v4 Looks good, though you might also want to check that document.createElememt(X).toString() starts with "[object HTMLElement"
Attachment #447770 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 21•14 years ago
|
||
This patch is using content/html/content/test/test_bug389797.html for testing (and removing content/html/content/test/test_bug559284.html). This is also removing "Basic" to use "" after discussing with Jonas on IRC.
Attachment #447770 -
Attachment is obsolete: true
Attachment #447777 -
Flags: review?(timeless)
Attachment #447770 -
Flags: review?(timeless)
Attachment #447770 -
Flags: review?(brade)
Assignee | ||
Updated•14 years ago
|
Attachment #447777 -
Flags: superreview?(jst)
Attachment #447777 -
Flags: review?(brade)
Comment 22•14 years ago
|
||
Comment on attachment 447777 [details] [diff] [review] Patch v5 >--- a/editor/libeditor/html/nsHTMLEditUtils.cpp >@@ -565,17 +566,19 @@ struct nsElementInfo > ELEM(applet, PR_TRUE, PR_TRUE, GROUP_SPECIAL | GROUP_BLOCK, > GROUP_FLOW_ELEMENT | GROUP_OBJECT_CONTENT), article should be after area :( >+ ELEM(article, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), > ELEM(area, PR_FALSE, PR_FALSE, GROUP_MAP_CONTENT, GROUP_NONE), >+ ELEM(aside, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), > ELEM(fieldset, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), footer should be after font: >+ ELEM(footer, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), > ELEM(font, PR_TRUE, PR_TRUE, GROUP_SPECIAL, GROUP_INLINE_ELEMENT), > ELEM(samp, PR_TRUE, PR_TRUE, GROUP_PHRASE, GROUP_INLINE_ELEMENT), section should be after script: >+ ELEM(section, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), > ELEM(script, PR_TRUE, PR_FALSE, GROUP_HEAD_CONTENT | GROUP_SPECIAL, > GROUP_LEAF),
Attachment #447777 -
Flags: review?(timeless) → review-
Assignee | ||
Comment 23•14 years ago
|
||
Attachment #447777 -
Attachment is obsolete: true
Attachment #448532 -
Flags: superreview?(jst)
Attachment #448532 -
Flags: review?(timeless)
Attachment #447777 -
Flags: superreview?(jst)
Attachment #447777 -
Flags: review?(brade)
Comment 24•14 years ago
|
||
Comment on attachment 448532 [details] [diff] [review] Patch v5.1 /content/html/content/test/Makefile.in * line 165 -- test_bug559284.html \ you need to remove this. i'd kind of like to see some editor tests, but i don't think it's great for me to hold this back on that.
Attachment #448532 -
Flags: review?(timeless) → review+
Updated•14 years ago
|
Attachment #448532 -
Flags: superreview?(jst)
Attachment #448532 -
Flags: superreview+
Attachment #448532 -
Flags: review?(ehsan)
Comment 25•14 years ago
|
||
Comment on attachment 448532 [details] [diff] [review] Patch v5.1 sr=jst, but Ehsan should have a look at the editor changes here.
Assignee | ||
Comment 26•14 years ago
|
||
(In reply to comment #24) > (From update of attachment 448532 [details] [diff] [review]) > /content/html/content/test/Makefile.in > * line 165 -- test_bug559284.html \ > > you need to remove this. This test file is duplicating tests from test_bug389797.html. > i'd kind of like to see some editor tests, but i don't think it's great for me > to hold this back on that. As I said, I did the changes in the editor to prevent having editor tests failing. I suppose those tests are covering (at least a part of) the changes. If you think some tests are still needed, please, let me know what I could do (maybe in a follow-up bug).
Comment 27•14 years ago
|
||
I'll do my best to look at the patch today, but I can promise a review tomorrow. Sorry about the delay.
Comment 28•14 years ago
|
||
Comment on attachment 448532 [details] [diff] [review] Patch v5.1 >+ ELEM(hgroup, PR_TRUE, PR_TRUE, GROUP_BLOCK, GROUP_FLOW_ELEMENT), According to http://dev.w3.org/html5/markup/hgroup.html, hrgroup cannot contain itself, and can only contain h[1-6]. You probably need to add a new group for the headings as well. The rest of the editor changes look good. I also skimmed over the rest of the patch, and it all looks perfect!
Attachment #448532 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 29•14 years ago
|
||
This patch should fix the issue with hgroup.
Attachment #448532 -
Attachment is obsolete: true
Attachment #450218 -
Flags: review?(ehsan)
Comment 30•14 years ago
|
||
Comment on attachment 450218 [details] [diff] [review] Patch v6 Looks great! r=me.
Attachment #450218 -
Flags: review?(ehsan) → review+
(In reply to comment #28) > According to http://dev.w3.org/html5/markup/hgroup.html, hrgroup cannot contain > itself, and can only contain h[1-6]. You probably need to add a new group for > the headings as well. http://dev.w3.org/html5/markup/ is non-normative. Furthermore, it restates document conformance requirements, not parsing requirements. hgroup fails validation if it contained in another hgroup. However, the parser must not try to prevent hgroup from nesting with itself.
Comment 32•14 years ago
|
||
(In reply to comment #31) > (In reply to comment #28) > > According to http://dev.w3.org/html5/markup/hgroup.html, hrgroup cannot contain > > itself, and can only contain h[1-6]. You probably need to add a new group for > > the headings as well. > > http://dev.w3.org/html5/markup/ is non-normative. True. But in this case, <http://www.whatwg.org/specs/web-apps/current-work/multipage/sections.html#the-hgroup-element> seems to agree. > Furthermore, it restates > document conformance requirements, not parsing requirements. Right. > hgroup fails > validation if it contained in another hgroup. However, the parser must not try > to prevent hgroup from nesting with itself. I'm not sure why this is relevant...
Henri, the changes discussed in comment 28 is in regards to editor. It IMO makes sense to not allow our editor to insert hgroup elements inside hgroup elements. No parser changes are involved here.
(In reply to comment #33) > Henri, the changes discussed in comment 28 is in regards to editor. It IMO > makes sense to not allow our editor to insert hgroup elements inside hgroup > elements. OK. Sorry. I thought the change was about the old parser. Please disregard my comment.
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed,
dev-doc-needed
Comment 35•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2dcce82f9d66
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Comment 36•14 years ago
|
||
Backed out because of mochitest failures: http://hg.mozilla.org/mozilla-central/rev/8cfc45d20852 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276738388.1276738893.4543.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276738462.1276739411.7119.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276737189.1276738059.32481.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276737959.1276740026.10311.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276738049.1276739534.7716.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 37•14 years ago
|
||
Reduced test: <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/509>. Should see "log: function" in the log.
That testcase is buggy and shouldn't work. Here is a corrected one: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/510
Assignee | ||
Comment 39•14 years ago
|
||
I forgot to send this patch to the try sever, I'm really sorry. The test is fixed and the patch make it well in the try server.
Attachment #450218 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Relanded: http://hg.mozilla.org/mozilla-central/rev/77216d0e9487
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Comment 41•14 years ago
|
||
teoli updated the MDC articles. (<https://developer.mozilla.org/en/HTML/Element/article> and others)
Keywords: dev-doc-needed → dev-doc-complete
Blocks: html5test
You need to log in
before you can comment on or make changes to this bug.
Description
•