Closed Bug 85016 Opened 24 years ago Closed 24 years ago

[FIX]WRMB:elements (such as images) have zero dimensions when percentages are used on page with no DOCTYPE specified

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: apardo, Assigned: attinasi)

References

(
URL
)

Details

(Keywords: topembed, Whiteboard: PDT+)

Attachments

(7 files, 1 obsolete file)

Testcase for determining what IE is doing in a few situations
1.05 KB, text/html
Details
Patch: Reworked CalcQuirkContainingBlockHeight to allow nesting of unconstrained blocks for IE compatibility
1.55 KB, patch
karnaze
: review+
waterson
: superreview+
Details | Diff | Splinter Review
Additional patch with some DEBUG code to make sure the BODY and HTML elements are correctly identified
3.00 KB, patch
Details | Diff | Splinter Review
New patch (against the 092 branch): this restricst the search for a non-auto containing clock to the specific case where it is really needed, to improve performance.
3.98 KB, patch
Details | Diff | Splinter Review
New Patch (against trunk): restrict the previous patch to the specific case where the extra work is needed
1.96 KB, patch
karnaze
: review+
Details | Diff | Splinter Review
Updated patch addressing waterson's comments
3.91 KB, patch
waterson
: superreview+
Details | Diff | Splinter Review
testcase that doesn't use bugscape
181 bytes, text/html
Details
The texto of this page has a GIF image, that is not seen in the display of the page.
confirmed win98 build:2001060820
The image is sized to 'width="95%" height="65%"' Unfortunately, the containing block for this image contains _just_ the image. So it's sized to 65% of auto, auto defaults to 0, and the image comes up zero- height. setting qa to hixie for verification, but this looks like a mistake in the HTML code to me.
QA Contact: doronr → ian
since there's no DOCTYPE, shouldn't this be rendered in Quirks mode and thereby render the image (im)properly?
Hmm. True. And this is not 74313 since that's Unix-only....
Keywords: qawanted
Mail from reporter: Yes, your sugestion works: if I delete the size of the image, it is displayed correctly. Also works adding <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> at the top of the code, like other mail sugested me.
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout
Ever confirmed: true
OS: Windows NT → All
Hardware: PC → All
Summary: GIF image not seen → image has zero dimensions when percentages are used on page with no DOCTYPE specified
reassign
Assignee: asa → karnaze
QA Contact: ian → petersen
See bug 85516. It looks like the problem is causes by the combination of a percentage scaled image nested in another block element. The DOCTYPE didn't fix anything with the images in the testcases on bug 85516 using Mozilla 2001062211 on Win2k.
Blocks: 81477
Another example, with a jpg image scaled with percentages in a <p></p> environment. Adding DOCTYPE does not seem to help. http://users.skynet.be/www/todo/etentje.htm (build 0.9.2 on GNU/Linux)
Yes, I would say this problem is related to 85516
Severity: normal → major
Chris: take a look at all of the dependents of bug 81477.
The linked to page appears to be rendering correctly in the latest build (2001071214/WinNT). The example given by TGKe, however, is stilling showing nothing
Taking - see bugscape bug 8794 for testcase (topembed)
Assignee: karnaze → attinasi
Keywords: qawanted → topembed
Attached file reduced testcase (for debugging) (obsolete) —
Bug 85516 is a dup of this, and has some good testcases
Status: NEW → ASSIGNED
*** Bug 85516 has been marked as a duplicate of this bug. ***
Why is the bug with "some good testcases" marked as a duplicate of one with almost none? Additionally, note that bug 85516 shows that the DOCTYPE has no effect on the bug. I ran one of the testcases on bug 85516 through the HTML validator at W3C.org, and it came out clean, but it still doesn't display correctly.
> Why is the bug with "some good testcases" marked as a duplicate of one with > almost none? They are dups. It does not matter which one is dup'd to which - they are linked by the duplication process, and after a fix all of the dups have to be checked anyway. In this case I just dup'd the later one to the earlier one, but it doesn't really matter (to me at least - let it be know if you feel differently).
Marc -- I am bumping up priority on this one (see bugscape 8794)
Priority: -- → P2
Summary: image has zero dimensions when percentages are used on page with no DOCTYPE specified → WRMB:image has zero dimensions when percentages are used on page with no DOCTYPE specified
OK, I'm also assigning a milestone, just to be clear on the target.
Target Milestone: --- → mozilla0.9.4
So my current thinking is that a container with ONLY percentage-height children has to get sized, in quirks mode, to 100% of it's container (and if that is the body, 100% of the viewport). This seems to be IE's behavior anyway. Any thoughts? I'll attach a testcase in a bit...
Further refinement, based on some more testing in IE: It looks like an IMG with a percentage height in a block with unconstrained height gets its computed height basis from the closest containing block that has a height (rather than the immediate containing block, which has no height). For example: <div style="border: 1px solid red;"> <img height="100%" src="http://home.netscape.com/ex/shak/home2001c/images/nslogo_t.gif"> <img height="100%" src="http://home.netscape.com/ex/shak/home2001c/images/nslogo_t.gif"> </div> Each image ends up the height of the *viewport*, so the DIV that contains them gets the height of the two images together, or double the viewport height. Tables, however, stop the search for a sized containing block.
Moving to mozilla0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
I attached a patch that tweaks the existing CalcQuirkContainingBlockHeight method to allows for percentage based elements in deeply nested blocks, instead of just elements that are directly in teh body or html element. I discussed the original patch with karnaze and he thought that he did it that way just to handle a special table case. I tested the patch with images and tables, nested in divs and other elements, and our compatibility with IE is better now, and the URL and testcases are fixed.
Summary: WRMB:image has zero dimensions when percentages are used on page with no DOCTYPE specified → [FIX]WRMB:image has zero dimensions when percentages are used on page with no DOCTYPE specified
Blocks: 96795
Comment on attachment 48247 [details] [diff] [review] Patch: Reworked CalcQuirkContainingBlockHeight to allow nesting of unconstrained blocks for IE compatibility r=karnaze
Attachment #48247 - Flags: review+
Comment on attachment 48247 [details] [diff] [review] Patch: Reworked CalcQuirkContainingBlockHeight to allow nesting of unconstrained blocks for IE compatibility forgot to add that r= mandates running table regression tests.
Comment on attachment 48247 [details] [diff] [review] Patch: Reworked CalcQuirkContainingBlockHeight to allow nesting of unconstrained blocks for IE compatibility sr=waterson. Might want to put |continue| statements on a different line so you can set a breakpoint there.
Attachment #48247 - Flags: superreview+
Good suggestion - will do.
table and block regression tests completed and passed.
*** Bug 42543 has been marked as a duplicate of this bug. ***
checked into trunk
Whiteboard: checked into trunk
someting - possibly this - just made loading http://linuxgames.com/ very slow, and also faulty. White "border" images around stories now look bad. Scrollin down in page after it's seemingly loaded, i see the the borders render terribly slow while i watch.
Checking it out - thanks rkaa
The patch does slow down the page at linuxgames.com a little for me, but I am not seeing the rendering problems. Also, I put a break in the code that was altered in the patch, and it is not hit during scrolling around at all. It could be that you were scrolling while the page was still loading, however, and the rendering of all of those images for the borders was competing with the layout, which admitedly does seem slower. From my instrumentation of the modified routine, I see about 3 frame levels being checked for the containing block height on that page, with a few 5-level checks. That routine used to max out at 2 levels, usually 1. This extra traversal will cost somewhat in performance, but it should not be that much. There is also an expensive #ifdef DEBUG that I added that could slow it down too, if you are using a DEBUG build - can you try removing that? It seems to speed it up a little for me. I'd be happy to turn off the DEBUG checks if that help enough...
It looks like we are computing the containing block height even when we don't need it - I think I can hack the patch to avoid the extra traversals if the actual computed height of the 'virtual' containing block is not needed, like when the frame being reflowed has an explicit height.
The layout glitch was a DTD issue as you saw, dbaron filed bug 98977. I build for speed, non-debug, -O2 etc, and the slowdown is noticable using a common P3/500.
I have a new patch for this problem. Basically, I restrict the change I made to a specific case, where we know that the actual computed size of the containing block is really needed. Otherwise, the method does what it always did, and the performance hit is totally mitigated. I'll attach a patch against the trunk hoping that RKAa can test it out.
Patched with attachment 48887 [details] [diff] [review], Linuxgames is fast again and no visible lag when scrolling. Lines are already in place by the time i get to them. (Roamed other sites as well, NOT that they were affected: cnn.com, www.vg.no, ximian, dagbladet.no, wired, pcworld, zdnet, cnet, microsoft: all good and speedy)
Could you make the |aRestrictToFirstLevel| parameter always be explicit instead of using a default parameter (just so nobody wastes time searching through the file for an overloaded version). Also, maybe elaborate a bit (in comments) why one might or might not care to restrict the search to one level or not? I.e., why is it that we don't need to search >1 level when computing the height from ComputeContainingBlockRectangle()?
Comment on attachment 48887 [details] [diff] [review] New Patch (against trunk): restrict the previous patch to the specific case where the extra work is needed r=karnaze
Attachment #48887 - Flags: review+
Chris, I added the comments, and made the argument required. To be honest, I'd like to always do the containing block height computation the same way, and walk up the tree until a real height is found, but this way it restricts the extra walking to the one case where we know we need it - %-height images in unconstrained blocks. Do you (or anyone else) know if we can skip the computation of the containing block height altogether for cases where the contained element does not need it? It seems like a waste to init a value that is not needed. I'd like to play with that when I have some more time.
*** Bug 96505 has been marked as a duplicate of this bug. ***
Comment on attachment 48940 [details] [diff] [review] Updated patch addressing waterson's comments sr=waterson
Comment on attachment 48940 [details] [diff] [review] Updated patch addressing waterson's comments sr=waterson
Attachment #48940 - Flags: superreview+
I don't know when we could skip containing block height. Maybe we ought to just file another bug to investigate that someday.
Bug 99219 opened for future investigation into the possibility of eliminating the computation of the containing block height (or more generally, the size). Trunk patched. Now ready to apply the newest fix to 0.9.2 branch. Also, nominating for 0.9.4 for consideration there.
Keywords: mozilla0.9.4
Checked into 0.9.2 branch, waiting for approval for 0.9.4 branch
Whiteboard: checked into trunk → checked into trunk, 0.9.2 branch
Attachment 44353 [details] on bug 85516 is still breaking on 2001091003-trunk. Should this behavior be affected by the patch, or not? All other testcases for that bug work as expected after the patch was applied.
Does anyone know if bug 96795 is a dupe of this bug?
yes, bug 96795 was a dup of this one, already dup'd - thanks.
*** Bug 99494 has been marked as a duplicate of this bug. ***
Nominating for PDT - shuold get into the eMojo thingy I think.
Whiteboard: checked into trunk, 0.9.2 branch → PDT
*** Bug 99795 has been marked as a duplicate of this bug. ***
*** Bug 81477 has been marked as a duplicate of this bug. ***
expanding summary since dependent bug was duped.
Summary: [FIX]WRMB:image has zero dimensions when percentages are used on page with no DOCTYPE specified → [FIX]WRMB:elements (such as images) have zero dimensions when percentages are used on page with no DOCTYPE specified
Marking nsbranch+. Fix is ready to be checked into 0.9.4 branch
Keywords: nsbranch+
PDT+. Let's get it checked in ASAP.
Whiteboard: PDT → PDT+
Chk'd into 0.9.4 branch - marking FIXED
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Image tag when used with Percentage values don't work for height but it works fine with width. (Ref Bug 84359 is resolved as duplicate of Bug 81477 and Bug 81477 is resolved as duplicate of Bug # 85016 which says its fixed) but the actuall porblem is not solved, Im reopening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
test on build ID 2001-09-19-05-0.9.4 platfrom :windows 2k
Moied, I just checked with 0.9.4 build and it works for me. What testcase or URL are you trying? Didi you try the attachement to this bug at http://bugzilla.mozilla.org/attachment.cgi?id=47687&action=view Tested with todays CVS builds, Win2K and Linux
was the performance fix for linuxgames.com backed out again? The white borders are very slow - noticed some days ago.
RKAa - trunk or 0.9.4 branch? I never backed it out, and it is still in the trunk according to lxr.
current CVS. I deleted all in dist and rebuilt now, then deleted cache: it got faster again. False alarm then. I seem to remember it rendering quicker but testing it now it was acceptable, and much faster than before the rebuild.
Confirming that testcases attached above is working with downloaded build (20010920) as well as my CVS build. Marking FIXED again. If there is still a perceived problem, please indicate the URL or testcase that is failing, and the build it is failing on.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
The testcase is rendered properly using the Sept 25th branch Mac OS X (2001-09-25-05) and Sept 24th branch Linux (2001-09-24-04) build. The image in the testcase fails to appear in the Sept 25th branch or trunk Windows builds.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 46975 [details] reduced testcase (for debugging) testcase is obsolete - see later testcase that uses publicly available image src (thanks basic)
Attachment #46975 - Attachment is obsolete: true
I just tried the new testcase (id=50796) with a brand-new CVS branch build (11:00am 09-25) and it works fine. Petersen, please recheck with tomorrow's build - I am suspecting some unrelated problem (possibly inaccessability of bugscape?).
Verified with the second testcase provided under Windows ME. Tested on branch build (2001-09-27-05).
Keywords: vtrunk
Based on petersen's comments and my own findings, marking FIXED
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Verified on Oct 22nd trunk builds.
Status: RESOLVED → VERIFIED
Removed vtrunk
Keywords: vtrunk
according to the testcase of 47687, for an image with 100% percent height which is inside a <div> with no css height, in strict mode, img use its original height, in quirk mode, img use viewport's height. for an image with 100% percent height which is inside a <td> with no css height, in strict mode, img use its original height, in quirk mode, img's height is set to zero. if replace img with iframe, how it should like?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: