Closed
Bug 1326514
Opened 7 years ago
Closed 6 years ago
Implement the CSSOM bits for CSSNamespaceRule
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: xidorn)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
We don't implement the "namespaceURI" or "prefix" attrs from https://drafts.csswg.org/cssom/#cssnamespacerule Should check whether other browsers do.
Updated•6 years ago
|
Priority: -- → P3
Updated•6 years ago
|
Keywords: dev-doc-needed
![]() |
Reporter | |
Comment 1•6 years ago
|
||
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¤tPage=3&gap=5&itemPerPage=15&searchKey=%22%22 this is implemented by Safari, Chrome, and Edge. Xidorn, want to take a shot at this?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 2•6 years ago
|
||
Seems to be easy.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c31a94b9729e8e5d5c8911d4ea2f87ed7132c2
![]() |
Reporter | |
Comment 6•6 years ago
|
||
mozreview-review |
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.
Attachment #8938214 -
Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
Thanks for the advice! It seems there are lots of things I need to learn with our WebIDL binding stuff.
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd981f26866c Implement the CSSOM bits for CSSNamespaceRule. r=bz
![]() |
Reporter | |
Comment 10•6 years ago
|
||
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.
Flags: needinfo?(xidorn+moz)
Comment 11•6 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c17dc856bea6 followup - Handle null prefix in namespace rule gracefully for Gecko backend.
Assignee | ||
Comment 12•6 years ago
|
||
(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
Flags: needinfo?(xidorn+moz)
![]() |
Reporter | |
Comment 13•6 years ago
|
||
> 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd981f26866c https://hg.mozilla.org/mozilla-central/rev/c17dc856bea6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 15•6 years ago
|
||
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!
Flags: needinfo?(xidorn+moz)
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
status-firefox53:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•