458381 - [FIX]empty string does not represent null namespace

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Anne (:annevk)]

The rule @namespace ""; does not associate the default namespace with no namespace.

E.g., if I have an element test in a namespace, it should not be matched by the following rule per the CSS Namespaces Module (see section 2.1 specifically):

@namespace "";
test { background:red }

Comment 1

[Boris Zbarsky [:bzbarsky]]

This was a spec change, right?  I don't recall it working this way before...

Comment 2

[Anne (:annevk)]

The specification never said what to do with the empty string either for the prefix or non-prefix case so we clarified that and decided it would make sense to do the same thing in both.

Comment 3

[Anne (:annevk)]

(FWIW, the spec in question has been through Last Call review and is currently in CR.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Anne, none of the tests in that directory seem to test the following situation:

CSS:

@namespace foo "bar";
root *|* { background: red; display: block }
baz { background: lime }

XML:

<root>
  <baz>This should have a green background</baz>
  <baz xmlns="bar">This should have a green background</baz>
</root>

Would you mind adding such a test?  The obvious fix for this bug in Gecko would regress this testcase, you see.  ;)

Also, would it make sense to toss a

  *|t { background: lime }

rule in before the background:red rule in the test for this bug, so that passing is just all lime while failure is part red, and no reading of the text is needed to see that we pass?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I'm a little torn about tests here.  Ideally I could just steal Anne's tests and check them in as reftests (after some editing to make them all use the same reference)...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Without the bogus comments

Comment 7

[fantasai]

Anne's tests will be part of the official CSS3 Namespaces test suite within the next week or two, and they'll available be under the BSD license. I guess the two things that come in mind here are: Would reftests be the most efficient way to run these tests? (I was advised to use the mochitest framework for selectors tests.) And how can we keep Mozilla's set of tests in sync with W3C's tests (e.g. accepting corrections, additions, etc)? This question has a much broader scope than this bug, though...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, yeah.  That's right; we have selector mochitests too.

I agree it's a bigger question; I was just asking dbaron what he wanted done here short term.  I'm happy to write some mochitests for this.

Comment 9

[Anne (:annevk)]

FWIW, I added the test suggested by bz, I also checked in the tests on dev.w3.org: http://dev.w3.org/CSS/css3-namespace-test-suite/

Comment 10

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

I spent quite a while looking for the relevant part of the spec that
requires this, and finally found it hidden in the terminology section:
http://dev.w3.org/csswg/css3-namespace/#terminology

Comment 11

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

Comment on attachment 341642 [details] [diff] [review]
Without the bogus comments

> nsCSSStyleSheetInner::RebuildNameSpaces()
> {
>   if (mNameSpaceMap) {
>     mNameSpaceMap->Clear();
>   } else {
>     mNameSpaceMap = nsXMLNameSpaceMap::Create();
>     if (!mNameSpaceMap) {
>       return; // out of memory
>     }
>   }
>+
>+  // Override the default namespace map behavior for the null prefix to return
>+  // the wildcard namespace instead of the null namespace.
>+  mNameSpaceMap->AddPrefix(nsnull, kNameSpaceID_Unknown);

Is there enough benefit to Clear-ing an existing namespace map rather than creating a new one that we shouldn't refactor this code into a CreateNameSpaceMap function so you can avoid duplicating it, and then (here, one of the two places), just unconditionally replace the namespace map?

r+sr=dbaron

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Is there enough benefit to Clear-ing an existing namespace map rather than
> creating a new one

I doubt it.  I'll make that change.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: With review comments addressed

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.  Still not sure about the best way forward here on tests...

Comment 15

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

I think it should be relatively straightforward to set up mochitests for this.  It's perhaps a little additional work to write a framework, but once it's there the cost per test would be a good bit lower than reftests, since it would all be in one file.  (See, e.g., test_selectors.html, test_media_queries.html, or test_parse_rule.html .)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, pushed as changeset 3143e3a43176.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Test plus fix for the bug in this patch it caught (*|foo being reserialized as foo if the default namespace is "")

Comment 18

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

Comment on attachment 345249 [details] [diff] [review]
Test plus fix for the bug in this patch it caught (*|foo being reserialized as foo if the default namespace is "")

>+               (!aIsNegated || (!mIDList && !mClassList && !mPseudoClassList && !mAttrList))) 

Maybe condense this into a private method DefaultNamespaceApplies(PRBool aIsNegated)?  Or maybe NamespacesPossible?  It's really saying both:
 * whether the default namespace applies
 * whether it's possible to specify a namespace

I'm not sure what a good name for the method is, given that it's both of those.

{
>+      // We have the default namespace (possibly including the wildcard
>+      // namespace).  Do nothing.  Note that we need the !aIsNegated,
>+      // etc. because default namespaces declarations do not affect negated
>+      // selectors that are not type or universal.
>+    } else if (mNameSpace != kNameSpaceID_Unknown) {

It seems like instead of checking the default-namespace-applies condition in the previous else, you could assert that it does in this one.  Or am I missing something?



As far as the tests, I didn't know that setting innerHTML worked in XHTML.  But I guess it's good that it does.

Could I talk you into adding some tests for :not(), with '*', 'test', and '[attr]' as arguments to the :not(), testing that default namespaces apply to the argument of the not in the first two cases but not the third?  The ensuing serialization tests should exercise this code a bit...

r+sr=dbaron

Comment 19

[Boris Zbarsky [:bzbarsky]]

> It seems like instead of checking the default-namespace-applies condition in
> the previous else, you could assert that it does in this one.  Or am I missing
> something?

I spent a bunch of time thinking about this, actually.  Doing it that way depends on the current parser behavior where there is no way to end up with a non-unknown mNamespace for a negated non-type selector.  I guess that's ok and we just need to remember to fix this code if we ever change parser behavior.

Maybe call the method CanBeNamespaced?

I'll add the tests.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: With the comments addressed

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 346304 [details] [diff] [review]
With the comments addressed

It'd be good to get this serialization snafu fix tested in the beta.  The tests cover all these codepaths (and in fact were what caught the problem the code change is fixing).

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346304 [details] [diff] [review]
With the comments addressed

a=beltzner, this is already marked FIXED though? Confusing!

Comment 23

[Boris Zbarsky [:bzbarsky]]

Sorry, about that.  The bug as originally filed is fixed; this patch was just the tests plus one followup fix for an issue that the tests caught.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pushed changeset da28a31fff2a.