Page info sometimes resizes an image incorrectly when it uses scaling

VERIFIED FIXED in Firefox 3.7a1

Status

()

VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mozilla.bugs, Assigned: mozilla.bugs)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 3.7a1
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

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

Comment 3

9 years ago
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;
}
(Assignee)

Comment 4

9 years ago
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?
(Assignee)

Comment 5

9 years ago
I'll go ahead and assign this to me.
Assignee: nobody → mozilla.bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Depends on: 522850
(Assignee)

Comment 6

9 years ago
Created attachment 406852 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Attachment #406852 - Flags: review?(dao)
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.
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?
(Assignee)

Comment 9

9 years ago
(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.
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.
Does setting <html:img style="min-width: 1px;"> work?
(Assignee)

Comment 12

9 years ago
(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.
(Assignee)

Comment 13

9 years ago
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.
(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.
(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.
(Assignee)

Comment 16

9 years ago
Created attachment 407471 [details] [diff] [review]
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.
Attachment #407471 - Flags: review?(dao)
(Assignee)

Comment 18

9 years ago
Created attachment 407473 [details] [diff] [review]
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.
Attachment #407471 - Attachment is obsolete: true
Attachment #407473 - Flags: review?(dao)
Attachment #407471 - Flags: review?(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
Attachment #407473 - Flags: review?(dao) → review+

Updated

9 years ago
Attachment #406852 - Attachment is obsolete: true
Attachment #406852 - Flags: review?(dao)
(Assignee)

Comment 20

9 years ago
Created attachment 407568 [details] [diff] [review]
Final patch

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

Dao: Thank you!
Attachment #407473 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4cd18988a7ca
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
(Assignee)

Updated

9 years ago
Attachment #407568 - Flags: approval1.9.2?
Comment on attachment 407568 [details] [diff] [review]
Final patch

a192=beltzner
Attachment #407568 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Comment 23

9 years ago
Checkin needed on 1.9.2
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/85fb29b6da87
status1.9.2: --- → final-fixed
Keywords: checkin-needed
(Assignee)

Comment 25

9 years ago
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
Status: RESOLVED → VERIFIED
Comment on attachment 407568 [details] [diff] [review]
Final patch

a192=beltzner, thanks for filing the followup

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.