Closed Bug 198346 Opened 22 years ago Closed 20 years ago

(XUL reflow problem) site icon and image input fail to render in page info, media tab


(Core :: Layout: Images, Video, and HTML Frames, defect, P3)






(Reporter: db48x, Assigned: bzbarsky)



(Keywords: helpwanted)


(2 files, 2 obsolete files)

Page Info attempts to create a |new Image()|, assign it the url of the icon
(included in the page via <link rel="icon"> or <input type="image">) and then
display it in the preview area. Unfortunatly, the image does not show up,
because it's computed width and height are 0px, instead of the correct dimensions.

Anyway, bz says he's got it pretty much narrowed down :)
blocks bug 83774
Blocks: 83774
So... some investigation shows that the image is sized 0x0.  This happens
because when the image node is stuck in the DOM the BoxToBlockAdaptor reflows it
with a computed height and width of 0.  _That_ happens because it defaults to
using the current rect for the computed size.  Then it adjusts that to
unconstrained, but _only_ when it can't compute the MEW, which only happens when
the reflow _it_ gets is not incremental.  In our case, the reflow for the image
is initial, but the reflow for the adaptor is in fact incremental, so...

I tried the following patch to the adaptor's RefreshSizeCache method:

    if (useHTMLConstraints) {
      nsSize constrained;
      rect.width = constrained.width;
      rect.height = constrained.height;
+   } else if (reason == eReflowReason_Initial) {
+     // Initial unconstrained reflow; using the oldRect for sizing
+     // here is silly
+     rect.width = NS_UNCONSTRAINEDSIZE;
+     rect.height = NS_UNCONSTRAINEDSIZE;

This gives me an unconstrained reflow, then I return a desired size of 224x224,
then the frame gets as SetRect for that size, then the box gets a SetBounds for
224x0 and sets that as the rect on the frame.  Now the second reflow comes
around, and the 224x0 size is treated as the computed size.  Oops.

I'm still trying to figure out how the image frame dealt with this before my
changes.... I'm pretty sure the above change to the adaptor is needed, though. 
Not sure what happens to cause SetBounds() to be called with a height-0 rect.....
Oh, and the more I think about it, the more I suspect that the old code just got
an initial reflow before the image had loaded and then triggered a dirty reflow
when the image loaded... 
Well, I found why this used to work.  Forget my exotic theories about reflow. 
The old code in nsHTMLImageElement (for the case when the element is not in the
document) set the width and height attrs as soon as the image size was known so
that .width and .height would work.  My change made .width and .height just look
at the mCurrentRequest, so the attrs are not set any longer.

A trivial fix for page info would be to attach a load listener to the node and
onload to do:

this.width = this.width;
this.height = this.height;

(I kid you not).

That fixes this bug, though obviously not the problem with nsBoxToBlockAdaptor
Attached patch possible workaround (obsolete) — Splinter Review
Not great, since it overrides whatever width/height were set in the page with
the intrinsic ones.  But this general approach would work....

This is just a workaround around the XUL bug for 1.4a if people want it. 
Please don't resolve this bug just because this patch or one like it lands.  ;)
Attached patch patch (obsolete) — Splinter Review
uses bz's method which works just fine (and doesn't hurt the physical width
I was just going to set the style width and height attributes and let it reflow
again or whatever, but this works perfectly.

The second chunk fixes a strict js warning I accidentally added with the last
Attachment #117962 - Attachment is obsolete: true
Comment on attachment 117976 [details] [diff] [review]

requesting r= and sr= for the workaround, so that at least page info isn't
broken in 1.4a
Attachment #117976 - Flags: superreview?(bzbarsky)
Attachment #117976 - Flags: review?(jaggernaut)
Comment on attachment 117976 [details] [diff] [review]

s/foo/imageLoading/ ?

And "doh" on the missing return from getContentTypeFromHeaders.
Attachment #117976 - Flags: review?(jaggernaut) → review+
Attached patch patchSplinter Review
oops, forgot about the foo varible
Attachment #117976 - Attachment is obsolete: true
Comment on attachment 117976 [details] [diff] [review]

What I meant is that this patch makes the following code completely irrelevant:

     if ("width" in item && item.width)
       newImage.width = item.width;
     if ("height" in item && item.height)
       newImage.height = item.height;

If that is the intent, then just remove that code.  If that is not the intent,
this patch needs work.
Attachment #117976 - Flags: superreview?(bzbarsky) → superreview-
Attached patch experimentSplinter Review
ok, so I'm fiddling with things, and decide to change it around just a little.
Along the way I notice that the image in the preview window is not being
displayed at the size specified in the html, like I wanted. Well, in attempting
to correct this, I discovered that changing the width and the height of the
image to the values I want doesn't work, unless there's an alert just before it
happens. It doesn't even matter what's in the alert, as you can see from the
patch. Any ideas?
oh, and yes. that other patch without those four lines would be acceptable to me. is a good place to test
out the resize weirdness
*** Bug 198838 has been marked as a duplicate of this bug. ***
So I am wondering if the problem I am seeing on a simple web page I made is
related to this bug.  I tracked the start of my problem to some checkin between
March 18th and 19th.  The page at
exhibits the problem.  Basically, I load some XML and create a list of photos. 
When you choose a photo name from the list it should show the photo and
descriptions in the content area using .innerHTML.  However, only the
description is displayed, the image is not.  When I Select All and do View
Selection Source, the image shows as being present in the page - it just is not
displayed.  I admit I am a novice so it is distinctly possible that I am doing
something wrong, but it did work prior to the 19th.  If it turns out to be the
same bug is there any chance that it could get a higher prioriy?
That's bug 198989.  Patch is awaiting reviews.  Pretty unrelated to this bug
(except insofar as both involve images).
Summary: site icon and image input fail to render in page info, media tab → (XUL reflow problem) site icon and image input fail to render in page info, media tab
I won't have time to work on the XUL reflow issue in the near future.  Daniel,
you may want to take this bug if you plan to work on a pageinfo-specific fix; if
so, I will file a separate bug for the XUL problem.
Priority: -- → P3
Target Milestone: --- → Future
This bug is still in 1.7 and it looks like it affects type=background too.
Keywords: helpwanted
exhibits this problem in 1.7.1.  Will test latest nightly
tomorrow morning.
This should be fixed on trunk by the change in bug 255270 -- that fixed the "can
compute MEW" path for incremental reflows to work right.

Jonathan, if you can still test a trunk build to verify, that would be much
Closed: 20 years ago
Depends on: 255270
Resolution: --- → FIXED
Indeed, testing with the latest nightly (20040902), it seems fixed.
Good to hear!  Marking verified based on that.
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.