1412011 - stylo: <svg:use> rules for selector-matching are followed inconsistently.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez [:emilio]]

Bug 265894 is buggy. I just noticed it.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Servo patch.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1412011: Test SVG <use> element matching rules.

Review commit: https://reviewboard.mozilla.org/r/193430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193430/

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8922390 [details]
Bug 1412011: Test SVG <use> element matching rules.

https://reviewboard.mozilla.org/r/193430/#review198672

::: testing/web-platform/tests/svg/linking/reftests/use-descendant-combinator.html:8
(Diff revision 1)
> +#test rect {
> +  stroke: red;
> +  stroke-width: 10px;
> +}
> +</style>
> +<p>
> +  You should see a green square, and no red.
> +</p>
> +<svg
> +  version="1.1"
> +  xmlns="http://www.w3.org/2000/svg"
> +  xmlns:xlink="http://www.w3.org/1999/xlink">
> +  <defs>
> +    <g id="square">
> +      <rect width="100" height="100"/>
> +    </g>
> +  </defs>
> +  <g id="test">
> +    <use xlink:href="#square" fill="green" />

Could you extend this slightly to verify that descendant selectors do match if they're entirely contained within the <use> subtree?

I'm imagining:
 - Wrap the <rect> with <g class="inside-use-clone"><g><rect ....></g></g>

 - Add another rule like so:
     .inside-use-clone rect { fill: green }
   (and don't specify fill on the <use> itself)

 - And maybe for good measure, change "test" from being an ID to being a class name, so that the *only* difference between the matching vs. mismatching rule is whether the ancestor is inside the use tree?

If you like, this might make a good second testcase rather than a tweak to this first testcase (though it could share the same reference, I think).

Comment 4

[Daniel Holbert [:dholbert]]

Quick live testcase for this bug, BTW: https://codepen.io/anon/pen/MOgwXa  (circle shouldn't see a red ring)

[Tracking Requested - why for this release]:
Functional regression in Firefox 57 -- bug 265894 (fixed in Firefox 56) becomes partially re-broken, because the Stylo version of its code was missing some cases.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8922390 [details]
Bug 1412011: Test SVG <use> element matching rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193430/diff/1-2/

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8922390 [details]
Bug 1412011: Test SVG <use> element matching rules.

https://reviewboard.mozilla.org/r/193430/#review198688

Thanks - r=me, though you probably want to rename the first testcase:

::: testing/web-platform/meta/MANIFEST.json:173284
(Diff revision 2)
> +   "svg/linking/reftests/use-descendant-combinator.html": [
> +    [
> +     "/svg/linking/reftests/use-descendant-combinator.html",
> +     [

Now that there's a "-002" version of this test, you should probably rename this one to "-001" (i.e. you'd want to rename the file, and update both mentions of its name in the manifest).

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8922390 [details]
Bug 1412011: Test SVG <use> element matching rules.

https://reviewboard.mozilla.org/r/193430/#review198688

Thanks - r=me, though you probably want to rename the first testcase:

::: testing/web-platform/meta/MANIFEST.json:173284
(Diff revision 2)
> +   "svg/linking/reftests/use-descendant-combinator.html": [
> +    [
> +     "/svg/linking/reftests/use-descendant-combinator.html",
> +     [

Now that there's a "-002" version of this test, you should probably rename this one to "-001" (i.e. you'd want to rename the file, and update both mentions of its name in the manifest).

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8922390 [details]
Bug 1412011: Test SVG <use> element matching rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193430/diff/2-3/

Comment 9

[Marcia Knous [:marcia]]

Tracking 57+ based on Comment 4.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8922388 [details] [diff] [review]
Servo patch.

Review of attachment 8922388 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewed in GitHub. Canceling r? here.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Servo bits landed in: https://hg.mozilla.org/integration/autoland/rev/bb08f6cf8f67

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06087989ebf4
Test SVG <use> element matching rules. r=dholbert

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/06087989ebf4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8922388 [details] [diff] [review]
Servo patch.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: broken svgs in subtle cases.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just makes a check consistent in every iteration of the loop, not only the first.
[String changes made/needed]: none.

Comment 16

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

Comment on attachment 8922388 [details] [diff] [review]
Servo patch.

Recent regression, servo related, Beta57+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a01684654651
https://hg.mozilla.org/releases/mozilla-beta/rev/ca0924347f3b

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.