Last Comment Bug 1355715 - stylo: test_namespace_rule.html fails for several cases
: stylo: test_namespace_rule.html fails for several cases
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: 53 Branch
: Unspecified Unspecified
P2 normal (vote)
: mozilla56
Assigned To: Xidorn Quan [:xidorn] UTC+10
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: stylo stylo-style-mochitest
  Show dependency treegraph
 
Reported: 2017-04-11 23:34 PDT by Xidorn Quan [:xidorn] UTC+10
Modified: 2017-07-14 06:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
testcase (256 bytes, application/xhtml+xml)
2017-07-11 00:02 PDT, Xidorn Quan [:xidorn] UTC+10
no flags Details
Bug 1355715 - Use empty atom rather than 'empty' atom for none namespace. (59 bytes, text/x-review-board-request)
2017-07-13 22:35 PDT, Xidorn Quan [:xidorn] UTC+10
bobbyholley: review+
Details | Review

Description User image Xidorn Quan [:xidorn] UTC+10 2017-04-11 23:34:39 PDT
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.
Comment 1 User image Xidorn Quan [:xidorn] UTC+10 2017-07-11 00:02:25 PDT
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
Comment 2 User image Simon Sapin (:SimonSapin) 2017-07-11 02:25:55 PDT
> 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.
Comment 3 User image Xidorn Quan [:xidorn] UTC+10 2017-07-11 03:27:17 PDT
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.
Comment 4 User image Bobby Holley (:bholley) (busy with Stylo) 2017-07-11 12:06:44 PDT
(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.
Comment 5 User image Bobby Holley (:bholley) (busy with Stylo) 2017-07-11 12:07:15 PDT
(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".
Comment 6 User image Simon Sapin (:SimonSapin) 2017-07-11 12:14:22 PDT
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 User image Boris Zbarsky [:bz] (vacation Aug 14-27) 2017-07-11 12:19:38 PDT
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 User image Boris Zbarsky [:bz] (vacation Aug 14-27) 2017-07-11 12:22:01 PDT
Note that servo gets a green square here too, so this is really pretty stylo-specific.
Comment 9 User image Xidorn Quan [:xidorn] UTC+10 2017-07-13 22:21:35 PDT
I know what's going on...
Comment 10 User image Xidorn Quan [:xidorn] UTC+10 2017-07-13 22:35:43 PDT Comment hidden (mozreview-request)
Comment 11 User image Bobby Holley (:bholley) (busy with Stylo) 2017-07-13 22:55:34 PDT
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
Comment 12 User image Pulsebot 2017-07-13 22:57:10 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2017-07-14 06:20:39 PDT
https://hg.mozilla.org/mozilla-central/rev/bd8b78472a65

Note You need to log in before you can comment on or make changes to this bug.