Closed
Bug 1412011
Opened 7 years ago
Closed 7 years ago
stylo: <svg:use> rules for selector-matching are followed inconsistently.
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
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)
41 bytes,
patch
|
emilio
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
Bug 265894 is buggy. I just noticed it.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8922388 -
Flags: review?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (obsolete) |
Comment 7•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Updated•7 years ago
|
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Keywords: regression
Priority: -- → P2
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
Servo bits landed in: https://hg.mozilla.org/integration/autoland/rev/bb08f6cf8f67
Comment 12•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/06087989ebf4 Test SVG <use> element matching rules. r=dholbert
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06087989ebf4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 14•7 years ago
|
||
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(emilio)
Flags: in-testsuite+
Assignee | ||
Comment 15•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a01684654651 https://hg.mozilla.org/releases/mozilla-beta/rev/ca0924347f3b
Comment 18•7 years ago
|
||
(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.
Description
•