96994 - Page info > Images doesn't list background images

Status: VERIFIED DUPLICATE of bug 229441

(SeaMonkey :: Page Info, defect, P2)

Description

[Benjamin Mucci]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.3+) Gecko/20010824
BuildID:    20010802408

Go to any pages with background image in body or tables and check Page info >
Images. The background images aren't displayed.

Reproducible: Always

Comment 1

[David Illsley]

Yep, seeing this on Win2k 20010824 on
http://www.hsd.dundeecity.sch.uk/prospectus/default.htm

Should be All/All

Comment 2

[Greg K.]

Confirming under Mac/2001082408. It's not clear, though, whether Mozilla.org intends 
background images to be included in the category of images on the page. Deferring to 
their commentary on that issue.

Interestingly, images not in the background but included via an OBJECT element rather 
than an IMG don't appear in Page Info, either.

Comment 3

[Daniel Brooks [:db48x]]

yea. bug 52730 adds everything mentioned execept background images, and then
some. I'll fix the background images in my next patch.

Comment 4

[Daniel Brooks [:db48x]]

/me thinks an option to "Reassign bug to self and Accept" would be nice to have

but I guess that's not germane to the discussion at hand.

Comment 5

[Daniel Brooks [:db48x]]

I'll be back from vacation soon.

Comment 6

[Daniel Brooks [:db48x]]

0.9.9 it is

Comment 7

[Daniel Brooks [:db48x]]

yay! finnally checked in. Thank you bz :)

Comment 8

[sairuh (rarely reading bugmail)]

mass moving resolved fixed bugs pertaining to page info, view source, find in
page and open web location to [e-mail].

to find all bugspam pertaining to this, set your search string to
"SunsComingUpLikeABigBaldHead".

Comment 9

[Daniel Brooks [:db48x]]

doh, this wasn't actually supposed to be marked fixed.

Comment 10

[Daniel Brooks [:db48x]]

*** Bug 122054 has been marked as a duplicate of this bug. ***

Comment 11

[Daniel Brooks [:db48x]]

I should be able to get to all of these by the time 1.1 is released.

Comment 12

[Neil Marshall]

Attachment: Work in progress

I was trying to fix this, but I've run into a snag.  I can get the list to
display the url to the background image, but I can't get it to preview for some
reason.  

Can someone take a look at the patch and help me figure out how to fix it?

Comment 13

[Daniel Brooks [:db48x]]

I like your background node idea. The problem is that you need to tell
makePreview() about it as well. makeMediaTab() shouldn't be doing new Image(),
makePreivew should. Just use elem.data as the second element in the array of
columns. Then, in makePreview, just add another case to the if (|| nn ==
"background"), and go from there. You may want to change the data parameter on
your custom node to src, just so you don't have to fiddle with getSrc().

Feel free to accept the bug for yourself :)

Comment 14

[Neil Marshall]

Attachment: Displays background images in the list

This patch displays the preview image.

Comment 15

[Daniel Brooks [:db48x]]

Comment on attachment 79036 [details] [diff] [review]
Displays background images in the list

Several strict JS warnings are now appearing in the js console, please
eliminate them.

Comment 16

[Neil Marshall]

Attachment: No Strict warnings

Comment 17

[Daniel Brooks [:db48x]]

Comment on attachment 79061 [details] [diff] [review]
No Strict warnings

hrm. I guess that works. I would have corrected the errors where they occured
with something like ("href" in foo || foo.href), but ok.

r=db48x

Comment 18

[Neil Marshall]

Attachment: Hopefully this catches them all.

Can you take a look at this updated patch?  I fixed the frameset problems and I
came across a strict warning with the file size code so I fixed that as well. 
I used && in a couple places and I'm not 100% sure if thats correct (Although
it worked in all my testcases)

Comment 19

[Daniel Brooks [:db48x]]

does this catch background images set in css?

Comment 20

[Neil Marshall]

No.  Well, I'm not actually 100% sure.  I only check the document.body tag but 
I'm not sure if css would modify that portion of the dom if it has a body 
background specified.  I'm assuming css only writes to the .style.xxxx portion.

It won't detect background images assigned to divs and other objects in its 
current form.

Comment 21

[Daniel Brooks [:db48x]]

I'm thinking it should look for all the images. It'd probably entail iterating
over the entire dom tree, but that shouldn't be too bad. Easiest way I can think
of is a very simple treewalker, just keep the node if it's got a background
image. (via css or html attribute.) I think the next priority will have to be
converting pageInfoOutlinerView to store lists as hashes instead of arrays. This
is probably going to turn up alot of duplicate images on some sites.

Comment 22

[Nicolás Lichtmaier]

The proper way to do this is to walk through all nodes and get the computed
style, then inspecting the CSS properties' values. Note that doing that will
cover *all* cases, there will be no need of getting the "background=" attribute
from anywhere. There will be no need of trating any DOM node specially.

Comment 23

[Daniel Brooks [:db48x]]

Attachment: better fix

now that bug 170143 is in, this is much simpler to implement

Comment 24

[Daniel Brooks [:db48x]]

Attachment: fix a few things timeless noticed

Comment 25

[Daniel Brooks [:db48x]]

Attachment: I must be silly to have thought the last patch was complete.

really, I must take a nap.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 103507 [details] [diff] [review]
I must be silly to have thought the last patch was complete.

+  var match = /^url\((.*)\)/i.exec(elem.ownerDocument.defaultView.
+	       getComputedStyle(elem,
"").getPropertyCSSValue("background-image").cssText);

Ew.  We fixed computed style so you don't have to do that... See
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsC
ontextMenu.js#467

The rest looks ok, but I'll have a closer look tonight or tomorrow morning.

Comment 27

[Daniel Brooks [:db48x]]

Attachment: nuke the regex

thanks bz, anything else?

Comment 28

[Daniel Brooks [:db48x]]

Attachment: fix two minor syntax errors, increase functionality of image preview

it's srange the things I don't think of when I first write stuff.

Comment 29

[jag (Peter Annema)]

Comment on attachment 103522 [details] [diff] [review]
fix two minor syntax errors, increase functionality of image preview

sr=jag. Just remove the "/me thwaps ..." and the "hasChildNodes might be cheap
..." comments.

Comment 30

[Daniel Brooks [:db48x]]

Attachment: well, I suppose I can do that.

any person in particular you want to recommend for the review?

Comment 31

[Daniel Brooks [:db48x]]

reassigning

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 103680 [details] [diff] [review]
well, I suppose I can do that.

r=bzbarsky

Comment 33

[Daniel Brooks [:db48x]]

Attachment: bz's nit :)

Comment 34

[Asa Dotzler [:asa]]

Comment on attachment 103698 [details] [diff] [review]
bz's nit :)

a=asa for checkin to 1.2 (on behalf of drivers). Also noting Jag and BZ
reviews.

Comment 35

[Hixie (not reading bugmail)]

This code misses any backgrounds set on :before and :after pseudo-elements.

Comment 36

[Daniel Brooks [:db48x]]

isn't that a dup of the bug asking that :before and :after generate real nodes?
Any idea about how to improve it so that it does list them? (I guess we could
grep the text of the css files for background stuff)

Comment 37

[Daniel Brooks [:db48x]]

Ok, I'm closing this bug. I'm going to file another bug where I'll go through
the css more thoroughly, and probably in C++.

Comment 38

[Nicolás Lichtmaier]

But this isn't fixed yet! Shouldn't the new bug block this one?

Comment 39

[Patty Mac]

it's not fixed yet, (netscape trunk build; 2002-12-04-03-TRUNK)

Comment 40

[Daniel Brooks [:db48x]]

well... it's fixed 90% of the way ;)
too bad the rest is the bulk of the work.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I've the same effect here with the actual releases from FB 0.7 and Mozilla 1.5.
Only images which are included with <img> are shown. Within the imagelist the
locations of background-images and favicons is shown correctly but are not shown
inside the preview. When i wants to save that images it's also ok (#178469).

I approve the reopening, cause where are the 90% you told? I can't see them.

Comment 42

[Tomer Cohen :tomer]

If there are problems using the catch for background (CSS) images, what about
fetching them again, until the missing 10% of the code will get fixed? I'm truly
missing this ability... 

(Mozilla Seamonkey 1.6a)

Comment 43

[OstGote!]

Similiar bug for firefox is fixed, see Bug 229441.

Still not working with Mozilla 1.8a3 WInNT4. No display of background images and
no saving (Bug 178469, now even nothing can be saved: Bug 239472). A lot of
functionality in Page Info - Media is broken, a big regression and useless UI.

Comment 44

[OstGote!]

Seems now to work with Mozilla 1.8a5. Lists and displays (preview) all
background images, even CSS ones. I guess it is fixed by bug 229441.
 http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/browser/resources/content/pageInfo.js

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, it is fixed now. Marking this one as dupe of bug 229441 because it was the
same issue.

*** This bug has been marked as a duplicate of 229441 ***