311366 - should make custom elements able to contain blocks (<section>)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[Hixie (not reading bugmail)]

HTML5 may introduce a raft of new "block-level" elements like <section>, <nav>,
etc, which should be able to contain block-level elements like <div> and <p> --
it would be nice if we were forward compatible with that.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Note bug 56245. I think that skipping userdefined elements when computing
containment should allow us to avoid that bug, but it's going to need some
pounding before I'm convinced that that's the right way to go.

Comment 2

[Erik Fabert]

*** Bug 332375 has been marked as a duplicate of this bug. ***

Comment 3

[Henrik Bechmann]

(In reply to comment #2)
> *** Bug 332375 has been marked as a duplicate of this bug. ***
> 
Just a note that a similar problem to that foreseen with HTML5 extensions occurs with (implicit or explicit) user-defined DTD extensions. (see my bug 332375 report for a scenario).

Comment 4

[Erik Fabert]

*** Bug 344908 has been marked as a duplicate of this bug. ***

Comment 6

[aja+bugzilla]

Any target milestone/priority updates?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Getting this fixed in 1.9 would be a big boon to HTML5 adoption.  See the thread with the subject "New block-level elements vs. Gecko" in the [e-mail] mailing list.

Comment 9

[aja+bugzilla]

(In reply to comment #8)
> Getting this fixed in 1.9 would be a big boon to HTML5 adoption.  See the
> thread with the subject "New block-level elements vs. Gecko" in the
> [e-mail] mailing list.
> 

Start of thread: http://lists.w3.org/Archives/Public/public-html/2007Sep/0370.html

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not a blocker, but if you have the cycles blake, it would be great to not block adoption of HTML5

Comment 11

[aja+bugzilla]

FYI: A work-around being proposed in HTML5 WG

Graceful degradation of sectioning elements:
http://lists.w3.org/Archives/Public/public-html/2007Nov/0043.html

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Blake, is this on your radar? I really think it'd be nice to have this one so I sort of want to mark it a blocker, just to make sure it doesn't get lost. But it really isn't something we'd hold release for unfortunately.

Comment 13

[Paolo Bonzini]

is it possible to just add support purely for the HTML5 block elements *existence* without adding (for now) support for their behavior, etc.?

Comment 14

[John Resig]

Just wanted to bring up that there's been some discussion concerning this bug here:
http://ejohn.org/blog/html5-shiv/
http://annevankesteren.nl/2008/01/firefox-html5

Considering that we're the only browser, now, incapable of handling future markup, it does tend to make us look rough around the edges.

Comment 15

[aja+bugzilla]

(In reply to comment #13)
> is it possible to just add support purely for the HTML5 block elements
> *existence* without adding (for now) support for their behavior, etc.?
> 
FWIW: 
A way to do HTML5 graceful degradation in IE6/7 has been identifed:
http://lists.w3.org/Archives/Public/public-html/2008Jan/0207.html
(the post also references this bug).
In my (rather limited) testing overnight, it seems to work well for styling the new HTML5 inline elements as well as block elements.

Comment 16

[Mike Schroepfer]

Given Comment 15 is there some easy way to get this in for FF3?

Comment 17

[James Justin Harrell]

Attachment: change mInclusionBits for userdefined elements to kFlowEntity

Patch v1.3 from Bug 56245 changed mInclusionBits for userdefined elements from kFlowEntity to kInlineEntity. This patch just reverses that.

The output of the testcases from Bug 56245 and Bug 66772 didn't seem to change. I don't know how to run the Mozilla tests so I don't know if anything in there will change.

I'm not familiar with the codebase, so I'm not sure this is the right way to do this. I've never provided a patch before, so I'm not sure what I'm supposed to do here.

Also, I'm not sure why changing kInlineEntity to kFlowEntity makes any different. Lots of elements like <b>, <a>, and <i> have their mInclusionBits set to kInlineEntity, and they can contain block elements just fine.

Comment 18

[James Graham]

(In reply to comment #17)

> The output of the testcases from Bug 56245 and Bug 66772 didn't seem to change.
> I don't know how to run the Mozilla tests so I don't know if anything in there
> will change.

Can you try something involving style elements nesting an unknown element like:

<i><b><section><p>foo</p></section></i></b>

You may find Hixie's Live DOM viewer helpful [1]. I tried making a simple kInlineEntity -> kFlowEntity change a while back and I think cases like that changed behavior (unfortunately, I seem to have broken something in my tree atm so I can't check myself). I don't know if these are sufficiently widely encountered in practice to block this change.

[1] http://software.hixie.ch/utilities/js/live-dom-viewer/

Comment 19

[aja+bugzilla]

A suggested test case:

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<title>test case</title>
<style>
p {background-color:red;}
section {display:block; background-color:lime; border: thick lime solid;}
section p {background-color:lime;}
</style>
</head>
<body>
There should be a single green box below with <i><b>foo</b></i> in it.
<i><b><section><p>foo</p></section></i></b>
</body>
</html>

Comment 20

[James Graham]

I can't recreate the problems that I had before with residual style, though it could just be that there is something extra that you have to do that I've forgotten. One problem that does exist is that something like <p><section><p> leads to nested paragraphs.

Comment 21

[Justin Bradford]

The problem here is that user-defined elements need to either accept flow entities or just inline entities dynamically, depending on their parent tag. So a user-defined element should have it's inclusion bits set to (kInlineEntities|kSelf) as default, and then add the kBlockEntities bits iff it's contextual parent element has those bits set.

nsElementTable.cpp just deals with static definitions, so we should probably introduce two types of user-defined elements (block and inline) with appropriate inclusion bits set. At a higher level in the parser, where we have sufficient context, we can assign a "tag not found" token with the appropriate user-defined element based simply on its parent's element's inclusion bits anded with kBlockEntities.

I'll work on a patch, but let me know if you can think of any problems with my approach.

Comment 22

[Justin Bradford]

There are a number of places in the parser code that are inconsistent regarding user-defined elements. Specifically, nsParserService::IsBlock, CNavDTD::IsBlockElement, and CNavDTD::IsInlineElement always return false for user-defined elements, though those functions should return true (setting aside the problem that we can't distinguish distinct usages of user-defined elements). A new user-defined element could be a block/block-element or an inline-element.

While those routines don't seem to affect normal parsing, there are number of places where handling of user-defined elements is rather frail. It looks like much of this portion of the parser needs to be re-thought and re-written. That big of a change probably wouldn't happen for FF3, however, so I'm looking for a more modest solution.

I propose we introduce eHTMLTags_userdefined_block and eHTMLTags_userdefine_inline just before the current userdefined at the end of the list. The current userdefined stays as-is for it's other esoteric uses, and the block and inline versions are defined more conservatively with appropriate parent and inclusion bits, and limited to the body of the document. Then we add a bit of logic to CNavDTD to transform a userdefined result from LookupTag to userdefined_block/userdefined_inline when appropriate.

It still requires modifying some critical code fairly late in the FF3 timeline, however, but besides the simple solution of reverting the patch for bug 56245 and bug 66772, I don't see a low-risk solution here.

Comment 23

[Maciej Stachowiak]

How would you know if an unknown element is supposed to be block-like or inline-like? You can't look at it and pick one of two options, there has to be a single way of handling them in the parser.

Comment 24

[James Graham]

(In reply to comment #20)

> One problem that does exist is that something like <p><section><p>
> leads to nested paragraphs.

FWIW Gecko has the same nested-paragraph behavior for <p><span><p>. Both this and <p><section><p> match Opera, whilst Safari closes the outer <p> so the <section> is a sibling of the outer <p> as long as there is no block level element between the unknown element and the inner <p> (so <p><section><div><p> leads to nested paragraphs). I'm not sure how relevant this is though.
 

Comment 25

[James Justin Harrell]

@ comments 18 and 19 :

In Firefox 3.0b2 I got:
i
	b
		section
p
	i
		b
			"foo"
i
	b
b

In my local build I got:
i
	b
		section
			p
				"foo"
b

The markup from comment 18 appeared as described.


@ comment 20 :

With this markup:
a<p>b<section>c<p>d

Firefox 3.0b2 parsed it as:
"a"
p
	"b"
	section
		"c"
p
	"d"

My local build parsed it as:
"a"
p
	"b"
	section
		"c"
		p
			"d"


How bad would this change in behavior be? I assume eventually Firefox will want parse text/html the way HTML5 describes. Would this behavior be closer or farther from HTML5's parsing instructions?

Comment 26

[j.j.]

(In reply to comment #25)
 
> How bad would this change in behavior be? I assume eventually Firefox will
> want parse text/html the way HTML5 describes. 

No, this bug is not "Implement HTML5 section elements in Fx3". 
Fx3 is not expected to support HTML5 completely.

It's just an issue of "Degrade Gracefully"
<http://www.w3.org/TR/2007/WD-html-design-principles-20071126/#degrade-gracefully>
This means, in some years from now, authers should be able to code HTML5 which works basicly in pre-HTML5-browsers too.
Fx3 would be such a browser than, like IE6,7, Safari and Opera are already.

> Would this behavior be closer or farther from HTML5's parsing instructions?

IMHO closer and good enough for Fx3, since it allows things like this:
 
 <script src="simple-IE6-7-workaround.js"></script>
 <header style="display:block; background:gold">
  <h1>heading</h1>
  <p>text</text>
 </header>

So I propose:
- Checkin attachment 299101 [details] [diff] [review] (after careful testing with today's content)
  This should be done soon to detect regressions early.
- Further (more comlicated) changes can be done later 
  (before or after 1.9 release), perhaps leading to backout this simple patch

Comment 27

[Jeff Walden [:Waldo]]

So, you're going to need at least a few things here:

Tests

Pick some random set of HTML5-in-HTML4 testcases that rely on the behavior you're adding.  Then create a copy of each of those testcases with a different name; we usually use <testname>.html and <testname>-ref.html.  Modify the -ref versions such that they don't use HTML5 tag names, so, say <section style="display:block"> would become <div>, and so on.  Now compare how a build with your patch renders the two: each pair of files should render equivalently.  If they don't, modify the tests until they do.  Now compare each pair in a build without your patch -- at least some of the files should display differently.  Dump all these pairs of files in mozilla/layout/reftests/bugs, and use the cvsdo utility (see Google) to add them to your tree.  Then make your patch, including these testcases in the patch.

Review

Now that you have a patch with testcases, you can reattach it to this bug.  Make sure to ask someone to review it, as HeroreV "sorta" did -- except you need to request review from a specific person, and his request didn't.  mrbkap@gm will probably work as a reviewer.

For more details on the patch submission process, see the following links (the latter of which is linked from the attachment submission page): 

http://developer.mozilla.org/en/docs/Creating_a_patch
http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree

Comment 28

[James Graham]

Attachment: Possible reftests

Attached a few tests that I think meet the requirements in Comment #27. I assume James Justin Harrell wants to update the patch, otherwise I can do it.

Comment 29

[Boris Zbarsky [:bzbarsky]]

For what it's worth, there are also explicit parser tests that test that given a certain bit of markup we get the right DOM...

Comment 30

[Hixie (not reading bugmail)]

HTML5 just says to handle all unknown elements like inline elements, which is what Firefox does. The problem is that Firefox doesn't treat inline elements like HTML5 says to -- it always closes all open inlines when a block (like <p>) is opened. What we really should do is make inlines in Firefox work like in HTML5, but I don't know if that's easy at this point.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Wouldn't that imply implementing the rest of the HTML5 residual style algorithm to deal with the inline closing prematurely, etc?

Comment 32

[James Justin Harrell]

Attachment: patch 2 - tests added

Here is the patch with the tests from James Graham included. I also added a test to check that unknown elements don't close known inline elements. The unknown-block tests behave differently with this patch applied. The unknown-inline tests show no change.

Comment 33

[Hixie (not reading bugmail)]

(In reply to comment #31)
> Wouldn't that imply implementing the rest of the HTML5 residual style algorithm
> to deal with the inline closing prematurely, etc?

Yeah. Like I said, I don't know how easy it would be at this point. I think on the
long term it's worth it, of course.

Comment 34

[Alistair Potts]

The desired behaviour is present in Safari 2 and 3 (Mac and Win), Opera 9+ and IE6, 7 and 8 (with the document.createElement fix).

For FF3 not to implement this behaviour will not only make it the least forward-compatible (more by accident by design, admittedly) but will also significantly hold back widespread adoption of HTML5 semantics. 

If the fix is pretty straightforward (looks to be) I'd urge its adoption, or FF3 will be the curse of developers everywhere!

Comment 35

[Brendan Eich [:brendan]]

Alistair Potts: save it, advocacy noise in bugs just drives developers away. See https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and avoid the temptation to grandstand.

Blake: how much work is the HTML5 residual style algorithm going to be?

Hixie: how stable is the spec on this alg?

/be

Comment 36

[Hixie (not reading bugmail)]

The HTML5 parser spec is reasonable stable. There are a few things that need changing in response to feedback we've received over the past year or so, but nothing of an architectural nature as far as I can recall. It's mostly about how to handle spaces in tables and stuff like that.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would be really great to have this, but only if we can get a safe and useful patch.

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Sorry for the delay here. I've been extremely busy with other stuff for the past couple of weeks. I'll try to review the patch by tomorrow.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

My concern with the patch is what happens when we're given a testcase such as:

  <i><b><asdf><p>foo </b>bar </i> baz

In this case, all of foo, bar, and baz will be bold and italics because we'll let <i> contain <b> and <b> contains <asdf> and (now) <asdf> contains <p> but when we try to close the bold tag, it'll find that the <p> blocks it from closing, so it remains open. Same with the italics tag. These elements will remain open until the end of the paragraph *and* the next </b> and </i>.

Is the construct <i><section> legal HTML5? At this point, any patch will be risky, but I still like my proposed solution from comment 1 where <section> will only be able to contain a block if its 'parent' tag can contain that block. So the testcase given in comment 19 will actually fail (<b> cannot contain <p>) but if the <i> and <b> tags move inside the <section> it will pass. It's not ideal, but IMO it's better than nothing.

Comment 40

[James Graham]

(In reply to comment #39)
> My concern with the patch is what happens when we're given a testcase such as:
> 
>   <i><b><asdf><p>foo </b>bar </i> baz
> 
> In this case, all of foo, bar, and baz will be bold and italics because we'll
> let <i> contain <b> and <b> contains <asdf> and (now) <asdf> contains <p> but
> when we try to close the bold tag, it'll find that the <p> blocks it from
> closing, so it remains open. Same with the italics tag. These elements will
> remain open until the end of the paragraph *and* the next </b> and </i>.
> 
> Is the construct <i><section> legal HTML5?

In the current HTML 5 algorithm the <asdf> tag is closed before the <p> and the formatting elements are reopened inside the <p>; see http://james.html5.org/cgi-bin/parsetree/parsetree.py?source=%3Ci%3E%3Cb%3E%3Casdf%3E%3Cp%3Efoo+%3C%2Fb%3Ebar+%3C%2Fi%3E+baz%0D%0A Maybe this is the issue that I was trying to remember in comment 18 :)

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #40)
> In the current HTML 5 algorithm the <asdf> tag is closed before the <p> and the
> formatting elements are reopened inside the <p>; see

Sorry, I wasn't clear. My comment refers to Gecko's behavior with the patch attached here. To Gecko's HTML parser, <section> and <asdf> are totally equivalent because it doesn't know about <section>. Therefore, what we do for one of those elements we'll do for both.

Comment 42

[aja+bugzilla]

(In reply to comment #39)
> ...I still like my proposed solution...where <section> will only be able to
> contain a block if its 'parent' tag can contain that block. 

Assuming you meant:
"where <section> will be able to contain a block only if its 'parent' tag can contain that block". (I moved the "only").

IMHO, this would be a big improvement for HTML5 graceful degradation in FF3, as sectioning elements would _normally_ have either <body> or another sectioning element as their parent.

---
Added parity-xx stuff to whiteboard status, for searchability purposes. Please edit as appropriate.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This includes the reftests. It makes us ignore userdefined elements when computing containment.

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 307861 [details] [diff] [review]
Proposed fix

Lets do it

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Still don't think this is a blocker, but it's _really_ wanted.

It's a little scary to take parser changes this late in the game. But the current patch should only affect pages with user-defined tags which is pretty few.

Comment 46

[Brendan Eich [:brendan]]

We should indeed take this for b5. Sicking tried to say that, but the wrong words came out :-P.

/be

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, sorry if I wasn't clear.

I do think that we should take this given that it would really help with deployment of HTML5, and the fix is pretty safe in that if it breaks things, it'll only break pages with user-defined tags, and there are relatively few such pages.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307861 [details] [diff] [review]
Proposed fix

a1.9=beltzner

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure how much blake is around so would be good to get help with landing this.

Comment 50

[aja+bugzilla]

/me offers FREE as in  BEER bug bounty  - A case to whoever gets bug 311366 patch checked in before b5 code freeze, and gets it to stick for FF3 final.

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If the patch sticks Blake should be the one getting the beer. Great work coming up with such a simple solution to this hairy problem.

Comment 52

[Reed Loden [:reed]]

Checking in parser/htmlparser/src/CNavDTD.cpp;
/cvsroot/mozilla/parser/htmlparser/src/CNavDTD.cpp,v  <--  CNavDTD.cpp
new revision: 3.505; previous revision: 3.504
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-1-ref.html,v
done
Checking in layout/reftests/bugs/311366-unknown-block-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-1-ref.html,v  <--  311366-unknown-block-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-1.html,v
done
Checking in layout/reftests/bugs/311366-unknown-block-1.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-1.html,v  <--  311366-unknown-block-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-2-ref.html,v
done
Checking in layout/reftests/bugs/311366-unknown-block-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-2-ref.html,v  <--  311366-unknown-block-2-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-2.html,v
done
Checking in layout/reftests/bugs/311366-unknown-block-2.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-2.html,v  <--  311366-unknown-block-2.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-3-ref.html,v
done
Checking in layout/reftests/bugs/311366-unknown-block-3-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-3-ref.html,v  <--  311366-unknown-block-3-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-3.html,v
done
Checking in layout/reftests/bugs/311366-unknown-block-3.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-block-3.html,v  <--  311366-unknown-block-3.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-1-ref.html,v
done
Checking in layout/reftests/bugs/311366-unknown-inline-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-1-ref.html,v  <--  311366-unknown-inline-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-1.html,v
done
Checking in layout/reftests/bugs/311366-unknown-inline-1.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-1.html,v  <--  311366-unknown-inline-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-2-ref.html,v
done
Checking in layout/reftests/bugs/311366-unknown-inline-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-2-ref.html,v  <--  311366-unknown-inline-2-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-2.html,v
done
Checking in layout/reftests/bugs/311366-unknown-inline-2.html;
/cvsroot/mozilla/layout/reftests/bugs/311366-unknown-inline-2.html,v  <--  311366-unknown-inline-2.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.399; previous revision: 1.398
done

Comment 53

[Justin Bradford]

I made a little page using the new html5 tags (http://etcland.com/). It worked perfectly with yesterday's nightly but it hangs today's nightly.

Comment 54

[Jeff Walden [:Waldo]]

File a new bug with a reduced testcase, please?

Comment 55

[Justin Bradford]

I've created bug 423373, including a reduced testcase (and a slightly more reduced "testcase" that doesn't hang the browser).