519390 - Page info sometimes resizes an image incorrectly when it uses scaling

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[Tanner M. Young [:tmyoung]]

1. Go to http://alyoung.com/ and open the Media Tab in Page Info.
2. Select "http://alyoung.com/News/RSS_Feeds/RSSLogo.jpg"

Results: The image has its natural width and its scaled height.

I looked through the code on the makePreview(row) function and experimented with it, but all of the values it has are correct, and I'm not sure what function controls the height and width after the image is passed to imageContainer.appendChild(newImage);

I'm glad to work on this, but I don't know where to go from here.

Comment 1

[Justin Dolske [:Dolske]]

Images in XUL tend to do this if you're not careful, because the image will stretch to fill it's box -- and if the box gets sizes/flex differently than the images aspect ration it's stretched.

I'd poke around with DOM inspector to see what the actual sizes of things in the windows end up as. I'd randomly guess there's some unexpected interaction with a min-size and flex.

Comment 2

[Philip Chee]

Perhaps instead of using |var newImage = new Image();| you could try using createElementNS() to force the image into the xhtml namespace.

Comment 3

[Tanner M. Young [:tmyoung]]

I tried createElementNS() and various elements and it didn't change anything, but I think moving to using that would be good.

I have found that if the following code block is disabled and the lower code block used instead:

933       newImage.width = ("width" in item && item.width) || newImage.naturalWidth;
934       newImage.height = ("height" in item && item.height) || newImage.naturalHeight;

939       newImage.width = newImage.naturalWidth;
940       newImage.height = newImage.naturalHeight;

the problem goes away, but that prevents the browser form detecting if an image has been scaled.

Also, if I create a text block like this, it doesn't fix the problem, which I would think would, because it would force both attributes to be present before using the scaled image:

if (!isBG) {
  if (("width" in item && item.width) && ("height" in item && item.height)) {
    newImage.width = ("width" in item && item.width);
    newImage.height = ("height" in item && item.height);
  }
  else {
    newImage.width = newImage.naturalWidth;
    newImage.height = newImage.naturalHeight;
  }
}
else {
  newImage.width = newImage.naturalWidth;
  newImage.height = newImage.naturalHeight;
}

Comment 4

[Tanner M. Young [:tmyoung]]

Using the DOM inspector, the width value that is passed to "imageContainer.appendChild(newImage);" is the correct value, but for some reason when it is loaded all values for width (width, clientWidth, etc.) list the naturalWidth.

Therefore, the problem is in whatever events the appenChild function calls.  Where would that code be?

Comment 5

[Tanner M. Young [:tmyoung]]

I'll go ahead and assign this to me.

Comment 6

[Tanner M. Young [:tmyoung]]

Attachment: Fix that uses HTML properties and XUL properties

I have filed Bug 522850, which is for the XUL:image vs. HTML:img problem.

Here is a potential workaround for the problem which uses the HTML elements to find the resize values (which XUL lacks) which providing a XUL image with the correct implementation of the width property.

If Bug 522850 is fixed, it will probably fix this bug, so I don't know if we want this patch or not.  Please provide any suggestions.

Comment 7

[Justin Dolske [:Dolske]]

I'm curious, having poked at this too, why this is happening. Seems like it's either a bug with sticking an xhtml:img into XUL, or yet another detail of how XUL does sizing that I don't grok.

Comment 8

[Dão Gottwald [:dao]]

Since this is a minor glitch, I'd rather not add a front-end workaround, unless it's clear that the core bug can't be fixed in the foreseeable future.

Is this a regression, by the way?

Comment 9

[Tanner M. Young [:tmyoung]]

(In reply to comment #8)
> Since this is a minor glitch, I'd rather not add a front-end workaround, unless
> it's clear that the core bug can't be fixed in the foreseeable future.

I understand.  Though nobody has respond to my other bug, which surprises me; I'll see what I can find out.

> Is this a regression, by the way?

Not as far as I am aware: users in #developers say it also appears in 3.0 and 3.5.

Comment 10

[Neil Deakin]

Is there a reason not to just use a xul image element instead? The bug is likely some block to box issue which is probably hard to fix.

Comment 11

[Neil Deakin]

Does setting <html:img style="min-width: 1px;"> work?

Comment 12

[Tanner M. Young [:tmyoung]]

(In reply to comment #10)
> Is there a reason not to just use a xul image element instead? The bug is
> likely some block to box issue which is probably hard to fix.

Yes, page scaling breaks because XUL has no property that I known of that determines what an image's natural dimensions are, so the patch gets those elements from the image and then changes the image type to a XUL image.

> Does setting <html:img style="min-width: 1px;"> work?

Yes it works for testcases in Bug 522850, but I can't get it to apply to this context since it is a script; whenever I do what should apply it using js, it does not affect it.  It is possible that I am using the wrong syntax for it though.

Comment 13

[Tanner M. Young [:tmyoung]]

I also have tried applying min-width in something like the page-info stylesheet, but that has no affect either, unless I am again using the wrong syntax.

Comment 14

[Justin Dolske [:Dolske]]

(In reply to comment #8)
> Since this is a minor glitch, I'd rather not add a front-end workaround, unless
> it's clear that the core bug can't be fixed in the foreseeable future.

It is a workaround, using a xul:image seems like a perfectly natural solution anyway.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #13)
> I also have tried applying min-width in something like the page-info
> stylesheet, but that has no affect either, unless I am again using the wrong
> syntax.

If you publish your code, we can help you figure that out.

Comment 16

[Tanner M. Young [:tmyoung]]

Attachment: Alternate patch using an html:img

I was able to get it to work using js/css.

I think the question now is, do we want to use a xul:image or an html:image, and then we select the patch accordingly.

Comment 17

[Dão Gottwald [:dao]]

You can access #thepreviewimage from <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.css>, can't you?

Comment 18

[Tanner M. Young [:tmyoung]]

Attachment: pageInfo.css solution that uses html:img

(In reply to comment #17)
> You can access #thepreviewimage from
> <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.css>,
> can't you?

That would be smart ;). It works, and I prefer it to doing it image-by-image like in the last patch.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 407473 [details] [diff] [review]
pageInfo.css solution that uses html:img

please add a comment with a reference to bug 522850

Comment 20

[Tanner M. Young [:tmyoung]]

Attachment: Final patch

Made those changes, now the patch needs to be checked in.

Dao: Thank you!

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4cd18988a7ca

Comment 22

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

Comment on attachment 407568 [details] [diff] [review]
Final patch

a192=beltzner

Comment 23

[Tanner M. Young [:tmyoung]]

Checkin needed on 1.9.2

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/85fb29b6da87

Comment 25

[Tanner M. Young [:tmyoung]]

Verified Fixed.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091031 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091031042521

Comment 26

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

Comment on attachment 407568 [details] [diff] [review]
Final patch

a192=beltzner, thanks for filing the followup