Bug 20029 – [XBL] Add more tags

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Julien Chaffraix]

To produce minimal example, we need at least <binding>, <template>, <div> (maybe <content> too).

[Julien Chaffraix]

Created attachment 22262 [details]
Add <binding>, <template>, <div> and <content> stubs

[Dave Hyatt]

I don't understand what the <div> element is in this example... XBL doesn't have a <div> element does it?

[Julien Chaffraix]

(In reply to comment #2)
> I don't understand what the <div> element is in this example... XBL doesn't
> have a <div> element does it?
> 

It does (http://www.w3.org/TR/xbl/#the-div). It is meant to be a styling hook and should be rendered as a block (http://www.w3.org/TR/xbl/#xbl-elements).

[Dave Hyatt]

Right, but it has no DOM interface does it?  I don't see why you would need a special subclass just for a generic block.

[Julien Chaffraix]

(In reply to comment #4)
> Right, but it has no DOM interface does it?

You are right.

>  I don't see why you would need a
> special subclass just for a generic block.

We could just use XBLElement to generate the <div> but there is a few drawbacks:
- make_names.pl currently creates a wrapper for each tags in the factory files which requires a class per tag. We would have to add an option to generate only the qualified name.
- XBL elements have to check their hierarchy to determine if they are in error, I had though of doing it per element.

The two previous points are quite simple to solve so either solution is fine.

[Dave Hyatt]

I do not think you should create extra element classes if the element is not going to have any specialized behavior.

[Dave Hyatt]

If you look at HTML, many of the elements do not have any specialized subclasses.

[Eric Seidel (no email)]

Comment on attachment 22262 [details]
Add <binding>, <template>, <div> and <content> stubs

As Hyatt says, any element which does not require custom behavior does not need to be generated for the factory (But should be generated as part of the XBLNames file).  You might have to add code to the generation scripts for that (But html needs that code anyway... when we finally make HTMLNames and HTMLElementFactory autogen -- there is another bug on that).

If it doesn't need custom behavior, you just test for the element using:

element->hasTagName(XBLNames::divTag)

the generated factory stuff should correctly generate generic XBLElements in that case, since these elements will have the XBL namespace (I know this works for SVG, so I'm sure the generated stuff will do the right hting for XBL too.)

[Julien Chaffraix]

Created attachment 22304 [details]
Updated patch: add the tags and only the useful classes

[Julien Chaffraix]

Created attachment 22341 [details]
Updated: add <binding>, template> and <div> but do not create XBLDivElement class

[Eric Seidel (no email)]

Comment on attachment 22341 [details]
Updated: add <binding>, template> and <div> but do not create XBLDivElement class

xblCustomMappings shouldn't be needed.  HTML will need this too.  This should just be a property of the tag in the .in file.

Also, I don't think you need a comment about <div> in the XBLElement header.  XBLElements probably want to be StyledElements anyway.  I mean, it's also possible to just create an empty XBLDivElement, despite hyatt's suggestions to the contrary.  The cost in terms of code and binary size are negligible.

I think you should make the "generate w/o having a custom class" thing work though, since our first pass in converting HTMLElementFactory will use that.

[Julien Chaffraix]

Created attachment 22371 [details]
Patch updated with Eric's comments

(In reply to comment #11)
> (From update of attachment 22341 [details] [edit])
> xblCustomMappings shouldn't be needed.  HTML will need this too.  This should
> just be a property of the tag in the .in file.

I thought it would interact badly with the .in file format change discussed on the ML that I intend to do (thus my original FIXME). However after thinking about it, I guess I could add just one property without too much trouble.

> Also, I don't think you need a comment about <div> in the XBLElement header. 
> XBLElements probably want to be StyledElements anyway.  I mean, it's also
> possible to just create an empty XBLDivElement, despite hyatt's suggestions to
> the contrary.  The cost in terms of code and binary size are negligible.

HTMLElement also derives from StyledElement so we could do the same here too.

> I think you should make the "generate w/o having a custom class" thing work
> though, since our first pass in converting HTMLElementFactory will use that.

It should work.

[Dave Hyatt]

(In reply to comment #11)
> (From update of attachment 22341 [details] [edit])
> xblCustomMappings shouldn't be needed.  HTML will need this too.  This should
> just be a property of the tag in the .in file.
> 
> Also, I don't think you need a comment about <div> in the XBLElement header. 
> XBLElements probably want to be StyledElements anyway.  I mean, it's also
> possible to just create an empty XBLDivElement, despite hyatt's suggestions to
> the contrary.  The cost in terms of code and binary size are negligible.
> 

XBLElements should not be StyledElements.  XBL is not a language that responds to inline style or to the class attribute, so you would create bugs by inheriting from StyledElement.  I will double-check this with Hixie, but I'm pretty sure the XBL div does not support class or style attributes.
 
I also disagree with making XBLDivElement.  There is absolutely no reason to need that class.  Why generate it?  We should not make classes for elements that don't need them.

[Dave Hyatt]

Ok, the <div> element does support class according to the spec (but no mention is made of style).  Other XBL elements do not.   This makes the hierarchy weird.  It means XBLDivElement might need to derive from StyledElement while the other XBLElement don't.

[Julien Chaffraix]

Created attachment 22378 [details]
Updated patch: make XBLDivElement inherits from StyledElement

[Eric Seidel (no email)]

Comment on attachment 22378 [details]
Updated patch: make XBLDivElement inherits from StyledElement

Seems like either all XBLElements should be StyledElements, or you'll need a StyledXBLElement to inherit from.

[Julien Chaffraix]

As discussed on IRC, even if it may lead to bugs we will simplify the hierarchy by having XBLElement inherit from StyledElement (without creating an XBLDivElement class).
It is ok for a first-pass and David Hyatt pointed that we should make StyledElement a mix-in to solve this issue.

[Julien Chaffraix]

Created attachment 22436 [details]
Repost "Patch updated with Eric's comments" (matches the latest discussion)

[Eric Seidel (no email)]

Comment on attachment 22436 [details]
Repost "Patch updated with Eric's comments" (matches the latest discussion)

LGTM.

[Julien Chaffraix]

Filed bug 20159: "[XBL] XBLElement inheritance issue" for the issue we mentioned.

Committed the patch in r35325.