103225 - [FIXr]Fix nsGenericHTMLElement::GetNameSpaceID() and dependants

Status: RESOLVED FIXED

(Core :: XML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Currently XHTML and HTML are keyed to the same thing in the namespace manager.

This means that setting case-sensitivity of CSS parsing based on the namespace
of a node is impossible -- the node would always seem as if it were in both
XHTML and HTML namespaces.

Comment 1

[Johnny Stenback (:jst)]

Actually this is detectable, an elements nsINodeInfo knows the difference, but
nsIContent::GetNameSpaceID() does not. I believe heikki has a fix in another bug
for the broken GetNameSpaceID() implementation so there's no need for this bug.
Marking INVALID (even if I was the one telling bz to file this bug).

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

No, I don't have the fix. We know what the issue is, and I think it would be
better to take care of it in here. The element's node info knows the difference
between HTML and XHTML, but GetNameSpaceID() always gives XHTML namespace. It
needs to be fixed, but additionally something needs to be done about the UA
stylesheets or stylesheet handling. See jst's and hyatt's comments on 2001-10-09
in bug 41983. Changing Summary to reflect the bug.

Boris, wanna have this? ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Heikki, if I took this I would not get to it for months (working on style stuff
and non-mozilla life).  I mostly filed it so we would have a bug on file for the
problem....

Comment 4

[Boris Zbarsky [:bzbarsky]]

So we have the following callers of GetNamespaceID in places where we could have
non-XHTML HTML:

1)  Frame constructor (this would be unaffected by fixing it, since it
    does IsContentOfType checks)
2)  XBL ResolveTag stuff (again, should be fine, since this is just looking up a
    namespace id for the frame constructor)
3)  Accessibility (no idea here.  Aaronl?)
4)  The style matching code, which can simply change:

  aContent->GetNamespaceID(&mNamespaceID);

into (20 lines lower, if we want to make use of the existing IsContentOfType()
check):

  if (aContent->IsContentOfType(nsIContent::eHTML))
    mNamespaceID = kNameSpaceID_XHTML;
  else
    aContent->GetNamespaceID(&mNamespaceID);

So assuming we're OK with that change to style code (David?) and that
accessibility doesn't have a problem, we should just fix this.

Comment 5

[Aaron Leventhal]

As far as the accessibility code is concerned it doesn't matter whether we're in
HTML or XHTML. That would change if we added support for XHTML 2.0, which would
have much bigger differences. However, should I be concerned about that now? Do
I ever need to worry about someone missing XHTML 1.1 an XHTML 2.0 in the same
document?

Comment 6

[Boris Zbarsky [:bzbarsky]]

If we ever implement XHTML2.0, then yes.  But they'd have different namespace
IDs, so you'd be OK.  This bug is just about non-XHTML HTML elements.

Comment 7

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

Aaron: I think you'd need to be concerned even without xhtml2. Fixing
nsGenericHTMLElement::GetNameSpaceID() means making it return kNameSpaceID_None
for HTML elements while still returning kNameSpaceID_XHTML for XHTML elements.

So you might need to switch to using IsContentOfType(nsIContent::eHTML) or doing
something like the snippet in comment 4.

Comment 8

[Aaron Leventhal]

Hmm, I fixed most of these once, but new ones crept into the accessibility code :(

Jonas, how about something like:

if (content->HasAttr(content->GetNamespaceID(&mNamespaceID),
nsAccessibilityAtoms::alt)) {
}

However, to me it seems like it would be better to change HasAttr, GetAttr and
friends to take something like kNameSpaceID_SameAsElement

Comment 9

[Aaron Leventhal]

Hmm, I fixed most of these once, but new ones crept into the accessibility code :(

Jonas, how about something like:

if (content->HasAttr(content->GetNamespaceID(&mNamespaceID),
nsAccessibilityAtoms::alt)) {
}

However, to me it seems like it would be better to change HasAttr, GetAttr and
friends to take something like kNameSpaceID_SameAsElement

Comment 10

[Boris Zbarsky [:bzbarsky]]

Huh?  Why do we care about the element's namespace for HasAttr/GetAttr purposes,
exactly?

Comment 11

[Johnny Stenback (:jst)]

Aaronl, it's really really rare for attributes to ever be in a namespace,
attributes don't inherit their namespace from the element or anything like that.
Only attributes with an explicit prefix are in a namespace.

Comment 12

[Aaron Leventhal]

Okay, I'm not sure what Jonas was saying I need to change.
When do I need to be careful of xhtml/html differences?

Can I just keep using kNameSpaceID_None when using GetAttr and HasAttr in the
accessibility module? For example I just want to get the alt attribute for an
img element.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, you could.

The one place in accessibility that calls GetNameSpaceID is
nsAccessNodeWrap::get_nodeInfo.  It returns the namespace ID to the caller; I
don't know what callers do with it, so I'm not sure what the impact of it
suddenly returning kNamespaceID_None for non-XHTML HTML would be.

Comment 14

[Aaron Leventhal]

No one is using that yet, afaik.

Comment 15

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

As bz and jst said, this does not affect attributes. In almost all cases you
want to use the namespace kNameSpaceID_None when getting and setting attributes.
The exception to this is attributes that use a namespace prefix, like
xbl:inherit and xlink:href.

The bug only affects the namespace of the element itself, and only when calling
GetNameSpaceID(). Also note that it's about changing the namespace *from*
kNameSpaceID_XHTML *to* kNameSpaceID_None

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix.

Comment 17

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

As you probably all know, I'm against this, and what we do in the DOM.  Though
mainly what we do in the DOM rather than this, since it makes it much harder for
DOM script authors to write scripts that work on both
 (1) old-fashioned HTML and
 (2) XHTML within multi-namespace documents.
And I don't think any spec requires that we do what we do in the DOM.

Comment 18

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

s/in the DOM/for the DOM/g

Comment 19

[Boris Zbarsky [:bzbarsky]]

Actually, I did not know that...  If you're not ok with this patch, please feel
free to mark r- on it.  I won't mind.  ;)

Comment 20

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

dbaron: what is it that you don't like? The DOM is unaffected by this patch. It
only changes an inconsitency for internal callers (currently some callers are
forced to use node->GetNodeInfo()->NamespaceID() rather then
node->GetNameSpaceID() )

Comment 21

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

Ah, sorry, reread your comment and I understand what you mean.

While I agree that returning the xhtml namespace for html would ease creating
cross xhtml-html applications (just see my patches in bug 1995), I still think
we should check in this patch. First of all the current state is confusing and
inconsistent, where the nsIDOMNode methods and the nsINodeInfo returns one
namespace, while nsIContent::GetNameSpaceID returns another.

Second, if we do decide on making html return the xhtml namespace we still want
this patch since in that case the nodeinfo should have an xhtml namespaceID and
the method in nsGenericElement would work just fine.

Comment 22

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

(In reply to comment #21)
> Second, if we do decide on making html return the xhtml namespace we still want
> this patch since in that case the nodeinfo should have an xhtml namespaceID and
> the method in nsGenericElement would work just fine.

Right, so I guess this is fine with me.

Comment 23

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

(except that it would expose yet more callers to a change if we decide to change
back)

Comment 24

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

True, but it would be a pretty big change anyway since we have quite a few
callers that use the namespaceID of the nodeinfo. I think we're in one of those
situations where there just aren't any good solutions, just more or less bad ones.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 170320 [details] [diff] [review]
Fix.

Very nice! r+sr=jst

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8b

Comment 27

[Allan Beaufour]

I try to keep my head out of layout/* :), but this patch might have created bug
279027. Our XForms XTF elements are suddenly not in the XForms namespace anymore?