176606 - Unable to extend HTML elements

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[David Hyatt]

Extension of HTML elements has been broken.  This is a regression and prevents 
XBL from being used to extend basic elements like <img>.

Comment 1

[David Hyatt]

This was broken by jst in the fix for 132478.  This change restricted the 
creation of HTML frames such that only HTML content nodes could make them.

Comment 2

[David Hyatt]

The namespace checks here were intentional and (looking at the patch for the 
bug), it appears that other conversions to IsContentOfType were made, all of 
which break XML elements in other namespaces that extend both XUL and HTML 
elements.

Comment 3

[David Hyatt]

As an example, I have been playing around with implementing xhtml2 using 
CSS/XBL, and this prevents me from simply extending existing HTML elements via 
XBL.

As a side note it does look like you can still extend XUL at least, since the 
ConstructXULFrame method was not changed, although the root XUL elements will 
not extend correctly now.

Comment 4

[David Hyatt]

Oops, the bug was 134278.  I had the digits inverted.

Comment 5

[David Hyatt]

Attachment: Patch the tree to handle XHTML2 elements as well as XHTML1 elements.

This patch adds support for XHTML2 elements, and makes sure they get treated
like XHTML1 elements.  I also implemented the ability to jump to anchors in
XHTML2 (which uses id rather than name).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Don't you need to put the uri in gURIArray?

> +  gURIToIDTable->Put(&mathmlKey, NS_INT32_TO_PTR(kNameSpaceID_XHTML2));

Wrong key?

>    // XXX This is incorrect!!!!!!!!!!!!!!!!
> +  // XXX It could be XHTML or XHTML2 or... etc. etc.

The reason GetNamespaceID is hacked like this is because it's used in style
resolution.  This means that if you want XHTML2 elements to match rules in the
XHTML2 namespace this function needs to return the right namespace id....

Perhaps this function should check the nodeinfo namespace id and if that's
kNameSpaceID_None (I think that's the right thing to check for) it should fall
back on kNameSpaceID_HTML?

> +         
cssParser->SetCaseSensitive(mNodeInfo->NamespaceEquals(kNameSpaceID_XHTML) ||
> +                                     
mNodeInfo->NamespaceEquals(kNameSpaceID_XHTML2));

Here, also, it may be better to switch to
SetCaseSensitive(!mNodeInfo->NamespaceEquals(kNameSpaceID_None)), assuming that
does the right thing.

> +  if (mNodeInfo->NamespaceEquals(kNameSpaceID_XHTML) ||
> +      mNodeInfo->NamespaceEquals(kNameSpaceID_XHTML2)) {

And here...I'll stop commenting on this; we should just decide what the right
approach is for callsites like these, where we know we have HTML and just don't
know whether it's tag-soup or not.

Scrolling to id should work in XHTML and HTML4.0, not just in XHTML2....  and it
applies to all elements, not just <a>.  I would hope we have code that already
does that (checks for <a name="foo"> first, then just does
document.getElementById("foo") if we are going to #foo).  If we do not, we
should.  See http://www.w3.org/TR/html401/struct/links.html#h-12.2.3

Comment 7

[David Hyatt]

Duh.  Missed the id checking code at the top of the function.  Verifying with 
my XHTML2 sample now...

Comment 8

[David Hyatt]

I tried returning kNameSpaceID_None in the HTML element function, but that 
causes the UA sheet not to match (which surprised me).  So I've left that 
alone.  I fixed the other problem though (of assuming None).

Comment 9

[David Hyatt]

Attachment: Better patch. This one was actually compiled and tested (gasp!).

Comment 10

[David Hyatt]

Bah. ROFL.  Ignore the changes to nsPresShell.cpp in that patch.  Just look at 
the rest.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 104084 [details] [diff] [review]
Better patch. This one was actually compiled and tested (gasp!).

 nsGenericHTMLElement::GetNameSpaceID(PRInt32& aID) const
 {
-  // XXX
-  // XXX This is incorrect!!!!!!!!!!!!!!!!
-  // XXX
-  aID = kNameSpaceID_XHTML;
+  aID = kNameSpaceID_XHTML; // XXX Return this for XHTML and XHTML2, so that
UA styles can apply to both. -dwh
   return NS_OK;
 }

Do *NOT* remove that XXX comment, that method is broken, the method should
return what you get from mNodeInfo->GetNameSpaceID(), but as you realized,
mozilla kinda falls appart if you do that. That however is a bug in the style
system, not a problem with this code...

Comment 12

[Johnny Stenback (:jst)]

Oh, and in stead of checking if an element's namespace ID is
kNameSpaceID_XHTML2? use nsIContent::IsContentOfType(nsIContent::eHTML) when
possible.

Comment 13

[David Hyatt]

Attachment: New patch.

Comment 14

[Boris Zbarsky [:bzbarsky]]

My point was that the style system uses GetNamespaceID() to get the namespace...
so if you have a sheet in the XHTML namespace it will match (with that patch),
but one in the XHTML2 namespace will not....

So how exactly are you applying the UA rules to XHTML2 elements (eg the XBL
bindings)?  By putting them in a sheet in the XHTML namespace?

That magic namespace directive is sounding mighty nice.... that would move this
CSS-specific hack over into the CSS part of the style system...

Comment 15

[David Hyatt]

Correct, my new sheets are in the XHTML namespace instead of the XHTML2 
namespace.  

I do think eventually we should use a magic directive in the CSS files, but 
I'm content to use the wrong namespace in my UA sheet for now (although you're 
right that namespaced author sheets in the XHTML2 namespace will not apply 
correctly).

I'd be happy to do this before landing if the reviewers feel like it's 
required.  I'd need pointers to the CSS code though.

Comment 16

[Hixie (not reading bugmail)]

IMHO namespaced author sheets in the XHTML2 namespace not applying correctly is
a big deal.

Comment 17

[David Hyatt]

Attachment: Final patch for review/super-review.

Ok, this patch fixes the bug with GetNameSpaceID in HTML element and just
returns the node info's namespace ID.  I patched the style system to understand
kNameSpaceID_None, and I implemented a new magic namespace called

http://www.mozilla.org/xhtml-generic

that matches HTML, XHTML and XHTML2.  

This patch also fixes ConstructHTMLFrame to also check the XHTML and XHTML2
namespaces in the event that the content node is not HTML but extends an HTML
namespace (which happens when using XBL).

I have also adjusted all stylesheets (including the prefs sheet) to use the new
magic namespace where appropriate.

Ready for comments/feedback (or r/sr if you like it that much right out of the
box). ;)

Comment 18

[Hixie (not reading bugmail)]

This seems wrong, both before and after your patch:

Index: content/html/style/src/nsDOMCSSAttrDeclaration.cpp
   
  // look up our namespace.  If we're XHTML, we need to be case-sensitive
  // Otherwise, we should not be
- (*aCSSParser)->SetCaseSensitive(nodeInfo->NamespaceEquals(kNameSpaceID_XHTML));
+ (*aCSSParser)->SetCaseSensitive(nodeInfo->NamespaceEquals(kNameSpaceID_XHTML) ||
+                                 nodeInfo->NamespaceEquals(kNameSpaceID_XHTML2));

Comment 19

[Boris Zbarsky [:bzbarsky]]

Shouldn't you be checking that the content is HTML content in the
kNameSpaceID_None cases in selector-matching?  We don't want to match
non-namespaced XML content with xhtml-namespaced CSS...

Hixie, the code you quote is correct because that's all working with HTML
elements only, so the only question there is just whether it's SGML-based HTML
or XHTML.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I really don't like this bogus generic namespace.  If the DOM WG really insists
on being so pedantically correct rather than having a useful API, then the CSS
WG probably needs to reconsider the interaction of selectors and namespaces. 
What does this change imply for our compliance to the css3-namespaces draft and
the parts of it in css3-selectors?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Actually, though, I'm having trouble finding the part of
http://www.w3.org/TR/DOM-Level-2-Core/ or http://www.w3.org/TR/DOM-Level-2-HTML/
that says how DOM representations of SGML-based documents must handle
namespace-related methods and properties.  What says so?)

Comment 22

[David Hyatt]

Another idea (although even more involved) is to enable @namespace to support
multiple URIs (i.e., allow the establishing of multiple default namespaces),
and/or the defining of prefixes that match multiple namespaces.

Comment 23

[David Hyatt]

Attachment: Patch that just fixes the XBL problem.

Ok, ignoring the controversial XHTML2 portion of the patch, here is the patch
to just fix the XBL regression (without introducing anything regarding XHTML2).

Comment 24

[Hixie (not reading bugmail)]

dbaron: I don't think this patch changes our compliance in any measurable way
(except if a test actually uses the magic namespace, which would be like a test
using a -vnd-foo property). All it does is make the @namespace rule for a
particular namespace match three namespaces rather than one.

The DOM is not affected in any way.

@namespace rules for XHTML namespaces will continue to match in the same way.

Note that we actually already have this hack in a way, as before this patch the
XHTML1 namespace also matched HTML4 content, which is incorrect. This patch
fixes this as well.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104158 [details] [diff] [review]
Patch that just fixes the XBL problem.

r/sr=bzbarsky

Comment 26

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Boarf. DavidH, your problem is only caused by badly chosen namespace URIs.

xhtml2 namespace should be http://www.w3.org/html/xhtml2/2002/06, not
http://www.w3.org/2002/06/xhtml2.

Then you could just use @namespace html url("http://www.w3.org/html/");
to select all namespaces "under" this URI, including html, xhtml1, xhtml2.

DavidH, your proposal of multiple URIs per @namespace rule could perfectly raised
to the WG.

Ian, I think you are wrong, this patch changes our conformance to Selectors CR.
Perhaps not from a test suite POV, but one unique @namespace cannot match both
xhtml1 and xhtml2 languages.

Comment 27

[Boris Zbarsky [:bzbarsky]]

The problem, of course, is that xhtml1 and xhtml2 are not really distinct
languages...

I'm not sure I follow what a different namespace for xhtml2 would enable us to
do, but if it would help, is XHTML2 so far advanced that changing its namespace
would be impossible at this point?

Comment 28

[David Hyatt]

I'm fairly certain we don't treat the @namespace directive as a prefix match,
but probably require an identical match.  

Even if we did, that is a more expensive computation that must be performed
(testing for prefix matching).  I suppose there are clever things we could do
for known namespaces.

As bz said, though, is the xhtml2 namespace likely to change?  The chosen
namespace may be deliberate, since they claim not to be backwards compatible
with xhtml1 (and thus may want to intentionally set themselves apart).  Despite
this claim, many of the tags are absolutely identical and it makes no sense for
us to duplicate all our UA rules. :)
 

Comment 29

[Hixie (not reading bugmail)]

> Then you could just use @namespace html url("http://www.w3.org/html/");
> to select all namespaces "under" this URI, including html, xhtml1, xhtml2.

This is incorrect; namespace URIs are by definition opaque and prefix matching
is therefore out of the question.


> Ian, I think you are wrong, this patch changes our conformance to Selectors CR

It's an extension. Extensions are legitimate, and are not considered to break
conformance, if they are carefully insulated from standard usage, as in this
case. With properties, our extensions are denoted by a '-moz-' prefix, with
namespaces, our extensions are denoted by namespaces in the mozilla.arg space.

Comment 30

[David Hyatt]

Can someone r= the XBL-only patch? :)

Comment 31

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> namespace URIs are by definition opaque and prefix matching
> is therefore out of the question.

Absolutely. That's my point : it's a design mistake. It's a namespace URI and
not a namespace URN. It uses all the bad from URNs and not the good from URIs.
Anyway.

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 104158 [details] [diff] [review]
Patch that just fixes the XBL problem.

r/sr=jst

Comment 33

[Boris Zbarsky [:bzbarsky]]

hyatt, are you planning to check in that reviewed patch sometime?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104158 [details] [diff] [review]
Patch that just fixes the XBL problem.

I just checked this patch in.

Comment 35

[Jerome Lacoste]

Wouahhhh. 

I often follow checkin activities, but this one, with more 13 months between
review and checkin is I think a candidate for a record!