112842 - XBL content should not generate comment nodes

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[[e-mail]]

While playing around with XBL I have left comments inside <binding> elements.
These comments appear in the DOM (via the DOM inspector). XUL comments do not.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So... Where should I strip comments from?  I can strip them from the XBL
document in general, or I can strip them from inside <content> (Or from inside
other random XBL elements, but that could get harder).  Which should we do?

I'm raising severity to major since this keeps us from documenting XBL bindings
propely...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ian, Neil's afraid to answer my question without your expert guidance.  Could
you provide some?

Comment 3

[Hixie (not reading bugmail)]

You don't want Gecko to strip comments from XBL at all. Doing that would mean
authors could not rely on reliable DOMs. You might want to strip all comments
from XBL files that we ship, though, as part of the build process -- either a
simple XML-based identity transform that drops comment nodes or using #-style
comments and running the XBL files through a preprocessor.

At least that would be my opinion.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The real issue here is that bindings in chrome are very fragile -- adding
comments to <content> makes all sorts of stuff that uses explicit child offsets
break.

This is a problem for authors as much as for chrome, if they use explicit
offsets that way.

Stripping in a preprocessing step is highly suboptimal, since it would make
chrome XBL not behave like other XBL...

We need to either strip comments across the board, or wontfix this bug, and fix
the chrome XBL to not be fragile and finally add some friggin' comments to the
undocumented mess that is chrome XBL.

Comment 5

[Hixie (not reading bugmail)]

Well, using #-style comments would clearly mark that these are not XML comments,
which would be pretty clear.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It will also totally confuse the XML-mode in any sane editor and prevent proper
validity-checking from being performed by said editor....

If it's clearly specified that comments are stripped out of XBL, where is the
"could not rely on reliable DOMs" coming in?  I guess for authors that want
their XBL to work with both current and future Mozilla builds?  But those aren't
really using comments anyway, because it's too painful....

Comment 7

[Hixie (not reading bugmail)]

> It will also totally confuse the XML-mode in any sane editor

No it won't, it would just be treated as text, which is fine.


> and prevent proper validity-checking from being performed by said editor....

You are unlikely to be validating XBL anyway. Well-formedness checking works
fine with such comments. (Well, assuming you don't actually include tag names in
the comments, but that's easy to avoid.)


> If it's clearly specified that comments are stripped out of XBL, where is the
> "could not rely on reliable DOMs" coming in?  I guess for authors that want
> their XBL to work with both current and future Mozilla builds?  But those 
> aren't really using comments anyway, because it's too painful....

I don't see why the spec should require that UAs have special code for their XBL
content models as opposed to other models. Why should XBL be any different than
any other XML language, like, say, XHTML?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> You are unlikely to be validating XBL anyway

Right.  Instead you're going to be wasting your time building chrome, starting
the browser, and looking at the JS console... That makes so much more sense.  

Avoiding stuff in comments is not a good idea.  The goal here is to make the
barrier to documenting XBL low enough that people will do it.  That means
comments need to Just Work somehow.  Adding comments should not require changes
to any other part of the binding. Everything else is secondary in my opinion. I
don't want to ever again be told that a review comment can't be addressed
because XBL is incapable of usefully and painlessly supporting comments.

The spec is defining how <children> works.  It could clearly say that all
non-comment things in <children> are moved over to be anonymous children of the
bound element.  Nothing to do with the actual XBL content model, which the
author can't ever access anyway (I don't believe there are ways of getting a
hold of the actual XBL document from inside a binding; "document" refers to the
document of the element the binding is attached to).

Another alternative is to add an <xbl:comment> which will be stripped out by the
content sink.  This has the same "avoiding" issue that the preprocessor comments do.

Ccing hyatt (should have done this when I took the bug), since he should
probably be in on this decision and since he may have a proposal for a solution
that you'll like more, Ian.

Comment 9

[David Hyatt]

You guys may not know this, but XBL already does evil processing of anonymous content.  Specifically it 
strips whitespace (even though it should not).  It had to strip whitespace because XUL strips 
whitespace, and we all know that's wrong.

We should make that stripping function just strip comments too, and then find some way to do this 
only for XUL.  Since we need to remain backwards-compatible probably, I'm not sure what a good 
solution is here, since what to do with <content> really depends on the bound element's document 
(XUL vs. HTML) and not on the binding document.

Comment 10

[David Hyatt]

A good solution might be to initially parse everything (comments and whitespace) and then make the 
decision on whether to strip the template in the XBL document based off of the first bound element that 
gets attached.  If it's a XUL document do the strip, and if it isn't XUL, then don't.

This does mean we'll waste time processing comments, but maybe after this initial step, we could add a 
little hint to the <binding> tag for our chrome XBL like xul="true" or something cheesy like that.

Comment 11

[[e-mail]]

How about stripping whitespace/comments if the XUL namespace is defined?

Comment 12

[Hixie (not reading bugmail)]

We shouldn't be doing _any_ stripping of any kind on anything, unless it uses
our XUL MIME type, or is bound to an element in a document that used the XUL
MIME type. I could live with stripping comments and whitespace from XBL (and
XUL) in those cases, though.

However, that will probably not address bz's concern since he wants to be able
to rely on using index-based lookup reliably, and this would not work across
document types.

Personally I'm against all stripping at runtime. (At compile time we should
strip as much as possible, IMHO.) The XML specs actually allow comment
stripping, and whitespace stripping if xml:preserve isn't "preserve", FWIW.

Comment 13

[Boris Zbarsky [:bzbarsky]]

As Neil pointed out on irc, <xbl:comment><!-- --></xbl:comment> would actually
be a simplish way of doing a comment that could be stripped out either at
runtime or compile time, as desired....

Comment 14

[Boris Zbarsky [:bzbarsky]]

Where you should read "strip" in that last comment as meaning "assign special
semantics to, just like <xbl:children />" (which we also "strip" if you think
about it).

Comment 15

[Hixie (not reading bugmail)]

<xbl:comment/> feels hacky but I could live with it. It at least makes good
sense in terms of the specs. :-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

So do we have agreement?  Should I add support for <xbl:comment> to the XBL
content sink?

Comment 17

[Hixie (not reading bugmail)]

Hyatt?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Moving bugs that I'm clearly not going to work on in the foreseeable future out
of buglist.

Comment 19

[Paul Silaghi, QA [:pauly]]

Dropping qawanted based on comment 18.

Comment 20

[Boris Zbarsky [:bzbarsky]]

XBL is now disabled in Firefox (Bug 1583314) and is in the process of being removed from Gecko (Bug 1566221), so closing bugs requesting changes to its implementation as wontfix.