526534 - Page Info does not detect multiple instances of the same image on a page if no alt text is provided or the alt text is the same

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Tanner M. Young [:tmyoung]]

When images with the same url and no alt text or no differing alt text appear on a page, the function that populates the tree list misses the later instances of that image.  When "View Image Info" selects the image, it searches for the imageElement which isn't in the list, and loads the first image in the tree instead of the correct image.

We can load the image that has the same url (though we would need to pass that as an argument or something), but it may be scaled to different size than the original image, as I have encountered before; so height and width data for web developers and other users would be meaningless--and perhaps latently so.

On the other hand, there are some sites that might have an image repeated dozens or hundreds of times, and each of them would be shown in the tree view, which probably isn't a bad thing, but it's worth thinking about.

Dao: What do you think is the best and most reasonable way to fix this?

Comment 1

[Tanner M. Young [:tmyoung]]

Attachment: Reduced Testcase

Here is the testcase.

Comment 2

[Florian Quèze [:florian]]

(In reply to comment #0)

> On the other hand, there are some sites that might have an image repeated
> dozens or hundreds of times, and each of them would be shown in the tree view,
> which probably isn't a bad thing, but it's worth thinking about.
Having hundreds of rows for the same image (common situation for images used as background) made the media tab very hard to use. See bug 201264.

Comment 3

[Alice0775 White]

This issue seems to be regression of Bug 524106

Comment 4

[Tanner M. Young [:tmyoung]]

(In reply to comment #2)
> Having hundreds of rows for the same image (common situation for images used as
> background) made the media tab very hard to use. See bug 201264.

That's what I was afraid of.  I just don't know how to work around this and how to reconcile the need for some users to get the correct sizing and alt-text data they need from an image without having an overpopulated list.

Perhaps we need a solution that searches for the imageURL in the tree (returning the correct image) but lists information about the imageElement and properly scales it.

Comment 5

[Alice0775 White]

I filed the other bug Bug 526721 - "Page Info" does not detect of images in a document,there are same url,same alt and different scale

Comment 7

[Dão Gottwald [:dao]]

addImage should just make sure not to skip gImageElement.

Comment 8

[Tanner M. Young [:tmyoung]]

Assigning to me.

Comment 9

[Tanner M. Young [:tmyoung]]

Attachment: Patch that ensure gImageElement in image tree

This patch just adds a check to addImage that the image either be a new one in the tree (the original behavior) or the gImageElement.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 410987 [details] [diff] [review]
Patch that ensure gImageElement in image tree

Below there's code that counts duplicate images. That needs to be taken care of as well.

Comment 11

[Tanner M. Young [:tmyoung]]

(In reply to comment #10)
> Below there's code that counts duplicate images. That needs to be taken care of
> as well.

What is the most elegant way to do that?

Comment 12

[Tanner M. Young [:tmyoung]]

How do we want the counter to behave?  Do we want it count the gImageElement as part of the total of the image that it would part of by default (if it wasn't be specially selected) in addition?  I'm not sure what we want to change it to.

Comment 13

[Dão Gottwald [:dao]]

If gImageHash[url][type][alt] already existed, we need to count that as a duplicate, which means to increase the counter by one.

Comment 15

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.1

(In reply to comment #13)
> If gImageHash[url][type][alt] already existed, we need to count that as a
> duplicate, which means to increase the counter by one.

That's what I thought you meant, I just wanted to make sure.

Comment 16

[Alice0775 White]

(In reply to comment #15)
> Created an attachment (id=411208) [details]
> Patch v 1.1
This patch do not solve Bug 526721.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 411208 [details] [diff] [review]
Patch v 1.1

>-  if (!gImageHash[url][type].hasOwnProperty(alt)) {
>-    gImageHash[url][type][alt] = gImageView.data.length;
>+  if (!gImageHash[url][type].hasOwnProperty(alt) || elem == gImageElement) {
>+    if (gImageHash[url][type].hasOwnProperty(alt)) {
>+      var i = gImageHash[url][type][alt];
>+      gImageView.data[i][COL_IMAGE_COUNT]++;
>+    }

This doesn't work as expected, as gImageView.addRow will be called regardless.

I think all you need to do is to insert this in the last else branch:

if (elem == gImageElement)
  gImageView.data[i][COL_IMAGE_NODE] = elem;

Comment 18

[Dão Gottwald [:dao]]

You should be able to modify browser_bug517902.js to cover this bug by adding something like <img src='about:logo?b' height=200 width=250 alt=2>.

Comment 19

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.2

This adds that last line to the final else branch, and it adds that new image tag to the browser test.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 411252 [details] [diff] [review]
Patch v 1.2

>-  if (!gImageHash[url][type].hasOwnProperty(alt)) {
>+  if (!gImageHash[url][type].hasOwnProperty(alt) || elem == gImageElement) {

because of that change, you'll never reach this:

>   }
>   else {
>     var i = gImageHash[url][type][alt];
>     gImageView.data[i][COL_IMAGE_COUNT]++;
>+    if (elem == gImageElement)
>+      gImageView.data[i][COL_IMAGE_NODE] = elem;
>   }


>   content.location =
>     "data:text/html," +
>     "<img src='about:logo?a' height=200 width=250>" +
>     "<img src='about:logo?b' height=200 width=250 alt=1>" +
>-    "<img src='about:logo?b' height=100 width=150 alt=2 id='test-image'>";
>+    "<img src='about:logo?b' height=100 width=150 alt=2 id='test-image'>" +
>+    "<img src='about:logo?b' height=200 width=250 alt=2>";

This passes even without this bug being fixed, doesn't it?

Comment 21

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.3

Here is a patch that fails the browser-chrome test if this patch is absent, and the new block of code only increments the count value of another image if the selected image is gImageElement and a duplicate of another.  I have tested it, and it returns the correct count totals.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 411283 [details] [diff] [review]
Patch v 1.3

That still populates gImageView.data with a new row. Was there a problem with the second chunk of the previous patch without the first chunk?

Comment 23

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.4

This patch checks if the image is already in the tree, and if it is, the tree entry is updated to point at gImageElement, otherwise a new row is created.

Comment 24

[Dão Gottwald [:dao]]

Was there a problem with the second chunk of attachment 411252 [details] [diff] [review] without the first chunk?

Comment 25

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.5

This simple patch works for all the cases I've seen except for the one in Bug 526721.

I would opt for having this fixed and the other bug reopened.

Comment 26

[Alice0775 White]

(In reply to comment #25)
> Created an attachment (id=411627) [details]
> Patch v 1.5
> 
> This simple patch works for all the cases

In the testcase Created an attachment (id=410276)

>Additional Image that makes the bug more visible:  icon
>No alt text on either image:  icon1  icon2
>Same alt text on each image: icon1  icon2

"Voew Page Info" of icon1 works,
but icon2 does not work.

Comment 27

[Alice0775 White]

Sorry (In reply to comment #26) is spam.
I won a wrong patch.

Comment 28

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2eb351cc47d3

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 411627 [details] [diff] [review]
Patch v 1.5

a192=beltzner, thanks for the test!

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3d2c81810e78

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 411627 [details] [diff] [review]
Patch v 1.5

(whoops, forgot to flip the flag)