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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b1

People

(Reporter: bugzilla, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 8 obsolete files)

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.
Summary: Support for defining HTML5 section elements as type = HTMLElement → HTML5 section DOM elements should be of type HTMLElement
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
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.
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.
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...
Status: NEW → ASSIGNED
Component: DOM → DOM: Core & HTML
Assignee: nobody → mounir.lamouri
Attached patch Tests v1 (obsolete) — Splinter Review
Attachment #446932 - Flags: review?(bzbarsky)
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #446933 - Flags: review?(bzbarsky)
Comment on attachment 446933 [details] [diff] [review]
Patch v1

Blake, could you review the old parser changes ?
Attachment #446933 - Flags: review?(mrbkap)
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+
Attached patch Patch v2 (obsolete) — Splinter Review
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)
Attachment #447237 - Flags: review?(timeless)
Josh, could you review the editor changes ?
Attachment #447237 - Flags: review?(timeless)
Attachment #447237 - Flags: review?(brade)
Attachment #447237 - Flags: review-
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 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-
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
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)
Attachment #447441 - Flags: review?(timeless)
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'.
Attached patch Patch v4 (obsolete) — Splinter Review
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)
Attachment #447770 - Flags: review?(timeless)
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+
Blocks: 568509
Attached patch Patch v5 (obsolete) — Splinter Review
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)
Attachment #447777 - Flags: superreview?(jst)
Attachment #447777 - Flags: review?(brade)
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-
Attached patch Patch v5.1 (obsolete) — Splinter Review
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 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+
Attachment #448532 - Flags: superreview?(jst)
Attachment #448532 - Flags: superreview+
Attachment #448532 - Flags: review?(ehsan)
Comment on attachment 448532 [details] [diff] [review]
Patch v5.1

sr=jst, but Ehsan should have a look at the editor changes here.
(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).
I'll do my best to look at the patch today, but I can promise a review tomorrow.  Sorry about the delay.
Blocks: 568515
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-
Attached patch Patch v6 (obsolete) — Splinter Review
This patch should fix the issue with hgroup.
Attachment #448532 - Attachment is obsolete: true
Attachment #450218 - Flags: review?(ehsan)
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.
(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.
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
Reduced test: <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/509>. Should see "log: function" in the log.
Attached patch Patch v6.1Splinter Review
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
Keywords: checkin-needed
Relanded: http://hg.mozilla.org/mozilla-central/rev/77216d0e9487
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 573357
Blocks: 573357
No longer depends on: 573357
Depends on: 573895
Depends on: 608651
You need to log in before you can comment on or make changes to this bug.