Closed Bug 801317 Opened 8 years ago Closed 8 years ago

Support transforms in SVG fragment identifiers

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #671109 - Flags: review?(dholbert)
Attached patch updated patch (obsolete) — Splinter Review
Attachment #671109 - Attachment is obsolete: true
Attachment #671109 - Flags: review?(dholbert)
Attachment #671194 - Flags: review?(dholbert)
Comment on attachment 671194 [details] [diff] [review]
updated patch

>+      mozilla::SVGAnimatedTransformList transforms;

You should be able to drop the "mozilla::" prefix there -- this file already has "using namespace mozilla".

This looks good, but I'd like to see it tested a little more thoroughly.  The following cases in particular would be good to test:
 - Invalid transform() being set (and ignored)
 - Valid transform() with multiple types of transformations (e.g. a translate and a rotation), to be sure we're parsing them all successfully & applying them in the right order.
 - Replacing one transform() with another
 - Removing a transform() (e.g. by navigating to a different #svgView() ID)

That should give you better test-coverage of the various edge cases & conditional branches.

(It might be worth updating https://mxr.mozilla.org/mozilla-central/source/content/svg/content/test/test_fragments.html for this, as well.)
Should be all the suggested cases covered + extra bonus tests for duplicate attributes.
Attachment #671194 - Attachment is obsolete: true
Attachment #671194 - Flags: review?(dholbert)
Attachment #673286 - Flags: review?(dholbert)
Comment on attachment 673286 [details] [diff] [review]
address review comments

Thanks! Just one nit on the reftest:

>+++ b/layout/reftests/svg/fragmentIdentifier-01.xhtml
>+    <div>
>+      <object type="image/svg+xml" width="100" height="100" data="fragmentIdentifier-rect-01.svg#svgView(viewBox(0,400,100,100);transform(translate(0,200)))" />
>+      <object id="replace" type="image/svg+xml" width="100" height="100" data="fragmentIdentifier-rect-01.svg#svgView(viewBox(0,400,100,100);transform(translate(0,0)))" />
>+      <object id="remove" type="image/svg+xml" width="100" height="100" data="fragmentIdentifier-rect-01.svg#svgView(viewBox(0,200,100,100);transform(translate(0,200)))" />
>+    </div>
>+    <script type="text/javascript">
>+      window.onload = function() {
>+        document.getElementById("replace").setAttribute("data","fragmentIdentifier-rect-01.svg#svgView(viewBox(0,400,100,100);transform(translate(0,200)))");
>+        document.getElementById("remove").setAttribute("data","fragmentIdentifier-rect-01.svg#svgView(viewBox(0,200,100,100))");
>+        document.documentElement.removeAttribute("class");
>+      }
>+    </script>
>   </body>
> </html>

If I delete the first line of script there (the "replace" line), the test still renders as fully-lime, on my machine.[1]  So we're not really testing whether transform() switcheroos will redraw correctly right now.  Could you tweak that to make it fail if the script doesn't run?

[1] apparently because that <object> initially renders as transparent (and shows the lime <body> background)
Attachment #673286 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/aaa5820e6d4f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.