I know that xml:id is now in Working Draft but it is similar to HTML's id attribute (xml:id ID #IMPLIED).

An ID used multiple times causes an xml:id error [1], as described in the spec. This nonfatal error must be reported to the application [2]. [1] http://www.w3.org/TR/xml-id/#terminology [2] http://www.w3.org/TR/xml-id/#errors

See comments in bug 258238.

It reached CR: <http://www.w3.org/TR/2005/CR-xml-id-20050208/>

We do not support xml-stylesheet with a same-document reference. Here is a test case that does not rely on that.

(In reply to comment #3) > See comments in bug 258238. As others explained there, there is a limitation on DTDs that they cannot specify two elements of type ID on a single element (XML Schema has a similar limitation though it's likely to go in a future version, Relax NG does not). This limitation is entirely inherited from SGML and has no use whatsoever. Furthermore, it does not indicate that an XML document containing two IDs on an element is invalid, simply that it cannot be validated with a DTD. In fact, xml:id specifically states that it does not impose that constraint (end of section 4). The addition of xml:id makes the lives of people that are dealing with multiple vocabularies not implemented by the UA a lot simpler, which in turn releases some pressure on UAs to implement more and more (this is the sort of facility that allows people to design vocabularies that can be entirely implemented in XBL for instance, while still providing the sort of facilities expected by end users, such as getElementById). Is there any chance of seeing this support in soon? Given that the CR period is likely to end soon since there are interoperable implementations, and given how useful it can turn out to be, I think it would be a really good thing to have it in.

> Is there any chance of seeing this support in soon? Smaug's been thinking about it. The problem is that there is some very performance-sensitive code involved (CSS selector matching), and any changes to support this must be made in a way that does not regress performance on existing (HTML, and especially "DHTML") content.

Now xml:id is in CR. I think it's time to implement. Even W3C implement xml:id as predefined attributes (like xml:lang) in XML Schema http://www.w3.org/2001/xml.xsd

xml:id is now a W3C Proposed Recommendation, and is also becoming a requirement for other specifications. (For example, it appears in the recent SVG Tiny 1.2 LC Working Draft (13 April 2005).)

Now a Recommendation.

Please see non-bug https://bugzilla.mozilla.org/show_bug.cgi?id=309182 to show why we need this now.

Opera now support xml:id (Opera 9.0)

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

I have some workaround code on http://wiki.svg.org/Starter , look for "getElm("

dtomaszuk, any real reason why this should block 1.9 ? I agree it would be great, but blocking...

xml:id (no error handling) and setIdAttr* support. Trying to keep the normal id attribute handling as fast as possible, xml:id and user defined IDs are slower. Works with CSS style selectors. Needs still some jproffing.

The patch seems to work, shouldn't regress performance and so on, but the bigger question is that whether we want to support xml:id/setIdAttr*. This does after all add some bloat. xml:id is imo good thing, needed especially for DIY XML languages, and it is quite easy to implement xml:id and setIdAttr* in the same time. And Opera does support xml:id.

This is adding a non-trivial amount of code for a case that does not matter on the web (we're not going to support SVG 1.2). It's bloat. We should be removing this kind of code, not adding more. /be

This is useful on the Web, if browsers supported it I know I would use it instead of having to translate from xml:id to id+DTD just for them. It's OT but I fail to see how not supporting SVG 1.2 can be considered an idea even remotely good. 90% of the differences between 1.1 and 1.2 are fixes to things that were insufficiently defined or did not fully cater to interoperability in 1.1. Not supporting it is making a deliberate decision to suck, and sounds very much like an ideologue's decision and surprisingly little like a technically informed one.

(In reply to comment #20) > This is useful on the Web, if browsers supported it I know I would use it > instead of having to translate from xml:id to id+DTD just for them. "Useful on the Web" and "useful to you" are not the same things. "If browsers supported it" is not the same as "if Firefox supports it". We will be removing code as well as adding code, and code implementing features that other browsers than Opera do not and will not support is ripe for removal. > It's OT but I fail to see how not supporting SVG 1.2 can be considered an idea > even remotely good. 90% of the differences between 1.1 and 1.2 are fixes to > things that were insufficiently defined or did not fully cater to > interoperability in 1.1. Not supporting it is making a deliberate decision to > suck, and sounds very much like an ideologue's decision and surprisingly little > like a technically informed one. Please leave your ideology out of it. SVG 1.2 is not for the web. There is no technical reason to impose it on browsers. It was neither designed to interop with web content formats, nor is it in demand among web content authors. Also, the way CR last call was handled makes me want to avoid ever doing business with the parts of the w3c involved in it, but that's icing on the cake. See numerous posts (I know you've read them) on the w3c list, or just (re-)read http://weblogs.mozillazine.org/roc/archives/2006/08/whats_wrong_with_the_svg_worki.html BTW, we still don't have nearly all of 1.1 implemented, so talking about "90%" does not help, even if SVG 1.2 were relevant to the web. /be

Just in case it's not clear (I've written about this often), adding code for a feature not in other browsers has to be a calculated move, based on a credible strategy for improving the web. It can't be just because there's a w3c spec. (Yes, this means MathML is grandfathered. If we could load it on demand safely across versions, and not bind it into the core of Gecko, I would.) There is no strategy behind SVG 1.2 that makes sense for the web. From bug 258238 I do not see any good strategy favoring xml:id. As Jonas wrote there, it is a thin-ice hazard. /be

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

Let's leave SVG 1.2 discussion for another bug. I agree that we should reduce feature bloat and code complexity where ever we have no good use cases. However, I do think we have an important use case for xml:id relating to a standard we want to promote. One of the problems encountered when using XBL is that getElementById can't be used to find widgets or whatever else you might be implementing using XBL. The _bound_ elements are typically in a custom namespace in a multi-namespace compound document unsuitable for a DTD. I had a chat with Jonas and I think we agree that the only realistic option is to work through the issues with xml:id. I'll let him speak for himself on that point though. Olli has agreed to simplify his patch as much as possible by e.g. removing the DOM 3 setIdAttribute stuff. Perhaps we can revisit this bug once we see where that gets us? When I say this is a problem I'm not speaking in the hypothetical. This is an issue we at Joost are finding problematic in our use of XBL.

You shouldn't be sending private markup vocabularies over the wire anyway.

Why? One of Mozilla's primary aims is to promote innovation on the Web. To my mind standardization and promotion of XBL would be a huge step towards this end primarily because it would allow Web content authors to create their own markup dialects. This provides them with a tool to help build what they need (as opposed to many W3C specs). I'm hoping to see "markup libraries" along the lines of JavaScript libraries such as MochiKit at some point.

Because if you send custom (i.e. proprietary) vocabs over the net to users who aren't expecting it (i.e. in any environment that isn't closed), then they can't use it. It's the same reason you shouldn't send data using Word, or Flash, or any other proprietary format. If adding xml:id would encourage authors to create proprietary formats and send them on the Web, then that's the biggest argument to avoid adding it.

I don't think it's right to compare a custom format implemented using XBL to proprietary formats such as those used by Word. XBL is open, and if the user agent knows how to handle XBL then it knows how to handle the custom format.

> [...] If the user agent knows how to handle XBL then it knows how to handle the > custom format. Exactly. User agents shouldn't have to know how to handle XBL. It's like CSS. It's a layer above, for making fancy widgets and stuff out of the boring lower level semantics. It's not about skipping the semantic layer altogether.

Web is not unique thing where XBL can be applied. I guess the main area for XBL is creating vocabularies for mozilla based applications like toolkit and here xmlid would be helpful.

Gecko is primarily aimed at the Web platform, though, and we shouldn't expose the Web platform to xml:id just because non-Web uses might make sense.

I'm actually thinking we should do something even more evil here. I wonder if we should simply say that all attributes named 'id' should be treated as ID attributes. In theory we can claim that we are just extending getElementById and CSS Selector matching. Yes, it's a bit evil, but I think it would make the world a better place.

I think that makes sense. It also keeps everything simple (only one ID per element, etc.).

It makes sense, maybe, but essentially it is "non-standard" way to do the same thing as xml:id. I hope w3c would have reserved "id" for IDs from the beginning... But if we make the change, would other browser vendors do the same. Opera supports xml:id already, no idea about webkit. (Actually some version of khtml used to recognize "id" always as ID)

I agree with Jonas. I think that conceptually, this should be defined analogously to xml:id. That is, there's an "id processor" between the XML processor and the app and the id processor assigns the type ID to any attribute whose local name is id and that is not in a namespace. It wouldn't have to be implemented like this as long as the observable results are indistinguishable (i.e. DOM methods report ids as IDs, XPath id() works, CSS #id works, getElementById() works, etc.).

Hixie, the whole point of stylesheets in the XML world is that you can send custom document formats over the Web to users who aren't expecting them, and still have them display reasonably. The claim that we must agree on all semantics in advance (or ever) is false. Custom document formats based on a shared text syntax are just fine. xml:id is, like stylesheets, one part of enabling this to work properly.

The Web platform includes raw XML which can be and is usefully published on the Web today. It mostly just works. ID support is one annoying wart that can and should be shaved off.

Declaring all attributes named id to be of type ID would be contrary to various specifications. The job here is to implement the spec, not to redefine it. Taking this nonconformant path would simply introduce interoperability problems into the Web that do not currently exist. I think Mozilla has full standards conformance as a goal. While it might have made sense to reserve the attribute name "id" in the early days of XML 1.0, that ship has long since sailed. At this point it is far more important to conform to the relevant specifications, even where they;re a little ugly, than it is to try to create an ideal language that is not shared with other tools.

Jonas, what do you think about this. I took the similar approach as Opera; element has only one ID attribute, but for example if 'id' is removed and there is already 'xml:id', 'xml:id' becomes the ID.

So there are some inconsistencies in the latest patch, mostly related to the fact that IsIDAttributeName always returns true for xml:id even if id is set. For example I suspect nsXMLEventsManager::AttributeChanged will behave wrong if id is set and then xml:id is changed because of this. I actually think that it might be simpler and from a user perspective simpler to let elements possibly have two ids. As sad as that makes me. Or do you think it's possible to fix the above problem without doing that?

(In reply to comment #42) > So there are some inconsistencies in the latest patch, mostly related to the > fact that IsIDAttributeName always returns true for xml:id even if id is set. > For example I suspect nsXMLEventsManager::AttributeChanged will behave wrong if > id is set and then xml:id is changed because of this. Ah, that is true. Will fix that. Sort of leftover from previous patch. Have to remove modtype checks and just mListeners.Enumerate(EnumAndSetIncomplete, aContent); AddListeners(aDocument); I think other callers of IsIDAttributeName should do the right thing. The comment about IsIDAttributeName tries to explain that it may return true whenever attribute is possibly an ID attribute. Maybe the method name should be IsPossibl(e|y)IDAttribute > Or do you think it's possible to fix the above problem without doing that? See above. I'd still like to make us work the same way as Opera. That keeps things simpler to implement and IMO it is up to content author to not to misuse xml:id.

Why is the change to nsFrameManager::HasAttributeDependentStyle needed? I would think it should not be necessary...

In particular, the nsCSSRuleProcessor should probably check aAttribute against IsIDAttributeName, not GetIDAttributeName.

Isn't this somewhat overengineered? Note that it has been suggested that the Attr.IsId attribute, and the Element.setIdAttribute(), Element.setIdAttributeNS(), and Element.setIdAttributeNode() methods, should be dropped altogether. I wouldn't expect the other browsers to implement this. Are we sure we want to add this? (Shouldn't we be working towards making Gecko slimmer, removing minor features like this rather than adding them?)

I'd object to losing Attr.isId, because my xpathgen component requires it.

That's a good example of why we should avoid adding features without carefully thinking of whether we want them. People start depending on them! :-)

Hixie: This patch does not affect any of those DOM properties. IsId is already implemented and the patch does not implement any of the other methods you mention.

Ah! Ok, fair enough then. My bad!

(In reply to comment #44) > Why is the change to nsFrameManager::HasAttributeDependentStyle needed? I > would think it should not be necessary... > If 'id' is removed, 'xml:id' becomes the ID so restyling is needed... > In particular, the nsCSSRuleProcessor should probably check aAttribute against > IsIDAttributeName, not GetIDAttributeName. > yes, this might do the same thing

Fixed nsXMLEventsManager.cpp, IsIDAttributeName used now in nsCSSRuleProcessor::HasAttributeDependentStyle.

I'm not volunteering (yet), but it would be nice to have a detailed document somewhere on how browsers are to support xml:id so that new browsers can simply support it without reverse engineering other browsers. (Like you guys had to do...)

> If 'id' is removed, 'xml:id' becomes the ID so restyling is needed... Yes, but this is covered by the normal AttributeChanged notification for "id" in the null namespace, no? Basically, we want to restyle only if there are id rules that might match the node, not whenever someone changes an id. You'll probably want to have dbaron check over that part of the patch.

That nsFrameManager::HasAttributeDependentStyle was really needed, but now because IsIDAttributeName is used in nsCSSRuleProcessor::HasAttributeDependentStyle, that covers also the removing of an ID attribute (and when another attribute becomes ID).

I'm not a fan of the name IsIDAttributeName since it returns true even if the attribute is in fact not currently the id-attribute. Maybe IsPotentialIDAttributeName? This does in fact seem to cause a bug in nsHTMLDocument::AttributeWillChange/AttributeChanged since you add and remove elements from the id-map based on the xml:id attribute even if that's not the id.

Same thing in nsXULDocument::AttributeChanged. I wonder if it'd be better to change the behavior of the function rather than its name?

Comment on attachment 267552 [details] [diff] [review] v3 Hmm.. actually, the nsHTMLDocument and nsXULDocument changes look right. So just change the function name. nsXMLEventsManager::AttributeChanged. Looks like a change in behavior here? Or is it just a de-optimization? In nsXULDocument::AddElementToMap, why not call GetID()->ToString() instead of GetIDAttributeName + GetAttr? Same in RemoveElementFromMap. Looks good otherwise. So please do the naming change, and it'd be great to get dbaron, bz or roc to sr this since it contains a good amount of style changes.

Changes to nsXMLEventsManager are needed because the ID attribute of the element may have changed.

Hoping either bz or dbaron has time to look at the patch...

Comment on attachment 273817 [details] [diff] [review] v3.1 >Index: content/base/public/nsIContent.h >+ * Note this may return true with many attributes, but only one >+ * is used as an ID at the time. "at a time", not "at the time" Do we really want nsGenericElement::GetIDAttributeName to return xml:id if neither attribute is set and the nodeinfo does have an ID attribute? What if GetIDAttributeName is used to determine what attribute to set? Should we really choose to set xml:id over the language's id? In nsXMLEventsElement::GetIDAttributeName, nsGenericHTMLElement::GetIDAttributeName, nsSVGElement::GetIDAttributeName, nsXTFElementWrapper::GetIDAttributeName, and nsXULElement::GetIDAttributeName, it seems more logical to use kNameSpaceID_None as the parameter to HasAttr, and then set aNameSpaceID only inside the if, if we're going to return early. In nsXULDocument, you should remove kIdentityAttrs since you're making it unused. In nsCSSRuleProcessor.cpp, the comment: + PRInt32 aNameSpaceID, // the namespace of the attribute should say "namespace of attribute NOT to test". And you should also find a nice way to wrap that at less than 80 characters :-) Still in SelectorMatches: + if (attr->mAttr == aAttribute && attr->mNameSpace == aNameSpaceID) { You need the latter condition to be ||ed with attr->mNameSpace == kNameSpaceID_Unknown since there are attribute selectors of the form [*|attr="value"]. You should add a test for this to the mochitests in layout/style/test/. + PRInt32 namespaceID; + if (aAttribute && + aAttribute == data.mContent->GetIDAttributeName(namespaceID) && + aNameSpaceID == namespaceID) { This needs to use IsPotentialIDAttributeName, because if you remove an id attribute on an HTML element, GetIDAttributeName will change to returning xml:id, and won't match. You should add a test for this to the mochitests in layout/style/test/. + if (aData->mNameSpaceID == kNameSpaceID_None && + aData->mAttribute == nsGkAtoms::href && Could you switch the order of these tests so the one most likely to fail comes first? nsIStyleRuleProcessor.h: + PRInt32 mNameSpaceID; + nsIAtom* mAttribute; // |HasAttributeDependentStyle| for which attribute? Could you put the comment above both lines, and a blank line below them? You should add at least some basic tests for the selector matching of xml:id as an ID selector in layout/style/test/. sr=dbaron with those changes.

(In reply to comment #61) > > Do we really want nsGenericElement::GetIDAttributeName to return xml:id > if neither attribute is set and the nodeinfo does have an ID attribute? Does that really matter? > What if GetIDAttributeName is used to determine what attribute to set? That is not what GetIDAttributeName is meant for, at least not after this patch. I'll add a comment that the method returns the name of the current ID attribute. > Should we really choose to set xml:id over the language's id? In Opera language IDs have higher priority.

I'm sort of with david, though I don't feel that strongly about it. What is the disadvantage of returning the 'native' id by default?

(In reply to comment #63) > What is the disadvantage of returning the 'native' id by default? > Would have to check whether there is xml:id attribute or the dtd ID.

Why do the mochitests have pass conditions in both the case where an exception is thrown and where it isn't? Shouldn't one of an exception being thrown or not being thrown be wrong?

Oh, yes, there is still one that kind of catch() {} left. Will fix that.

I checked in the patch, but it may have caused small tp/tp2 regression on bl-bldlnx03. Waiting still for few cycles, then backing out if the regression isn't just noise.

Changes are in content/. Adding IsPotentialIDAttributeNameInternal and GetIDAttributeNameInternal so that non-virtual calls can be used in places which are called often (for example nsGenericElement::ParseAttribute). Adding GetPrimaryIDAttributeName() so that implementations of IsPotentialIDAttributeName and GetIDAttributeName and GetID can be kept in one place in nsGenericElement. The patch might help a bit with the perf, though can't verify with this machine. (Perf numbers vary too much).

+nsIAtom * +nsGenericElement::GetIDAttributeNameInternal(PRInt32& aNameSpaceID) const +{ + aNameSpaceID = kNameSpaceID_None; + nsIAtom* idAtom = GetPrimaryIDAttributeName(); + if (idAtom && HasAttr(aNameSpaceID, idAtom)) { + return idAtom; + } + + idAtom = mNodeInfo->GetIDAttributeAtom(); + if (idAtom && HasAttr(aNameSpaceID, idAtom)) { + return idAtom; + } + + aNameSpaceID = kNameSpaceID_XML; + return nsGkAtoms::id; +} This unfortunately doesn't look any faster since there's two HasAttr calls which are both virtual and slow :(

Those were there in the first patch anyway. IsPotentialIDAttributeName is called more often than GetIDAttributeName. It may (may not) be a bit faster in the new patch. It also depends on how compilers manage to optimize the code. By keeping more code in one place, optimizing is perhaps easier. yeah yeah, guessing... but anyway, I prefer this new patch.

I accept the new member to prototype element, because I've reduced the size of the xul element during 1.9 :)

If you do want to do that, can't you put the script type id and the one bit into 32 bits?

Or the scripttypeIdDcould be smaller. GenericElement can't anyway handle too large scripttypeIDs.

Comment on attachment 274183 [details] [diff] [review] smaller members in protoelement Ugh, no, I get some assertions with this.

Comment on attachment 274174 [details] [diff] [review] Add NODE_MAY_HAVE_ID_ATTRIBUTE flag Don't have time today for anything better than this.

Comment on attachment 274183 [details] [diff] [review] smaller members in protoelement Oops, this works just fine. I forgot to compile also xblcontentsink :( I also verified that using the flag lots of method calls are saved.

FYI, running mochitest with the patch, there are ~2900000 calls to GetIdAttributeName(Internal). ~2300000 cases the flag is not set.

I've run tp2 locally, and couldn't see any pref regression.

Comment on attachment 274183 [details] [diff] [review] smaller members in protoelement >+nsIAtom * >+nsGenericElement::GetIDAttributeNameInternal(PRInt32& aNameSpaceID) const >+{ >+ if (HasFlag(NODE_MAY_HAVE_ID_ATTRIBUTE)) { >+ aNameSpaceID = kNameSpaceID_None; >+ nsIAtom* idAtom = GetPrimaryIDAttributeName(); >+ if (idAtom && HasAttr(aNameSpaceID, idAtom)) { >+ return idAtom; >+ } >+ >+ idAtom = mNodeInfo->GetIDAttributeAtom(); >+ if (idAtom && HasAttr(aNameSpaceID, idAtom)) { >+ return idAtom; >+ } >+ >+ aNameSpaceID = kNameSpaceID_XML; >+ if (HasAttr(aNameSpaceID, nsGkAtoms::id)) { >+ return nsGkAtoms::id; >+ } >+ } >+ >+ aNameSpaceID = kNameSpaceID_Unknown; >+ return nsnull; >+} What i don't like about this is that xhtml element could potentially start looking at other attributes than 'id' if they end up with a nodeinfo with a id-name set. This can happen in documents that inconsistently uses prefixes and has a DTD. IMHO elements for which we know the id should never pay attention to the nodeinfo. Now that you've moved the HasFlag(NODE_MAY_HAVE_ID_ATTRIBUTE) check into the GetID function, it seems like it would be better to make GetID call GetIDAttributeName and have the default implementation of that check the nodeinfo, but let the various subclasses override to just look directly for 'id'. This should even be slightly faster.

This also optimizes GetID even more. GetIDAttributeInternal returns AttrInfo, so no need to re-iterate attributes to find ID attribute.

This is not going to happen.