Closed Bug 1412011 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 265894 is buggy. I just noticed it.
Attached patch Servo patch.Splinter Review
Attachment #8922388 - Flags: review?(xidorn+moz)
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).
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 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).
Tracking 57+ based on Comment 4.
Keywords: regression
Priority: -- → P2
Comment on attachment 8922388 [details] [diff] [review]
Servo patch.

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

Reviewed in GitHub. Canceling r? here.
Attachment #8922388 - Attachment is patch: true
Attachment #8922388 - Attachment mime type: text/x-github-pull-request → text/plain
Attachment #8922388 - Flags: review?(xidorn+moz)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/06087989ebf4
Test SVG <use> element matching rules. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/06087989ebf4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(emilio)
Flags: in-testsuite+
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.
Flags: needinfo?(emilio)
Attachment #8922388 - Flags: review+
Attachment #8922388 - Flags: approval-mozilla-beta?
Comment on attachment 8922388 [details] [diff] [review]
Servo patch.

Recent regression, servo related, Beta57+
Attachment #8922388 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.