1326514 - Implement the CSSOM bits for CSSNamespaceRule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

We don't implement the "namespaceURI" or "prefix" attrs from https://drafts.csswg.org/cssom/#cssnamespacerule

Should check whether other browsers do.

Comment 1

[Boris Zbarsky [:bzbarsky]]

According to https://web-confluence.appspot.com/#!/catalog?releaseKeys=%5B%22Firefox_56.0_Windows_10.0%22,%22Safari_11.0_OSX_10.12.6%22,%22Chrome_61.0.3163.79_Windows_10.0%22,%22Edge_15.15063_Windows_10.0%22%5D&releaseOptions=null&numAvailable=null&currentPage=3&gap=5&itemPerPage=15&searchKey=%22%22 this is implemented by Safari, Chrome, and Edge.

Xidorn, want to take a shot at this?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Seems to be easy.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1326514 - Implement the CSSOM bits for CSSNamespaceRule.

Review commit: https://reviewboard.mozilla.org/r/208962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208962/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8938214 [details]
Bug 1326514 - Implement the CSSOM bits for CSSNamespaceRule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208962/diff/1-2/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c31a94b9729e8e5d5c8911d4ea2f87ed7132c2

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8938214 [details]
Bug 1326514 - Implement the CSSOM bits for CSSNamespaceRule.

https://reviewboard.mozilla.org/r/208962/#review214728

::: layout/style/CSSNamespaceRule.h:39
(Diff revision 2)
>  
>    // WebIDL interfaces
>    uint16_t Type() const final {
>      return nsIDOMCSSRule::NAMESPACE_RULE;
>    }
> +  void GetNamespaceURI(nsAString& aNamespaceURI) {

Justd make the arg `nsString& aNamespaceURI` and then you can call through to GetURLSpec() directly without the "spec" temporary.

::: layout/style/CSSNamespaceRule.h:44
(Diff revision 2)
> +  void GetNamespaceURI(nsAString& aNamespaceURI) {
> +    nsString spec;
> +    GetURLSpec(spec);
> +    aNamespaceURI.Assign(spec);
> +  }
> +  void GetPrefix(nsAString& aPrefix) {

This works, or you could make it a DOMString& arg and then call SetOwnedAtom(GetPrefix(), DOMString::eNullNotExpected) and avoid some copies.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8938214 [details]
Bug 1326514 - Implement the CSSOM bits for CSSNamespaceRule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208962/diff/2-3/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Thanks for the advice! It seems there are lots of things I need to learn with our WebIDL binding stuff.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd981f26866c
Implement the CSSOM bits for CSSNamespaceRule. r=bz

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Actually, eNullNotExpected is not right, and neither was the original nsDependentAtomString thing, because the prefix can be null if this is a default-namespace rule, no?

You presumably want eTreatNullAsEmpty and we should have tests for this if there aren't any so far.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c17dc856bea6
followup - Handle null prefix in namespace rule gracefully for Gecko backend.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> Hmm.  Actually, eNullNotExpected is not right, and neither was the original
> nsDependentAtomString thing, because the prefix can be null if this is a
> default-namespace rule, no?

This is not an issue for Servo backend, because it returns atom!("") when the prefix is null. [1]

But it is an issue for Gecko backend, since that returns a nullable pointer.

There is a wpt checking the behavior, and it crashes when stylo is disabled. I didn't notice that in the try push because I thought all oranges on the wpt task are from unexpected pass of the relevant tests.

> You presumably want eTreatNullAsEmpty and we should have tests for this if
> there aren't any so far.

Anyway, I've pushed a followup to fix it. We may want to change it back to eNullNotExpected after we remove Gecko backend, if that matters.


[1] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/servo/ports/geckolib/glue.rs#1916-1921

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We may want to change it back to eNullNotExpected after we remove Gecko backend, if that matters.

It's a bit more efficient to have eNullNotExpected; the compiler can optimize out the null-check then.  So yeah, we may want to do that.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd981f26866c
https://hg.mozilla.org/mozilla-central/rev/c17dc856bea6

Comment 15

[Chris Mills [:cmills]]

I've documented this:

* The reference pages already existed, so I made sure the Fx support info was up to date: 
https://developer.mozilla.org/en-US/docs/Web/API/CSSNamespaceRule
https://developer.mozilla.org/en-US/docs/Web/API/CSSNamespaceRule/namespaceURI
https://developer.mozilla.org/en-US/docs/Web/API/CSSNamespaceRule/prefix

* I added a note to the Fx59 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/59#CSSOM

* I added this interface to the browser compat data project:
https://github.com/mdn/browser-compat-data/pull/896

Please let me know if this looks OK. Thanks!

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Looks good, thanks.