Page Info should list links and images in SVG pages

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Page Info Window
--
enhancement
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Jillian, Assigned: longsonr)

Tracking

Trunk
Firefox 3 beta3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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.

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

11 years ago
Created attachment 286497 [details] [diff] [review]
patch

links are no more but we can still do image support
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
Attachment #286497 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #286497 - Flags: review? → review?(mano)
What's the browser.js inclusion for?
(Assignee)

Comment 3

11 years ago
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 on attachment 286497 [details] [diff] [review]
patch

Either way, including browser.js for an helper method is an overkill.
Attachment #286497 - Flags: review?(mano) → review-
(Assignee)

Comment 6

11 years ago
(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.

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

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

Comment 8

11 years ago
Created attachment 293291 [details] [diff] [review]
address review comment

Required function moved to utilityOverlay.js
Attachment #286497 - Attachment is obsolete: true
Attachment #293291 - Flags: review?(mano)
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.
Attachment #293291 - Flags: review?(mano) → review+
(Assignee)

Comment 10

11 years ago
Created attachment 293377 [details] [diff] [review]
address final review comments
Attachment #293291 - Attachment is obsolete: true
Comment on attachment 293377 [details] [diff] [review]
address final review comments

a=mconnor on behalf of drivers
Attachment #293377 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
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.
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.
Version: unspecified → Trunk
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.
Status: RESOLVED → VERIFIED
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..
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?  ;)
(Assignee)

Comment 18

11 years ago
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?
> The resolve was just copied from the browser.js version of the function.

Yes, I know.  It was silly there too, of course...
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?
(Assignee)

Comment 21

11 years ago
A separate bug sounds good.
I filed bug 409310, though to be honest, I'm not quite sure why I'm the one spending time on this....
Filed Bug 411310 for the SeaMonkey port.
You need to log in before you can comment on or make changes to this bug.