Closed
Bug 54119
Opened 24 years ago
Closed 24 years ago
percentage (%) image (img) heights ignore height attribute
Categories
(Core :: Layout, defect, P1)
Core
Layout
Tracking
()
VERIFIED
FIXED
People
(Reporter: bugzillaabuse, Assigned: buster)
References
()
Details
(Keywords: testcase, Whiteboard: [rtm++] r=karnaze and attinasi, a=waterson)
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Comment 2•24 years ago
|
||
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.
Comment 3•24 years ago
|
||
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
Comment 4•24 years ago
|
||
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++.
Comment 8•24 years ago
|
||
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.
Updated•24 years ago
|
Comment 10•24 years ago
|
||
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-]
Assignee | ||
Comment 11•24 years ago
|
||
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.
Comment 12•24 years ago
|
||
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?
Assignee | ||
Comment 13•24 years ago
|
||
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]
Assignee | ||
Comment 14•24 years ago
|
||
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.
Assignee | ||
Comment 15•24 years ago
|
||
Comment 16•24 years ago
|
||
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
Assignee | ||
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
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. :-)
Comment 19•24 years ago
|
||
If the above change is indeed correct (buster, karnaze, sanity check me there),
then make it, and a=waterson
Assignee | ||
Comment 20•24 years ago
|
||
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
Comment 21•24 years ago
|
||
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
Assignee | ||
Comment 22•24 years ago
|
||
fix checked into branch
Assignee | ||
Comment 23•24 years ago
|
||
fix now checked into trunk as well.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 24•24 years ago
|
||
What is the bug number for percentage heights within tables?
Assignee | ||
Comment 25•24 years ago
|
||
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
Reporter | ||
Comment 26•24 years ago
|
||
Sure enoughlooks beautiful:). Marking VERIFIED.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Reporter | ||
Comment 27•24 years ago
|
||
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 → ---
Assignee | ||
Comment 28•24 years ago
|
||
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 ago → 24 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 29•24 years ago
|
||
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
Reporter | ||
Comment 30•24 years ago
|
||
*** Bug 56577 has been marked as a duplicate of this bug. ***
Comment 31•24 years ago
|
||
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.
Description
•