1406817 - stylo: Selector matching methods on Element should support :scope pseudo-class

Status: RESOLVED FIXED

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

Description

[Kacper Michajłow [:kasper93]]

Hi,

On :scope doc page https://developer.mozilla.org/en-US/docs/Web/CSS/%3Ascope we can read:

Note: In JavaScript, when used with Element.querySelector, Element.querySelectorAll, or Element.closest, :scope refers to the element on which these methods are invoked. For example, document.body.querySelector(":scope") returns the body element. While support for :scope has been removed for CSS, this use of :scope continues to be supported.

After merging bug 1404897 :scope no longer works in Element.matches. And I suspect after fixing bug 1345025 it will cease to work for all related operations.

So if :scope is planned to be supported in JavaScript this oversight need to be fixed before moving forward with bug 1345025. Otherwise documentation need to be updated. 

Regards,
Kacper

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

I guess the document should be updated. I suppose we are going to unship :scope everywhere.

Also it seems that Chrome doesn't support ":scope" as described in querySelector either.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

This sounds like something unexpectedly left when unshipping :scope, if I understand correctly.

If it was intended that :scope was available in those methods, we should probably issue a new intent to unship to dev-platform.

Comment 3

[Kacper Michajłow [:kasper93]]

Well I think :scope is important. How else would you scope selector to base element? See Quirks section here https://developer.mozilla.org/en-US/docs/Web/API/Element/querySelectorAll#Quirks

> Also it seems that Chrome doesn't support ":scope" as described in querySelector either.

What do you mean? Both Firefox and Chrome prints proper output (1 0) for case decried in Quirks section.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Oh, okay. Looks like this part is not dropped from the spec, and we are likely need to continue supporting :scope in those methods, at least for querySelector and querySelectorAll, given that other browsers also support it.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 6

[Anne (:annevk)]

Were there no regressions on web-platform-tests?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

There isn't any. If we change the impl of closest, there would be one, but it seems there is no test for matches. And there is no test for :scope without scoping either. For the latter case, I'm adding a new css reftest in the Servo PR.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/autoland/rev/1849927185c6

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  So do we have tests here?  Obvious things that should have been tested but were not, and fail without this patch:

  <style>
    div, :scope { color: green; }
  </style>
  <div>This text should be green</div>

and actual matches() calls.  I don't see tests as mentioned in comment 7 in the patch that landed...

I expect that querySelector/querySelectorAll are tested by dom/base/test/test_bug416317* and closest() has that web platform test you mentioned.

Comment 10

[Kacper Michajłow [:kasper93]]

For CSS case there is a perf (layout.css.scoped-style.enabled) that disables :scope support (disabled by default), see Bug 1291515. Generally it looks like it has been dropped. Not sure if you want to follow this perf. I mean the CSS case was not working even with Gecko css engine since v55. So it might be good to keep it disabled to avoid confusion. Docs page clearly says it is not supported anymore.

Important thing is that querySelector/querySelectorAll/matches and so on works correctly with :scope. So indeed appropriate tests are in order here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Web-visible stylo regression, _including_ in 57.

> For CSS case there is a perf (layout.css.scoped-style.enabled) that disables :scope support

You're confusing support for scoped stylesheets and support for parsing the :scope selector.  In a <style> element, :scope is supposed to be parsed but not match.  This works with Gecko in Firefox 55 and 56, but is broken in stylo.  Which means we should probably actually uplift these patches, since this is a web-visible stylo regression.  My testcase in comment 9 tests parsing, not matching.  It looks like we have no :scope tests in layout/style/test/test_selectors.html, and should have. :(

Note that we also have a pref that controls _parsing_ of :scope.  That's "layout.css.scope-pseudo.enabled", defaulting to true.

Comment 12

[Kacper Michajłow [:kasper93]]

Right, I see. Indeed I missed the point here. To my defense I still think the wording on docs is confusing. 

> Gecko 55 (Firefox 55) removes support for <style scoped> and its corresponding :scope pseudo-class.

I read here that :scope pseudo-class was also removed. But in fact :scope still is/should be supported (parsed) and only difference is that it can't be used in regards to <style scoped> which was removed.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9)
> Hmm.  So do we have tests here?  Obvious things that should have been tested
> but were not, and fail without this patch:
> 
>   <style>
>     div, :scope { color: green; }
>   </style>
>   <div>This text should be green</div>
> 
> and actual matches() calls.  I don't see tests as mentioned in comment 7 in
> the patch that landed...

That test was landed in wpt in Servo, see [1]. wpt changes in Servo is not synced to Gecko directly.

I did this just to make landing things easier (so that I don't need coordinate landing). Since this change is shared between Servo and Gecko, I think that should be enough for now, and I expect this test to be upstreamed from Servo and synced to Gecko after a while.

[1] https://github.com/servo/servo/pull/18790/files#diff-31d6953174f9c7de6f255c045a0f92e5

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8916509 [details] [review]
Servo PR

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: content-visible stylo regression on web feature
[Is this code covered by automated tests?]: by a new wpt in Servo side
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: the change is very specific to the given feature, doesn't affect anything else
[String changes made/needed]: n/a

Note that this Servo PR includes wpt change which is not synced to Gecko repo directly, so we should just uplift https://hg.mozilla.org/integration/autoland/rev/1849927185c6 instead.

Comment 15

[Chris Peterson [:cpeterson]]

Xidorn, do we know if this bug affect any real websites? What is the side effect for websites that try to use this :scope feature if we ship Stylo in 57 without uplifting this fix?

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Chris Peterson [:cpeterson] from comment #15)
> Xidorn, do we know if this bug affect any real websites?

No.

> What is the side effect for websites that try to use this :scope feature if
> we ship Stylo in 57 without uplifting this fix?

Rules which use :scope would not be parsed.

This probably isn't a big problem, actually, because without <style scoped>, :scope in stylesheet can only match :root, which isn't quite useful. So I'm not too concerned about the risk of not uplifting it. But I don't think this is risky to uplift either.

Given it's a web feature which is content-visible, and its low risk, I'd prefer we uplift it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> That test was landed in wpt in Servo, see [1]. wpt changes in Servo is not synced to Gecko directly.

Ah, I see.  Thanks!

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916509 [details] [review]
Servo PR

If this issue doesn't affect too many sites and the severity is low, I'd prefer to let this fix ride the 58 train. Thanks!