Child-indexed pseudo-class selectors should match without a parent.

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

({dev-doc-complete})

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox -, firefox51 disabled, firefox52 fixed, firefox53 fixed)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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

3 years ago
Summary: Child-indexed selectors should match without a parent. → Child-indexed pseudo-class selectors should match without a parent.

Comment 2

3 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

3 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 7

3 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

3 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

3 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

3 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

3 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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1311329
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

2 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)
> 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?
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: --- → ?
Bz, do we really care about a test failing on acid3 to mention it in the release notes?
Thanks
Flags: needinfo?(bzbarsky)
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

2 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)
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

2 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.
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...
(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)
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)
Of course.  I filed bug 1331994.
Flags: needinfo?(bzbarsky)
This was resolved, so no longer relnote material I think.
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)
OK, restoring the flag.
Flags: needinfo?(jcristau)
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)
I've added a bullet point to the documentation here:

https://developer.mozilla.org/en-US/Firefox/Releases/51#CSS
Flags: needinfo?(eshepherd)
Perfect, thanks!

Comment 34

2 years ago
Test case://jsbin.com/qajowefoni/edit?html,css,output
Flags: needinfo?(Ms2ger)
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

a year 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)
Thanks Emilio, flagging :sheppy to get this fixed :-)
Flags: needinfo?(eshepherd)
Fixed. Sorry it took so long. Still ironing out issues with my default views after a recent mail client change.
Flags: needinfo?(eshepherd)
Duplicate of this bug: 1184088
You need to log in before you can comment on or make changes to this bug.