| Summary: | Modifying <text rotate=""> doesn't clear the corresponding SVGAnimatedNumberList | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Cameron McCormack (:heycam) <heycam> | ||||
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | eric | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
Created attachment 38462 [details]
Fix and test
Comment on attachment 38462 [details] Fix and test > + * svg/dom/resources/text-rotate-live.js: Added. > + (getRotate): > + (getAndSetRotate): If you're not commenting on these functions individually and the file is new, then there's no need to leave this list of functions in the change log. The idea of prepare-ChangeLog is that it helps you write the ChangeLog entry, not that you should use that it generates as-is. The test code doesn't follow our usual brace style. Open braces for functions go on their own lines. Single line for loop bodies don't get braces around them. I would have put the "var" for the "i" variable inside the for loop. I believe the function results could be arrays rather than strings -- the shouldBe macro knows how to check arrays for equality. We don't use "get" in the names of functions that simply return results. None of these are significant issues, r=me on this patch as it is. (In reply to comment #2) > If you're not commenting on these functions individually and the file is new, > then there's no need to leave this list of functions in the change log. The > idea of prepare-ChangeLog is that it helps you write the ChangeLog entry, not > that you should use that it generates as-is. I will keep that in mind for future patches. > The test code doesn't follow our usual brace style. OK. I see there's no mention of JS style in http://webkit.org/coding/coding-style.html but I'll admit I didn't check it before writing the code. > Open braces for functions > go on their own lines. Single line for loop bodies don't get braces around > them. I would have put the "var" for the "i" variable inside the for loop. I used to too, but now tend to put vars at the top of functions to emphasise that their scope is indeed the whole function and not just the loop. > I believe the function results could be arrays rather than strings -- the > shouldBe macro knows how to check arrays for equality. Acknowledged. I didn't look up shouldBe to see exactly what it was checking; I just copied another test that was doing string comparisons and thought that it was just safer to do that. > We don't use "get" in the names of functions that simply return results. I guess that's Java style rubbing off on me. :) > None of these are significant issues, r=me on this patch as it is. Thanks! Comment on attachment 38462 [details]
Fix and test
Looks good to me too. I wonder if other "parse" implementations are missing a clear() call.
Comment on attachment 38462 [details] Fix and test Clearing flags on attachment: 38462 Committed r47720: <http://trac.webkit.org/changeset/47720> All reviewed patches have been landed. Closing bug. |
When script modifies the rotate="" attribute on a <text> (or <tspan>, etc.) element, the baseVal of the SVGAnimatedNumberList that corresponds to the attribute is not cleared before the attribute is re-parsed. E.g.: <text id="t" rotate="10 20 30"/> <script> t = document.getElementById('t'); l = t.rotate.baseVal; t.setAttribute('rotate', '40 50'); assert(l.numberOfItems == 2); // should be 2 but it's 5 </script>