315010 - Page Info should list links and images in SVG pages

Status: VERIFIED FIXED

(Firefox :: Page Info Window, enhancement)

Description

[Jillian]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

SVG documents may contain links and images, which should be listed under the Links and Media tabs in Page Info.

Reproducible: Always

Steps to Reproduce:
Browse to a SVG page, such as http://www.w3schools.com/svg/a_1.svg or http://oskamp.dyndns.org/Tangente/SVG/Tangente.svg

Choose Tools|Page Info.
Actual Results:  
The Links and Media tabs are empty.

Expected Results:  
Links and images should be parsed and displayed under the appropriate tabs.

I linked to .svg page for the sake of simplicity, obviously it doesn't matter whether you invoke Page Info directly from the .svg page, or from the web (html) page by right-clicking on the SVG object and choosing This Frame|View Frame Info.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

links are no more but we can still do image support

Comment 2

[Asaf Romano (gone)]

What's the browser.js inclusion for?

Comment 3

[Robert Longson [:longsonr]]

That's where makeURLAbsolute is defined. In HTML elem.href gives you an absolute URL but in SVG you get back what you put in. The rest of the code in the dialogue assumes you are working with an absolute URL.

Comment 4

[Asaf Romano (gone)]

If i'm not reading this:
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGImageElement.cpp#261
wrong, you can use elem.src to get the absolute url.

Comment 5

[Asaf Romano (gone)]

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

Either way, including browser.js for an helper method is an overkill.

Comment 6

[Robert Longson [:longsonr]]

(In reply to comment #4)

http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGImageElement.cpp#261

That's just a protected internal helper method of the class. It's not part of the SVG DOM and as I understand it therefore not callable from javascript.

That leaves me with two options

a) copy the function I need from browser.js to pageInfo.js
b) move the function to some other file I can use in pageInfo.js - suggestions as to which file to move it to appreciated.

Comment 7

[Asaf Romano (gone)]

*gah*, right, somehow my eyes did s/void/NS_IMETHODIMP/, sorry

I'm fine with a), or with moving it to utilityOverlay.js.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comment

Required function moved to utilityOverlay.js

Comment 9

[Asaf Romano (gone)]

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

>Index: utilityOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/utilityOverlay.js,v
>retrieving revision 1.56
>diff -u -p -8 -r1.56 utilityOverlay.js
>--- utilityOverlay.js	25 Nov 2007 19:12:29 -0000	1.56
>+++ utilityOverlay.js	15 Dec 2007 16:57:39 -0000
>@@ -490,16 +490,26 @@ function isElementVisible(aElement)
>     return false;
> 
>   // If aElement or a direct or indirect parent is hidden or collapsed,
>   // height, width or both will be 0.
>   var bo = aElement.boxObject;
>   return (bo.height > 0 && bo.width > 0);
> }
> 
>+function makeURLAbsolute(base, url)
>+{
>+  // Construct nsIURL.
>+  var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                .getService(Components.interfaces.nsIIOService);
>+  var baseURI  = ioService.newURI(base, null, null);
>+
>+  return ioService.newURI(baseURI.resolve(url), null, null).spec;

while you're here, maybe change this to just use IO.newURI?


r=mano otherwise.

Comment 10

[Robert Longson [:longsonr]]

Attachment: address final review comments

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 293377 [details] [diff] [review]
address final review comments

a=mconnor on behalf of drivers

Comment 12

[Asaf Romano (gone)]

mozilla/browser/base/content/browser.js 1.915
mozilla/browser/base/content/utilityOverlay.js 1.57
mozilla/browser/base/content/pageinfo/pageInfo.js 1.12

Comment 13

[Marcia Knous [:marcia]]

Using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007121804 Minefield/3.0b3pre and loading the URL, I don't see the media tab.

Comment 14

[Florian Quèze [:florian]]

The testcase in the url field illustrated the problem for links but the Links tab is no longer in Page Info so only the images part was fixed.

Changing the URL to a testcase actually using <svg:image> tags.

Comment 15

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007121804 Minefield/3.0b3pre. I verified using the test case that Florian cites in Comment 14.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Maybe I'm missing something, but the absolute-URI stuff seems to be overly complicated in a silly way.  Why is there a resolve() call there?  If it's really needed for some reason (which I can't imagine offhand), that reason should be documented, and a bug filed accordingly..

Comment 17

[Boris Zbarsky [:bzbarsky]]

Also as written MakeURLAbsolute can throw on perfectly reasonable web pages.  I see no provisions for handling that in the code that got checked in.

Is it time for me to mutter about sr yet?  ;)

Comment 18

[Robert Longson [:longsonr]]

The resolve was just copied from the browser.js version of the function.

Another follow-up issue is that makeURLAbsolute is not the only place where nsIIOService could be replaced by IO.newURI is it?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> The resolve was just copied from the browser.js version of the function.

Yes, I know.  It was silly there too, of course...

Comment 20

[Boris Zbarsky [:bzbarsky]]

And that still doesn't address the issue of it throwing, which would basically break page info on some pages.  Do you want a separate bug on that regression?

Comment 21

[Robert Longson [:longsonr]]

A separate bug sounds good.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I filed bug 409310, though to be honest, I'm not quite sure why I'm the one spending time on this....

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Filed Bug 411310 for the SeaMonkey port.