Closed
Bug 198346
Opened 21 years ago
Closed 20 years ago
(XUL reflow problem) site icon and image input fail to render in page info, media tab
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect, P3)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
VERIFIED
FIXED
Future
People
(Reporter: db48x, Assigned: bzbarsky)
References
Details
(Keywords: helpwanted)
Attachments
(2 files, 2 obsolete files)
1.69 KB,
patch
|
Details | Diff | Splinter Review | |
5.13 KB,
patch
|
Details | Diff | Splinter Review |
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 :)
Assignee | ||
Comment 2•21 years ago
|
||
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; aState.GetScrolledBlockSizeConstraint(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.....
Assignee | ||
Comment 3•21 years ago
|
||
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...
Assignee | ||
Comment 4•21 years ago
|
||
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 sucking....
Assignee | ||
Comment 5•21 years ago
|
||
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. ;)
Reporter | ||
Comment 6•21 years ago
|
||
uses bz's method which works just fine (and doesn't hurt the physical width stuff) 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 patch.
Attachment #117962 -
Attachment is obsolete: true
Reporter | ||
Comment 7•21 years ago
|
||
Comment on attachment 117976 [details] [diff] [review] patch 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 8•21 years ago
|
||
Comment on attachment 117976 [details] [diff] [review] patch s/foo/imageLoading/ ? And "doh" on the missing return from getContentTypeFromHeaders.
Attachment #117976 -
Flags: review?(jaggernaut) → review+
Reporter | ||
Comment 9•21 years ago
|
||
oops, forgot about the foo varible
Attachment #117976 -
Attachment is obsolete: true
Assignee | ||
Comment 10•21 years ago
|
||
Comment on attachment 117976 [details] [diff] [review] patch 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-
Reporter | ||
Comment 11•21 years ago
|
||
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?
Reporter | ||
Comment 12•21 years ago
|
||
oh, and yes. that other patch without those four lines would be acceptable to me.
Reporter | ||
Comment 13•21 years ago
|
||
http://db48x.hypermart.net/pageInfo/tests/imagesize.html is a good place to test out the resize weirdness
Reporter | ||
Comment 14•21 years ago
|
||
*** Bug 198838 has been marked as a duplicate of this bug. ***
Comment 15•21 years ago
|
||
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 http://www3.telus.net/ljs/html/gardens.htm 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?
Assignee | ||
Comment 16•21 years ago
|
||
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
Assignee | ||
Comment 17•21 years ago
|
||
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
Comment 18•20 years ago
|
||
This bug is still in 1.7 and it looks like it affects type=background too.
Assignee | ||
Updated•20 years ago
|
Keywords: helpwanted
Comment 19•20 years ago
|
||
<http://www.w3.org/WAI/UA/TS/html401/cp0203/0203-INPUT-ALT.html> exhibits this problem in 1.7.1. Will test latest nightly tomorrow morning.
Assignee | ||
Comment 20•20 years ago
|
||
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 appreciated.
Comment 21•20 years ago
|
||
Indeed, testing with the latest nightly (20040902), it seems fixed.
Assignee | ||
Comment 22•20 years ago
|
||
Good to hear! Marking verified based on that.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•