410502 - Page Info should show SVG images as images

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Also Xlinks are not resolved properly as the conversion from href to absolute url always fails since it uses an undefined variable. http://www.w3schools.com/svg/a_1.svg has an XLink in to test this.

See bug 315010 and bug 409310 for the firefox version of this patch.

Comment 2

[Daniel Brooks [:db48x]]

Comment on attachment 295112 [details] [diff] [review]
patch

>@@ -578,19 +578,22 @@ function grabAll(elem)
>     addImage(elem.src, gStrings.mediaEmbed, "", elem, false);
>   else if (elem.hasAttributeNS(XLinkNS, "href"))
>   {
>     var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>                               .getService(Components.interfaces.nsIIOService);
>     url = elem.getAttributeNS(XLinkNS, "href");
>     try {
>       var baseURI = ioService.newURI(elem.baseURI, elem.ownerDocument.characterSet, null);
>-      url = ioService.newURI(href, elem.ownerDocument.characterSet, baseURI).spec;
>+      url = ioService.newURI(url, elem.ownerDocument.characterSet, baseURI).spec;
>     } catch (e) {}
>-    gLinkView.addRow([getValueText(elem), url, gStrings.linkX, ""]);
>+    if (elem instanceof SVGImageElement)
>+      addImage(url, gStrings.mediaImg, "", elem, false);
>+    else
>+      gLinkView.addRow([getValueText(elem), url, gStrings.linkX, ""]);

The way you have this, svg images will only show up if they are also xlinks. Is that intentional?

Comment 3

[Robert Longson [:longsonr]]

This is an example SVG image...

<svg>
  <image x="200" y="200" width="100px" height="100px"
           xlink:href="myimage.png">
</svg>

So the code does think it is an xlink at the moment. If the xlink attribute is missing then we have no image to show. So yes it was intentional.

Comment 4

[Robert Longson [:longsonr]]

What would you like me to do Daniel?

Comment 5

[Daniel Brooks [:db48x]]

Ah, hrm. I hadn't realized that it worked that way. What's supposed to happen if there is an image tag without an xlink?

Comment 6

[Robert Longson [:longsonr]]

That would be an error, the upshot of which would be that the page would be rendered as if the image did not exist.

Comment 7

[Daniel Brooks [:db48x]]

Comment on attachment 295112 [details] [diff] [review]
patch

In that case, r=db48x

Comment 8

[Daniel Brooks [:db48x]]

Actually, add a comment about the images needing an xlink in order to count before you check it in.

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 10

[[e-mail]]

Comment on attachment 296987 [details] [diff] [review]
address review comment

>+  if (item instanceof SVGImageElement) {
>+    setItemValue("imagetitletext", null);
>+    setItemValue("imagelongdesctext", null);
>+    setItemValue("imagealttext", null);
>+  }
I don't like the way you override the values here. I'd prefer it if you added extra conditions to the cases above to set the values appropriately for SVG.

>   if ((item instanceof HTMLLinkElement || item instanceof HTMLInputElement ||
>        item instanceof HTMLImageElement ||
>+       item instanceof SVGImageElement ||
If you make my other changes then could you also neaten this up; I guess each item on its own line would be best.

>+    if (item instanceof SVGImageElement) {
>+      newImage.width = item.width.baseVal.value;
>+      newImage.height = item.height.baseVal.value;
>+    }
Again, I feel it would be best with this before the previous condition (with an else in between of course).

Comment 11

[Robert Longson [:longsonr]]

Attachment: version for check in

address sr comments

Comment 12

[Robert Longson [:longsonr]]

checked in