Closed
Bug 1300374
Opened 8 years ago
Closed 8 years ago
Child-indexed pseudo-class selectors should match without a parent.
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: emilio, Assigned: emilio)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(3 files)
Selectors 4 changed the spec so it should match without a parent.
https://drafts.csswg.org/selectors-4/#child-index
I wonder if this would somehow make our restyling logic bogus in this case, and my intuition is that it could, but how could I test this? Shadow DOM?
Assignee | ||
Updated•8 years ago
|
Summary: Child-indexed selectors should match without a parent. → Child-indexed pseudo-class selectors should match without a parent.
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8787925 [details]
Bug 1300374: child index selectors should match without a parent element.
https://reviewboard.mozilla.org/r/76508/#review76192
For testing you can add a mochitest that checks the root element matches, e.g. document.documentElement.matches(":first-child"). Also for an element not in the document.
For restyling, I don't think we will have a problem, because the root element cannot change whether it matched :first-child or :nth-child(2) etc., as it's impossible for its index (1) to change. So I don't think we even need to set these flags. (I don't think we check them on parent Document nodes anyway.) It looks like you skip setting the flag for nthChildGenericMatches but not for edgeChildMatches. Can you test that that works?
However I don't actually understand how your change makes html:first-child work. Can you explain?
Attachment #8787925 -
Flags: review?(cam)
Assignee | ||
Comment 3•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8787925 [details]
Bug 1300374: child index selectors should match without a parent element.
https://reviewboard.mozilla.org/r/76508/#review76192
Sure, it eliminates the early return from edgeChildMatches and nthChildGenericMatches.
I'll add a test ASAP.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8787925 [details]
Bug 1300374: child index selectors should match without a parent element.
https://reviewboard.mozilla.org/r/76508/#review77490
Attachment #8787925 -
Flags: review?(cam) → review+
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8791463 [details]
Bug 1300374: Reftest for child-indexed selectors matching the root element.
https://reviewboard.mozilla.org/r/78874/#review77494
This is correct, but visually more complex than it needs to be. We should try to write reftests that are self-describing when possible:
http://testthewebforward.org/docs/test-style-guidelines.html#self-describing-tests
http://testthewebforward.org/docs/test-style-guidelines.html#indicating-success
One way to do that would be to make the test do something like this:
<style>
body { color: red; }
:root:first-child #a,
:root:last-child #b,
:root:first-of-type #c,
... {
color: green;
}
</style>
<p id=a>This text must be green.</p>
<p id=b>This text must be green.</p>
<p id=c>This text must be green.</p>
...
r=me if you adjust the test like this.
Attachment #8791463 -
Flags: review?(cam) → review+
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8791464 [details]
Bug 1300374: Web platform test for child-indexed selectors matching the root element and standalone elements.
https://reviewboard.mozilla.org/r/78876/#review77492
::: testing/web-platform/tests/selectors/child-indexed-pseudo-class.html:15
(Diff revision 1)
> + var check = function(element, selectors) {
> + for (var i = 0; i < selectors.length; ++i) {
> + var selector = selectors[i][0];
> + var expected = selectors[i][1];
> + assert_equals(expected, element.matches(selector),
> + "Expected element to " + (expected ? "match " : "don't match ") + selector);
s/don't/not/
Maybe put element.nodeName in the assertion message so we can have different messages for the document.documentElement and <div> tests?
Attachment #8791464 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f16d8caa86fb
child index selectors should match without a parent element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/66dcd191108e
Reftest for child-indexed selectors matching the root element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e5ae5ed5b441
Web platform test for child-indexed selectors matching the root element and standalone elements. r=heycam
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f16d8caa86fb
https://hg.mozilla.org/mozilla-central/rev/66dcd191108e
https://hg.mozilla.org/mozilla-central/rev/e5ae5ed5b441
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
![]() |
||
Comment 14•8 years ago
|
||
So I'm a little unclear on how this works for dynamic restyling in cases when the element has no parent. Is that a case that can't be hit? If it can be hit, how are we handling it?
Flags: needinfo?(emilio+bugs)
Assignee | ||
Comment 15•8 years ago
|
||
See my question in the first comment and Cameron's answer in comment 2.
An element without a parent doesn't change it's index (so we don't need the slow selector flags), and we'd trigger normal restyles when the element changes for other reasons as far as I know.
Flags: needinfo?(emilio+bugs)
![]() |
||
Comment 16•8 years ago
|
||
> An element without a parent doesn't change it's index
This is true for the root. Does this situation not get hit in cases like shadow DOM? I guess in that case we actually have the ShadowRoot as parent?
![]() |
||
Comment 17•8 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: Causes us to fail Acid3, unless Hixie updates it. We may want to call
this out specifically in the relnotes as the test no longer matching the spec.
[Affects Firefox for Android]: Yes.
[Suggested wording]: "Firefox 51 will fail one of the Acid3 tests, because we have updated to
a spec change in CSS selector matching that makes the test invalid. We are waiting for the
test to be updated to track the new spec."
[Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Comment 18•8 years ago
|
||
Comment 19•8 years ago
|
||
Bz, do we really care about a test failing on acid3 to mention it in the release notes?
Thanks
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 20•8 years ago
|
||
Well, it's a guessing game. Will we get worse press if we do it that way, or if we just ship and then people notice and start guessing/speculating about how we have no regression testing or whatever...
What I think we should probably _really_ do is not ship this until at least one other UA is close to shipping it or the test is fixed. But the latter effort seems to have stalled and the former is dependent on someone else moving. Google is getting there, but slowly...
Ms2ger, Anne, what _is_ the status of getting the test fixed?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(annevk)
Flags: needinfo?(Ms2ger)
Comment 21•8 years ago
|
||
Nobody replied to Ian when he asked for rationale. I just did: https://lists.w3.org/Archives/Public/www-archive/2017Jan/0014.html.
Flags: needinfo?(annevk)
![]() |
||
Comment 22•8 years ago
|
||
I feel pretty strongly that given
1) The pushback in https://github.com/w3c/csswg-drafts/issues/695
2) The lack of clarity about whether anyone other than us actually plans to implement this and when
3) The potential PR fallout of starting to fail Acid3
we should not ship this in 51 and see how things evolve. Sylvestre, is it too late to take a very small, very safe patch that disables this in 51?
Flags: needinfo?(sledru)
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #22)
> 1) The pushback in https://github.com/w3c/csswg-drafts/issues/695
See also the pusback-back at https://bugs.chromium.org/p/chromium/issues/detail?id=675670
> 2) The lack of clarity about whether anyone other than us actually plans to
> implement this and when.
See https://groups.google.com/a/chromium.org/d/msg/blink-dev/RgdSWmv0s4g/tZpibBAOEAAJ and https://codereview.chromium.org/2588643004/
> 3) The potential PR fallout of starting to fail Acid3
I think if only, this is the only valid point for disabling this in 51.
![]() |
||
Comment 24•8 years ago
|
||
Yes, I'm aware that this likely _will_ happen in the end after all the grousing is done. That's why I'm saying to only back out on 51, not in general...
Comment 25•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #22)
> is it too late to take a very small, very safe patch that disables this in 51?
There are only a few developers that I would trust to write a "very safe" patch for this and you are one of them :p
Sure, we can give it a try but we would need it today to have SV doing the sign off of RC2 tomorrow (Thursday).
Flags: needinfo?(sledru)
Comment 26•8 years ago
|
||
bz, that is fine, if you can request uplift to m-b and m-r for 51 today it can still make the RC2 build.
Can you do that in a separate bug so we can keep the status flags straight? Thanks!
Flags: needinfo?(bzbarsky)
Updated•8 years ago
|
![]() |
||
Comment 29•8 years ago
|
||
This still needs a relnote because Acid3 is still failing because hixie refuses to update it. So it's worth calling out as a "yes, we know this is failing" kind of thing.
Flags: needinfo?(jcristau)
Comment 31•8 years ago
|
||
I'm not really sure what to do with this and don't think we need it in the main product release notes. Probably more appropriate for the MDN developer relnotes, https://developer.mozilla.org/en-US/Firefox/Releases/51. sheppy, what do you think about putting it into MDN (maybe even for 51, for reference?)
Flags: needinfo?(eshepherd)
Comment 32•8 years ago
|
||
I've added a bullet point to the documentation here:
https://developer.mozilla.org/en-US/Firefox/Releases/51#CSS
Flags: needinfo?(eshepherd)
Keywords: dev-doc-complete
Comment 34•8 years ago
|
||
Test case://jsbin.com/qajowefoni/edit?html,css,output
Updated•8 years ago
|
Flags: needinfo?(Ms2ger)
Comment 35•7 years ago
|
||
Reviewing a PR for BCD and reading this bug and bug 1331994, I wonder if this change really landed in 51. I have that it rode the train of 51 but was backed out in Fx 51 RC2 (according bug 1331994), so actually the first Fx release with it is Fx 52 (and also Fennec 52).
Emilio, can you confirm, or infirm, that I have understood correctly and that what we did in comment 32 is not correct?
Flags: needinfo?(emilio)
Assignee | ||
Comment 36•7 years ago
|
||
(In reply to Jean-Yves Perrier [:teoli] from comment #35)
> Reviewing a PR for BCD and reading this bug and bug 1331994, I wonder if
> this change really landed in 51. I have that it rode the train of 51 but was
> backed out in Fx 51 RC2 (according bug 1331994), so actually the first Fx
> release with it is Fx 52 (and also Fennec 52).
>
> Emilio, can you confirm, or infirm, that I have understood correctly and
> that what we did in comment 32 is not correct?
Yeah, I just tested and in a build from https://archive.mozilla.org/pub/firefox/releases/51.0.1/, the following page:
data:text/html,<style>:root:nth-child(1) { background: green }</style>
Isn't green, so the first release this patch was in was 52.
Flags: needinfo?(emilio)
Comment 37•7 years ago
|
||
Thanks Emilio, flagging :sheppy to get this fixed :-)
Flags: needinfo?(eshepherd)
Comment 38•7 years ago
|
||
Fixed. Sorry it took so long. Still ironing out issues with my default views after a recent mail client change.
Flags: needinfo?(eshepherd)
You need to log in
before you can comment on or make changes to this bug.
Description
•