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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
relnote-firefox --- -
firefox51 --- disabled
firefox52 --- fixed
firefox53 --- fixed

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?
Summary: Child-indexed selectors should match without a parent. → Child-indexed pseudo-class selectors should match without a parent.
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)
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 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 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 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+
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
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)
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)
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)
(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)
Depends on: 1331994
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!
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)
(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)
You need to log in before you can comment on or make changes to this bug.