Closed
Bug 801317
Opened 12 years ago
Closed 12 years ago
Support transforms in SVG fragment identifiers
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: longsonr, Assigned: longsonr)
Details
Attachments
(1 file, 2 obsolete files)
|
11.26 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #671109 -
Flags: review?(dholbert)
|
Assignee | |
Comment 1•12 years ago
|
||
Attachment #671109 -
Attachment is obsolete: true
Attachment #671109 -
Flags: review?(dholbert)
Attachment #671194 -
Flags: review?(dholbert)
|
||
Comment 2•12 years ago
|
||
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.)
|
|
Assignee | |
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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+
|
|
Assignee | |
Comment 5•12 years ago
|
||
|
|
Assignee | |
Comment 6•12 years ago
|
||
Flags: in-testsuite+
|
||
Comment 7•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•