Closed
Bug 1406817
Opened 7 years ago
Closed 7 years ago
stylo: Selector matching methods on Element should support :scope pseudo-class
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | - | wontfix |
firefox58 | --- | fixed |
People
(Reporter: kasper93, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
41 bytes,
text/x-github-pull-request
|
ritu
:
approval-mozilla-beta-
|
Details | Review |
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
Reporter | ||
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Reporter | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → xidorn+moz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Summary: Element::Matches doesn't support :scope after enabling Stylo while documentation says it should be supported in JS context. → stylo: Selector matching methods on Element should support :scope pseudo-class
Assignee | ||
Comment 5•7 years ago
|
||
Comment 6•7 years ago
|
||
Were there no regressions on web-platform-tests?
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/1849927185c6
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 9•7 years ago
|
||
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.
Flags: needinfo?(xidorn+moz)
Reporter | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
[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.
tracking-firefox57:
--- → ?
Reporter | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
(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
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 14•7 years ago
|
||
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.
Attachment #8916509 -
Flags: approval-mozilla-beta?
status-firefox57:
--- → affected
Comment 15•7 years ago
|
||
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?
status-firefox56:
--- → unaffected
status-firefox58:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 16•7 years ago
|
||
(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.
Flags: needinfo?(xidorn+moz)
Comment 17•7 years ago
|
||
> That test was landed in wpt in Servo, see [1]. wpt changes in Servo is not synced to Gecko directly.
Ah, I see. Thanks!
Updated•7 years ago
|
Target Milestone: --- → mozilla58
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!
Attachment #8916509 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•2 years ago
|
Blocks: selectors-4
You need to log in
before you can comment on or make changes to this bug.
Description
•