Open the attached test case. Result: One red rectangle and one green rectangle. Expected: Tow green rectangles. In the test case there are two SVG <rect> elements. The two <rect> elements are referenced by two <set> elements to change their "fill" attribute from "red" to "green". The first <set> element has the attribute "href" set to "#rect". The second <set> element has the attribute "xlink:href" set to "#rect2". The first <set> element fails to set its target correctly while the second succeeds to set its target correctly. This is a left over work from r234683.
Created attachment 377460 [details] test case
Created attachment 377461 [details] Patch
Created attachment 377467 [details] Patch
Comment on attachment 377467 [details] Patch Clearing flags on attachment: 377467 Committed r249216: <https://trac.webkit.org/changeset/249216>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54803998>
Comment on attachment 377467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377467&action=review > LayoutTests/svg/custom/href-svg-namespace-animate-target.svg:2 > + <desc>Verify the XLinkNames namespace is not requried before the 'href' attribute of the SVG animate elements.</desc> Typoe "requried". Also, is <desc> the best way to add comments to SVG? Since this ends up just serving as a comment.
Comment on attachment 377467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377467&action=review > LayoutTests/svg/custom/href-svg-namespace-animate-target.svg:6 > + <rect id="rect1" width="100" height="100" fill="red"/> > + <set id="set" attributeName="fill" attributeType="CSS" href="#rect1" to="green" begin="0s" fill="freeze"/> > + <rect id="rect2" x="110" width="100" height="100" fill="red"/> > + <set id="set" attributeName="fill" attributeType="CSS" xlink:href="#rect2" to="green" begin="0s" fill="freeze"/> I think we should consider more test coverage: tests that check which attribute is respected if multiple are present in various orders. The code currently will prioritize href over xlink:href no matter what order the attributes appear in, and I’d like to see tests that check that because those are the kinds of things that can become interoperability problems between web browsers. I’d also be interested in at least some tests that check if the elements respond properly if the attributes are changed dynamically by script.
Comment on attachment 377467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377467&action=review >> LayoutTests/svg/custom/href-svg-namespace-animate-target.svg:2 >> + <desc>Verify the XLinkNames namespace is not requried before the 'href' attribute of the SVG animate elements.</desc> > > Typoe "requried". > > Also, is <desc> the best way to add comments to SVG? Since this ends up just serving as a comment. I will fix the typo in a followup patch. >> LayoutTests/svg/custom/href-svg-namespace-animate-target.svg:6 >> + <set id="set" attributeName="fill" attributeType="CSS" xlink:href="#rect2" to="green" begin="0s" fill="freeze"/> > > I think we should consider more test coverage: tests that check which attribute is respected if multiple are present in various orders. The code currently will prioritize href over xlink:href no matter what order the attributes appear in, and I’d like to see tests that check that because those are the kinds of things that can become interoperability problems between web browsers. I’d also be interested in at least some tests that check if the elements respond properly if the attributes are changed dynamically by script. This is true. We are not compliant with the specs in what to be respected when both href and xlink:href exist. For example, this SVG will display a red circle in WebKit although it should show a green rectangle: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <defs> <circle id="circle" cx="50" cy="50" r="50" fill="red"/> <rect id="rect" width="100" height="100" fill="green"/> </defs> <use href="#rect" xlink:href="#circle"/> </svg> This issue was reported in https://bugs.webkit.org/show_bug.cgi?id=195802 which I am going to use to address your comments and to ensure we are compliant with the specs.
(In reply to Darin Adler from comment #7) > Comment on attachment 377467 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377467&action=review > > > LayoutTests/svg/custom/href-svg-namespace-animate-target.svg:2 > > + <desc>Verify the XLinkNames namespace is not requried before the 'href' attribute of the SVG animate elements.</desc> > > Typoe "requried". > > Also, is <desc> the best way to add comments to SVG? Since this ends up just > serving as a comment. The typo and the <desc> issue were fixed in <https://trac.webkit.org/changeset/249579>.