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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Loco]

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]

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]

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)]

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)]

Buster, this should probably rtm+, right?

Comment 5

[buster]

I suggest we get this fixed for rtm, but not hold beta3 for it.

Comment 6

[Loco]

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]

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)]

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)]

Adding rtm+.

Comment 10

[Phil Peterson]

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]

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)]

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]

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]

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]

Attachment: proposed patch

Comment 16

[Marc Attinasi]

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]

Attachment: 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]

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]

If the above change is indeed correct (buster, karnaze, sanity check me there),
then make it, and a=waterson

Comment 20

[buster]

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]

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]

fix checked into branch

Comment 23

[buster]

fix now checked into trunk as well.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What is the bug number for percentage heights within tables?

Comment 25

[buster]

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]

Sure enoughlooks beautiful:).  Marking VERIFIED.

Comment 27

[Loco]

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]

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]

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]

*** Bug 56577 has been marked as a duplicate of this bug. ***

Comment 31

[Asa Dotzler [:asa]]

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)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.