stylo: test_namespace_rule.html fails for several cases

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
3 months ago
11 days ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
There are 17 failures in layout/layout/test/test_namespace_rule.html, and it seems to me they are probably just bugs in how Servo handles @namespace rule, and this test doesn't use much the CSSOM namespace rule.
Priority: -- → P2
Assignee: nobody → ferjmoreno
Assignee: ferjmoreno → nobody
(Assignee)

Comment 1

14 days ago
Created attachment 8885133 [details]
testcase

So there are 6 failures left, which all have empty namespace uri involves. See the testcase.

I suspect these failures are all because we use empty atom for kNameSpaceID_None rather than an independent namespace as mentioned in comment inside nsNameSpaceManager.h [1] and bug 1292278 comment 2.

I'm not sure what should we do for the failures here. All the failing tests were imported from Opera's test suite, and all other browsers pass them. Empty namespace may not really lead to webcompat issue nowadays, but given that Gecko's current behavior is quite interop with others, I'm somehow unwilling to see it gets changed with Stylo.

bholley, what do you think?


[1] https://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/dom/base/nsNameSpaceManager.h#52-53
Flags: needinfo?(bobbyholley)
> we use empty atom for kNameSpaceID_None rather than an independent namespace

I believe this behavior is correct per spec. In every entry point (the XML parser, DOM APIs, CSS @namespace rules), using an empty string for a namespace URL is either an error or is normalized to "no namespace" a.k.a. "the null namespace". Relevant to the attached test case:

https://www.w3.org/TR/REC-xml-names/#defaulting
> If there is no default namespace declaration in scope, the namespace name has no value.
> […]
> The attribute value in a default namespace declaration MAY be empty. This has the same effect, within the scope of the declaration, of there being no default namespace. 

https://drafts.csswg.org/css-namespaces/#terminology
> In CSS Namespaces a namespace name consisting of the empty string is taken to represent the null namespace or lack of a namespace.
(Assignee)

Comment 3

14 days ago
So, that may be correct per spec, and in the attached testcase, replacing "foo|test" with "|test" also gets a green rect in all browsers, which indicates that "foo" namespace really just means null namespace in browsers.

If empty string in xmlns attribute also indicates null namespace, I guess getting a green rect from the test case is the correct behavior per spec.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> I'm not sure what should we do for the failures here. All the failing tests
> were imported from Opera's test suite, and all other browsers pass them.
> Empty namespace may not really lead to webcompat issue nowadays, but given
> that Gecko's current behavior is quite interop with others, I'm somehow
> unwilling to see it gets changed with Stylo.
> 
> bholley, what do you think?

If all the UAs currently agree and disagree with the spec, then nobody has demonstrated that the spec is web-compatible, and stylo is not the right vehicle to do that. Let's align with Gecko and the other UAs.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> If all the UAs currently agree and disagree with the spec

To be precise, I meant "agree with each other and jointly disagree with the spec".
That is not the case here as far as I can tell. There is interop and implementations agree with the spec and this test. It’s only Stylo that has a bug somewhere, but I believe that bug is not "empty string namespace should be distinguished from no namespace" as Xidorn suggested earlier.

Comment 7

14 days ago
All the UAs agree with each other an with the spec here.  "" means null namespace.  Per spec, the square in https://bug1355715.bmoattachments.org/attachment.cgi?id=8885133 should be green, and is in browsers.

It's possible the problem lies in whatever code maps servo namespace strings to Gecko namespace ids?

Comment 8

14 days ago
Note that servo gets a green square here too, so this is really pretty stylo-specific.
(Assignee)

Comment 9

12 days ago
I know what's going on...
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)

Comment 11

11 days ago
mozreview-review
Comment on attachment 8886481 [details]
Bug 1355715 - Use empty atom rather than 'empty' atom for none namespace.

https://reviewboard.mozilla.org/r/157274/#review162408

Doh! r=me
Attachment #8886481 - Flags: review?(bobbyholley) → review+

Comment 12

11 days ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd8b78472a65
Use empty atom rather than 'empty' atom for none namespace. r=bholley

Comment 13

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd8b78472a65
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.