Last Comment Bug 54119 - percentage (%) image (img) heights ignore height attribute
: percentage (%) image (img) heights ignore height attribute
Status: VERIFIED FIXED
[rtm++] r=karnaze and attinasi, a=wat...
: testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P1 major (vote)
: ---
Assigned To: buster
: Loco
Mentors:
http://slip/projects/marvin/html/img_...
: 56577 (view as bug list)
Depends on:
Blocks: html4.01 55874
  Show dependency treegraph
 
Reported: 2000-09-25 19:26 PDT by Loco
Modified: 2001-09-05 10:39 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (2.46 KB, patch)
2000-10-05 15:49 PDT, buster
no flags Details | Diff | Review
same patch but with a quick check for compatibility mode, to make sure we don't break CSS2 rules in standard mode. (3.10 KB, patch)
2000-10-05 16:22 PDT, buster
no flags Details | Diff | Review

Description Loco 2000-09-25 19:26:00 PDT
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.
Comment 1 Loco 2000-09-25 19:31:29 PDT
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.
Comment 2 Peter Trudelle 2000-09-26 22:31:11 PDT
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 ekrock's old account (dead) 2000-09-27 09:21:03 PDT
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.
Comment 4 karnaze (gone) 2000-09-27 11:08:46 PDT
Buster, this should probably rtm+, right?
Comment 5 buster 2000-09-27 12:00:25 PDT
I suggest we get this fixed for rtm, but not hold beta3 for it.
Comment 6 Loco 2000-09-27 20:35:19 PDT
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?
Comment 7 buster 2000-09-27 21:44:24 PDT
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 ekrock's old account (dead) 2000-09-27 21:48:59 PDT
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.
Comment 9 karnaze (gone) 2000-10-02 12:02:08 PDT
Adding rtm+.
Comment 10 Phil Peterson 2000-10-03 17:28:32 PDT
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.
Comment 11 buster 2000-10-04 00:00:47 PDT
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 ekrock's old account (dead) 2000-10-04 15:59:18 PDT
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?
Comment 13 buster 2000-10-05 15:43:13 PDT
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.
Comment 14 buster 2000-10-05 15:45:37 PDT
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.
Comment 15 buster 2000-10-05 15:49:58 PDT
Created attachment 16268 [details] [diff] [review]
proposed patch
Comment 16 Marc Attinasi 2000-10-05 16:15:10 PDT
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
Comment 17 buster 2000-10-05 16:22:04 PDT
Created attachment 16272 [details] [diff] [review]
same patch but with a quick check for compatibility mode, to make sure we don't break CSS2 rules in standard mode.
Comment 18 Chris Waterson 2000-10-05 20:37:03 PDT
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 Chris Waterson 2000-10-06 09:52:41 PDT
If the above change is indeed correct (buster, karnaze, sanity check me there),
then make it, and a=waterson
Comment 20 buster 2000-10-06 09:59:21 PDT
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.
Comment 21 Phil Peterson 2000-10-06 16:12:51 PDT
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.
Comment 22 buster 2000-10-09 01:05:12 PDT
fix checked into branch
Comment 23 buster 2000-10-09 14:20:40 PDT
fix now checked into trunk as well.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-10-09 14:25:10 PDT
What is the bug number for percentage heights within tables?
Comment 25 buster 2000-10-09 14:46:27 PDT
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.
Comment 26 Loco 2000-10-09 17:12:32 PDT
Sure enoughlooks beautiful:).  Marking VERIFIED.
Comment 27 Loco 2000-10-10 18:11:13 PDT
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.
Comment 28 buster 2000-10-10 22:30:50 PDT
there was no reason to reopen this bug.  bug 55874 covers all portions of the
fix that are not in the code now.
Comment 29 Loco 2000-10-11 00:56:33 PDT
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
Comment 30 Loco 2000-10-16 14:42:42 PDT
*** Bug 56577 has been marked as a duplicate of this bug. ***
Comment 31 Asa Dotzler [:asa] 2000-10-18 17:05:35 PDT
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
Comment 32 Heikki Toivonen (remove -bugzilla when emailing directly) 2001-09-05 10:39:44 PDT
SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Note You need to log in before you can comment on or make changes to this bug.