Closed Bug 54119 Opened 24 years ago Closed 24 years ago

percentage (%) image (img) heights ignore height attribute

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: bugzillaabuse, Assigned: buster)

References

()

Details

(Keywords: testcase, Whiteboard: [rtm++] r=karnaze and attinasi, a=waterson)

Attachments

(2 files)

BuildID: 2000-09-19-09 (Mac); 2000-09-13-08 (Linux); 2000-09-13-09 (Win98) IMG HEIGHT not functional. Reproducible: Always Steps to Reproduce: 1. Go to aforementioned URL 2. See error. Actual Results: Same image appearing regardless of "HEIGHT =" setting Expected Results: Should behave properly, as WIDTH does now. Writing new bug as suggested.
Related to bug #39901 (WIDTH). Reason for Priority: Will muck up all pages that use this-and that should be quite a lot. Also part of HTML 3.2 spec at least.
Severity: critical → major
Keywords: nsbeta3
cc'ing ekrock for input. Is this severe enough to consider for beta3, or could we live with it until rtm? Doesn't seem like a beta-stopper to me.
Dan, is this a recent regression? If so, please mark regression keyword and increase priority to P2 at least (high profile backward compatibility) or more likely P1 as this will likely affect top 100 sites. If this is a recent regression (and it may well be, since image height in HTML is fairly basic functionality), then we need to carefully evaluate whether this will screw up layout on top 100 sites before letting this slide for nsbeta3, as we could embarrass ourselves in that case due to the ubiquity of images on the web. It would be worth running that search spider Marcell was using (gerardok or paw know about this I think) to see whether image heights with percentages are used on top 100 sites. If on the other hand this is something we just tested for the first time and it's not working (seems unlikely), we could let this slip for nsbeta3 and go through to rtm. Nominating rtm to make sure this potentially-serious bug doesn't fall off radar without more info.
Keywords: rtm
Buster, this should probably rtm+, right?
Assignee: clayton → buster
Priority: P3 → P2
I suggest we get this fixed for rtm, but not hold beta3 for it.
Status: NEW → ASSIGNED
Priority: P2 → P1
Uh, it fails in PR2 actually. Thing is, this was known, but as far as I knew, it was logged under bug 39901. It turns out that 39901 was *specific to width* and no one let me know:). Anyway, I took the suggestion and filed a new one. So no, not a new issue, but I figure code from the fix that went into 39901 could be transposed into this fix, no?
unfortunately, it's not a matter of "what you do for %-width, just do that for %-height." HTML is width-biased, and the code reflects that, so it's slightly asymmetric. In any event, this will be a 1-2 day fix, not much code, but tons of testing will need to be done. I suggest rtm++.
Strongly endorse for RTM+. We're screwing up HTML 3.2 backward compatibility here. Last time I checked, there was a *lot* of HTML 3.2 out there on the web ... ;-> Let's try to get this fixed and checked in on the trunk ASAP and get mozilla community volunteers to help pound on this to make sure we're not regressing something else when we fix it.
Blocks: html4.01
Keywords: testcase
Adding rtm+.
Whiteboard: [rtm+]
PDT marking [rtm-] because we don't see that severity is bad enough to call P1. If it really is breaking a ton of pages, it's [rtm need info] at best since no patch or reviews are available.
Whiteboard: [rtm+] → [rtm-]
I have a partial fix for this that makes images behave properly everywhere except in tables. Making them work in tables would take some time.
Even if we can't fix this in tables, fixing it in the simple case would be a huge step forward. Steve, could you please get review and super-review and set [rtm+] for re-evaluation by PDT for RTM?
clearing rtm- and reverting to [rtm need info] for reconsideration, per ekrock. eric says it's better to get a partial fix in than no fix at all, and the partial fix is very safe. patch to be attached momentarily.
Whiteboard: [rtm-] → [pdt need info]
patch about to be attached: karnaze or marc, please review. waterson, please super-duper-double-dog-dare-ya review. Note: this is a *partial* fix. It enables %-height on images that are _not_ in tables. %-height images in tables still will not get their height set correctly, probably because they are not getting a final reflow after the cell decides it's own height. That problem will have to wait post-rtm.
Attached patch proposed patch (deleted) — Splinter Review
The attached patch looks good to me, although this construct: if (NS_FRAME_REPLACED(NS_CSS_FRAME_TYPE_INLINE) == mFrameType) is not one I've seen before so I rely on you knowing it is the correct way to check for a replaced inline frame (it certainly looks like that is what it is doing). r=attinasi
You don't need the last to "else" clauses in the second part of the patch. You've already set heightUnit to eStyleUnit_Auto. So I'd change it to: if (eCompatibility_NavQuirks == mode) { aContainingBlockHeight = CalcQuirkContainingBlockHeight(*cbrs); } else if (NS_AUTOHEIGHT != cbrs->mComputedHeight) AContainingBlockHeight = cbrs->mComputedHegiht; } Sound right? FWIW, there are plenty of other problems with images in tables. :-)
If the above change is indeed correct (buster, karnaze, sanity check me there), then make it, and a=waterson
marking rtm+ per karnaze's verbal instructions, noting reviews and super-reviewer in whiteboard. recommend rtm++. low risk, high value. wish I had time to make them work in tables, but that's a bigger task.
Whiteboard: [pdt need info] → [rtm+] r=karnaze and attinasi, a=waterson
PDT marking [rtm++] but we meant it before when we said this wasn't high severity, so check in soon since the ++ won't last a long time.
Whiteboard: [rtm+] r=karnaze and attinasi, a=waterson → [rtm++] r=karnaze and attinasi, a=waterson
fix checked into branch
fix now checked into trunk as well.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
What is the bug number for percentage heights within tables?
Blocks: 55874
submitted bug 55874 to track replaced elements with %-height not working in tables. QA: when verifying, please verify IMG and other inline replaced elements in BODY, DIV, P, etc. But do not verify these elements in TABLE. This problem is covered in bug 55874 and won't be fixed for NS6 RTM.
Summary: percentage (%) image (img) heights do nothing → percentage (%) image (img) heights ignore height attribute
Sure enoughlooks beautiful:). Marking VERIFIED.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
It has come to my attention I'm not *really* supposed to verify the bug, but leave the vtrunk KW in there and leave as resolved. I'll fix this now.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
there was no reason to reopen this bug. bug 55874 covers all portions of the fix that are not in the code now.
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
The problem was that I wasn't supposed to verify it without also verifying the trunk. I meant to resolve it again, sorry-I crashed and lost my train of thought. -d
*** Bug 56577 has been marked as a duplicate of this bug. ***
Verified Fixed on trunk builds testcase URL displays sized pics properly linux 101808 RedHat 6.2 win32 101804 NT 4 mac 101804 Mac OS9 Setting bug to Verified and removing vtrunk keyword
Status: RESOLVED → VERIFIED
Keywords: vtrunk
SPAM. HTML Element component deprecated, changing component to Layout. See bug 88132 for details.
Component: HTML Element → Layout
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: