Closed Bug 5821 Opened 25 years ago Closed 24 years ago

{compat} Nav4 vs CSS2 line box model

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: cpeterso, Assigned: buster)

References

(
URL
)

Details

(Whiteboard: [NOTESTCASENEEDED])

Attachments

(4 files)

linked images create too big table cells - testcase
595 bytes, text/html
Details
An even simpler example w/o table.
359 bytes, text/html
Details
patch described in comments
1.85 KB, patch
Details | Diff | Splinter Review
New testcase, simplified version of www.citec.fi
419 bytes, text/html
Details
Assignee: rickg → karnaze
Chris -- another table problem. Here's the min. case:

<html>
<body>
  <table width="100%" cellspacing="0" cellpadding="0" border="0">
    <tr>
      <td>
        <table cellspacing="0" cellpadding="0" border="0">
          <tr>
            <td bgcolor="#003366"><img src="/gresources/cleardot.gif" alt="."
width="1" height="3" border="0"></td>
          </tr>
          <tr>
            <td bgcolor="#003366">Books &nbsp;Home</td>
          </tr>
        </table>
      </td>
    </tr>
  </table>
</body>
</html>
Assignee: karnaze → kipp
Here is a simpler example illustrating why the table cell/row is too tall.
During nsTableCellFrame::Reflow() on line 532, the area frame (1st child of
nsTableCellFrame) is returning a desired height bigger than the 3 pixels that it
should be.

<html>
<body>
 <table cellspacing="0" cellpadding="0" border="1">
  <tr>
   <td bgcolor=white><img src="raptor.jpg" width="1" height="3"></td>
  </tr>
</table>
</body>
</html>
Severity: normal → major
Status: NEW → ASSIGNED
Priority: P3 → P5
Summary: barnesandnoble.com's menu bar table is too tall and menu text is not centered. → {compat} barnesandnoble.com's menu bar table is too tall and menu text is not centered.
Target Milestone: M15
Yet another example of a conflict between nav compatability and css2's
line-height and line-layout specification.
Summary: {compat} barnesandnoble.com's menu bar table is too tall and menu text is not centered. → {compat} Nav4 vs CSS2 line box model
This could be fixed by applying vertical-align:bottom to images that are in a
content model containing only images.

So, for example, the following:

   <div>
     <img ...>
     <img ...>
     <img ...>
   </div>

would imply vertical-align:bottom for those images, while the following:

   <div>
     <img ...>
     is good.
   </div>

...would use vertical-align:baseline (the initial value), aligning the images
with the text baseline.

A simpler fix would be achieved by using the following rule in the
compatibility mode ua.css:

   IMG { vertical-align: bottom; }

Of course, that would also cause images that are mixed with text to be slightly
too low, hence the more complex solution given above.

A study of 4.x behaviour may be needed to ascertain the best solution, though.
*** Bug 4769 has been marked as a duplicate of this bug. ***
*** Bug 4376 has been marked as a duplicate of this bug. ***
[ccing dbaron as he was on 4769's cc list]

Created an attachment (id=25)
Series of test cases that were attached to 4769, showing simplified form of
problem and its cause.
*** Bug 5834 has been marked as a duplicate of this bug. ***
*** Bug 7118 has been marked as a duplicate of this bug. ***
Bug 7118 pointed to the small "GO" logo at the top of
   http://abcnews.go.com/
*** Bug 7290 has been marked as a duplicate of this bug. ***
Priority: P5 → P1
*** Bug 5900 has been marked as a duplicate of this bug. ***
Bug 5900 points to the following, which should be checked when fixing and
verifying this bug:

   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=120
   more duplicates: bug 6140, bug 6631, bug 5528, bug 6237, bug 7336
added attachment (testcase originally from #7290),
added me to cc-list.

I hope it can be fixed before M15...
*** Bug 7581 has been marked as a duplicate of this bug. ***
Priority: P1 → P2
KaiRo@StarTrekMail.com: The M15 marker is in place because kipp@netscape.com
is on sabbatical. This compatability issue will likely be addressed when he
returns, in around a month's time.

Also, please note that *this is not a bug*. Mozilla is acting correctly in all
these cases. It is the web pages that are incorrect. Mozilla is rendering the
pages exactly as per the CSS2 specification.

[I just realised I set the priority to P1, which kipp reservers for crashers.
 Oops. Reducing priority to P2.]
*** Bug 7675 has been marked as a duplicate of this bug. ***
*** Bug 7352 has been marked as a duplicate of this bug. ***
*** Bug 8466 has been marked as a duplicate of this bug. ***
*** Bug 8534 has been marked as a duplicate of this bug. ***
You might also want to look at linux.com as per bug 8534 when this bug gets
fixed as an additional test case.
*** Bug 8180 has been marked as a duplicate of this bug. ***
*** Bug 5851 has been marked as a duplicate of this bug. ***
*** Bug 5031 has been marked as a duplicate of this bug. ***
*** Bug 5031 has been marked as a duplicate of this bug. ***
*** Bug 8757 has been marked as a duplicate of this bug. ***
Just wondering what the status is on this puppy getting fixed.
kipp's last comments were on 05/03/99 .. almost 2 months ago.
Fixed? Our current behaviour is correct!

Kipp is currently on sabbatical. I believe he should be back soon. This
compatability issue will likely be addressed when he returns.
Whiteboard: [NOTESTCASENEEDED]
Marking NOTESTCASENEEDED.  The problem is well understood.  The issue is what to
do about it, and what fix would be compatible enough.
*** Bug 9244 has been marked as a duplicate of this bug. ***
*** Bug 9402 has been marked as a duplicate of this bug. ***
*** Bug 9402 has been marked as a duplicate of this bug. ***
Priority: P2 → P5
*** Bug 9481 has been marked as a duplicate of this bug. ***
*** Bug 4677 has been marked as a duplicate of this bug. ***
*** Bug 3016 has been marked as a duplicate of this bug. ***
*** Bug 3016 has been marked as a duplicate of this bug. ***
I was suprised to see that noone reacted on Ian's 05/19/99 comment, so I do
it now. I'm talking about the "vertical-align:bottom" solution, which has
its problems. First of all it does not fix the problem...
It is only producing a looks-like-expected layout, if the line-height is
smaller than the height of the image. Now this is one of the first things
a web designer has to find out not to rely upon.

Something to clarify about this bug:
In current versions (like 1999-07-10-08) there's no such gap when the
content is only an image. There's only a gap when the image is wrapped in
an "empty" inline-element, such as an anchor.
The attachment which was added by Ian is showing that.

The problem is that while we expect the cell to end up as high as the image,
the inline element streches it. If it's big enough it can stretch the cell
regardless of the aligment of the image.

This is indeed a serious issue, because it appears that mozilla not only
"breaks" existing pages, but there's no standard compliant way to
modify such a web page to produce the desired result.

I've studied CSS2 for a few hours (which is not much I can agree) to find out
about this. Here's what seems to apply:

In chapter 10.6.1 Inline, non-replaced elements:
... The 'height' property doesn't apply, but the height of the box is given
by the 'line-height' property.

In chapter 10.8.1 about the calculation of "line-height":
[it should by default to be set] to a "reasonable" value based on the
font size of the element.

In 15.2.4 it says about the font-size:
The _actual_value_ of this property may differ from the _computed_value_ due a
numerical value on 'font-size-adjust' and the unavailability of certain font
sizes.

So if we want to attack this we should argue about what's "resonable" or
that we needs a deviation of the font-size's actual value from the computed
value.

I've programmed some simple text layout program myself and I came up
calculating line-height as the maximum of the line's glyps' ascent plus
the maximum of the line's glyps' descent. Since ascent and descent is
(by definition?) the same in all glyps in a font, so line-height is the maximum
of the line's fonts' ascent plus the maximum of the line's fonts' descent.
What is one may think at first is that an inline element will use only
one font, but that's not actually true. It is possible that different
characters in the element are requiring different fonts because of different
encoding. (NS4 didn't support this but IS4 and mozilla is.) Because the font
inavailibility the font-size may different with each font used, hence the
maximum calculation is needed.
Question is: what is the maximum of the "height of the glyps" (simplified)
if there are no glyps as there are no characters in the element.
The standard say to pick a "resonable" value, which may differ from the
computed one. I'd say pick zero in that case. It's a _resonable_ value,
because it ensures compatibility.

Let's make it clear, I didn't talk about EMPTY element's. I talked about
elements with no characters in them. The element may contain inline elements.
Of course if an element contains no characters, but it contains an inline
element that does the inside element could strech the hight of the outside
but I will investigate that more (both standard-wise and compatibility-wise).

Oh yes, just my $0.02
QA Contact: petersen → chrisd
Depends on: 6865
hyp-x@inf.bme.hu: I disagree. IMHO, the vertical-align:bottom solution _is_ a
satisfactory solution.

You say:
> In current versions (like 1999-07-10-08) there's no such gap when the
> content is only an image. There's only a gap when the image is wrapped in
> an "empty" inline-element, such as an anchor.

This is bug 6865, which I am adding as a dependency.


> It is only producing a looks-like-expected layout, if the line-height is
> smaller than the height of the image.

Which is absolutely correct. Do any of the dozens of dups and test cases for
this bug have a line-height bigger than the images?


Note also that your method of calculating line-height on a per line basis is
all very well, but it is also completely non-spec compliant. The CSS specs are
actually quite thorough in their definition of how to calculate line heights.
*** Bug 2971 has been marked as a duplicate of this bug. ***
*** Bug 11243 has been marked as a duplicate of this bug. ***
*** Bug 11371 has been marked as a duplicate of this bug. ***
Target Milestone: M15 → M10
Kipp just accepted 11371 for M10, which is a duplicate,
so I'm marking this one M10.
*** Bug 11529 has been marked as a duplicate of this bug. ***
*** Bug 12605 has been marked as a duplicate of this bug. ***
*** Bug 12786 has been marked as a duplicate of this bug. ***
*** Bug 5775 has been marked as a duplicate of this bug. ***
*** Bug 12744 has been marked as a duplicate of this bug. ***
*** Bug 7529 has been marked as a duplicate of this bug. ***
*** Bug 13045 has been marked as a duplicate of this bug. ***
Severity: major → critical
Priority: P5 → P2
Target Milestone: M10 → M11

    
    

    
  
One way to fix this would be to (in compat mode only) make inline boxes that do
not have text nodes as *children* have zero height and sit on the text
baseline.  Would that work?

    
    

    
  
David: If bug 6865 is fixed by implementing your anonmyous-inline-around-all-
inlines-in-a-block solution, then yes, doing this would work. (Note that only non-
replaced inline elements should have their heights shrunk in this way -- the
images must still have their height to give the line box a height!)

Kipp: I *strongly* recommend fixing bug 6865 first. Doing so should be relatively
easy: you just have to insert an anonymous inline element around all inline
content in a block. For example, the following:

   <div>
      <span> abc </span>
      <span> def </span>
   </div>

...would internally become

   <div>
      <anonymous-inline>
         <span> abc </span>
         <span> def </span>
      </anonymous-inline>
   </div>

See http://lists.w3.org/Archives/Public/www-style/1999Jan/0027.html for an
explanation of this problem and the proposed solution.

Once this is implemented, then this bug can be fixed completely by implementing
the mechanism that David has just proposed:

   In compat mode only, make non-replaced inline boxes that do not have text
   nodes as children have zero height and sit on the text baseline.

...or, in pseudo-css:

   @compat-mode {
      :inline:contains(:text-node) { line-height: 0; }
   }

I believe this is a complete solution, and does not require any further changes.
It requires _no_ changes to strict mode at all.

    
    

    
  
I started poking around in the code, and I think I may have the beginning of a
fix for this.  It has the following problems:

 * I don't know how to detect compatMode, so I made a variable called compatMode
and set it to PR_TRUE.  It should be initialized correctly (true if quirks,
false if standard).
 * I don't handle ignorable whitespace properly.  This means if there is any
space like <a ... > <img></a> rather than <a ...><img></a> it doesn't work.

A few more notes:
 * It's against a source pull from Saturday
 * I tested it a bit side-by-side with an opt build from Monday.
 * I ran through 5 of my CSS tests (linebox[1-4] and inlinetest) and a number of
the bugs (5-7??) that were resolved as dups of this one.
 * It seemed to fix all but one of the bugs I tested here (the whitespace
issue), except that http://www.linux.com/ and http://www.barnesandnoble.com/
didn't seem to load quite properly in either.  I was getting errors in the debug
build but not the opt build, so it's hard to tell if I caused that.
 * Among the 5 of my tests I tested, the only regression I saw was in linebox2
(middle test, I think).  If this is compat mode only, then I think that's OK.
It's what I would expect to happen...
 * It does need a bit more testing, I think, and of course you should look at it
carefully since I don't completely know what I'm doing (especially C vs. C++
issues like where I should declare the variables - I've never really done much
C++ as opposed to C).
 * So that the diffs are easier to read as diffs, I didn't make an indenting
change in about 50 lines of code that need an additional (2-space) tab of
indenting.

    
    

    
  
Now that I think about it, you might want to make this depended on the inline
element having no padding, border, or margin, and possibly also having
vertical-align of baseline.  It could cause some strange things otherwise.

    
    

    
  
Well a quick test of david's suggested solution (my test was simpler - I always
set the height/ascent/descent to zero :-) lead to the right compatabile
behavior. Ignoring the other issues regarding the spec, this is kinda nice. So
I'm coding up a proper version that takes into account text-frames and
borders/padding/etc....More later.

    
    

    
  
You probably noticed this already, but, just for the record, I noticed that I
used two boolean variables where I should have used one... compatMode and
noTextChildren should be replaced by something like collapseHeight.  I'm not
that bad once I get a chance to look at (really, think about, since I was
thinking about the p/b/m and vertical-align stuff) what I've done :-).

    
    

    
  
Okie dokie. I've tested every regression that this page links to with my coding
of david's suggestion and it *works*. The other case where there was another
problem I reopened.

Yippee... The implementation pays attention to the "dtd mode" and if we are in
compatability mode (most likely currently) then it behaves like nav.

Fix landing shortly.

    
  
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED

    
    

    
  
Changes checked in. Try out tommorows build and let me know...

    
    

    
  
This bug appears to be fixed.  However, verifying this should be a good bit of
work, because we need to check all the test cases in case some of them were not
truly duplicates of this bug or in case some of them weren't fixed by the
current fix.

It would be helpful if anyone who wants to help could go down (starting at the
top, or we'll get way too confused!) and write comments here that you've checked
things out or that you've reopened a bug because there are other problems.

(I'm going to investigate the problems I'm seeing on
http://www.barnesandnoble.com/ and http://www.linux.com/ in a few minutes.  I'll
probably look at some other cases too...)

    
    

    
  
I've started verification from the top:

4769
  The page has a <BODY><P> margin problem, covered in another bug.
4769 -> 5080
  It only has an unrelated table problem (table bgcolor)
  I'll look into this.
4376
  It has a residual style problem (<TABLE> inside <P>) remained.
5834
  The author changed the page to work without the fix.
7118
  It still has some table problems.
  I'll look into this.
7290
  It's rather invalid than a duplicate, because the table cell has an extra
  whitespace in which case both Nav4 and IE4 renders things "correctly".
  (Even so IE4 doesn't render the whitespace itself.)
  I'm leaving the bug alone. (RIP)
5900 is looking good
  It has a table alignment problem down at the applet.
  I have a testcase, I'll file a bug for that (or look-up an existing one.)
5900 -> 6140
  No problems with this site.
5900 -> 6631
  Page no longer exist.
5900 -> 5528
  No problems with this site.
5900 -> 6237
  No problems with this site.
5900 -> 7336
  No problems with this site.

    
  
Status: RESOLVED → VERIFIED

    
    

    
  
7581
  No problems with this site.
7675
  No problems with this site.
7352
  Kipp reopened this bug for another problem.
  I made a testcase.
  This is the same problem I found on 5900, so that bug can be left alone.
8466
  Page no longer exists.
8534
  Dbaron is doing this (www.linux.com)
8180
  No problems with this site.
5851
  No problems with this site.
5031
  No problems with this site.
8757
  Site has a well known residual style problem.
9244
  No problems with this site.
9402
  Dbaron is doing this (www.linux.com)
9481
  No site URL, just testcase. It's OK.
4677
  No problems with this site.
3016
  Site URL is internal, testcases OK
2971
  The site has other bugs (well quirks) already covered elsewhere.
11243
  Page no longer exists.
11371
  No site URL, just testcase. It's OK.
11529
  No site URL, just testcase. It's OK.
12605
  No problems with this site.
12786
  No problems with this site.
5775
  No problems with this site.
5775 -> 8273
  I see an "entity in invalid table area" parsing problem.
  I was just about to file a bug for this, anyway. :)
5775 -> 9692
  No problems with this site.
5775 -> 10009
  There's a table problem on the site.
  I've reopened the bug.
5775 -> 10100
  There's a table problem on the site.
  I'll look into this.
5775 -> 10345
  I see covered residual style problems and the table alignment problem (7352)
5775 -> 10835
  No problems with this site.
12744
  I have a totally unrelated behaviour problem with the site.
  I'll look into it.
7529
  No problems with this site.
13045
  No problems with this site.

Huhh. That should do it.
One more doesn't matter, so I checked www.linux.com too.
I see no problem there.

I'm marking this one verified.

    
    

    
  
hyp-x@inf.bme.hu: Impressive. Nice work. :-)

    
    

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

    
    

    
  
The fix for this bug has bug 13097.

    
    

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

    
  
Target Milestone: M11 → M10

    
    

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

    
    

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

    
    

    
  
This one is back, or not exactly this one but very close and since just about
every bug similar to this one has been marked a dup of this I'm reopening this
bug. The below html shows the new problem, the problem is that there's an about
3 pixel high gap between the two images, if I remove the 'a' that wraps the
first image then the extra space goes away. I first noticed this on
http://www.citec.fi.

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<html>
<body>
<table border="0" cellpadding="0" cellspacing="0">
<tr> 
 <td valign="bottom">
  <a href="http://www.mozilla.org">
   <img src="http://www.mozilla.org/images/mozilla-banner.gif"
    border="0"></a></td>
</tr>
<tr>
 <td valign="top">
  <img src="http://www.mozilla.org/images/mozilla-banner.gif"
   border="0">
 </td>
</tr>
</table>
</body>
</html>

dbaron, I verified that this problem is caused by the modifications you did
on 02/14/2000 to nsLineLayout, could you have a look at that, backing out those
changes makes the testcase look like it does in NS4.7.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

    
    

    
  
I'm sure I tested a bunch of the testcases for this bug, but now I realize it
was probably not with the final version of the code that I checked in (since I
found and fixed a few bugs in it, and went through most, but not all, of the
testing over again).

Steve - do you think I should back the changes out?  I may have a chance to look
at this this evening, but I won't have all that much time.  Do you know what the
schedule for M14 is?


    
    

    
  
Right now this problem is happening within table cells, but not within divs.  I
don't know why that is.  I don't think there's something strange about how
things work inside of table cells, but perhaps there is.  I suspect it's the
same thing as what's causing bug 15933.

    
    

    
  
Hey, my testcase I made in the good old days for #7290 (a dupe) - the testcase
ist the one included here called "linked images..." - behaves strange:

When it replaces the image with the string "testpic1" because it can't find the
image, the page reflows and the table height gets correct. If I copy it to a
local directory and put an image called "testpic1.gif" there, it shows the
picture and the table height is false.

Is there any conflict between quirks and strict mode? I assumed from this bug
discussion that for some strange reason the bigger cells are right in strict
mode but they are surely false in quirks mode...


    
    

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

    
    

    
  
OK... I've refixed this.  I'd like to test a bit more before I check in. 
However, I'm posting the patch for review now because I'm pretty sure it's
right, and I'd like to get a code review (and then approval) as soon as possible
so this can get fixed again.

The problem was that I was making the wrong comparison when seeing if the bottom
edge of a span needed to be shrunk.  The code was wrong when the span's children
extended out of it on top, but did not reach its edge on the bottom.  The point
where the top of the span's box should be is 0, so maxY is the lowest bottom
among the span's children, and minY is the highest top.  The bottom of the span
needs to be shrunk if its height is bigger than maxY, not when it's bigger than
(maxY-minY), since 0 is the top of the span.  I'll save you the explantion of
why I think I wrote what I did...

    
    

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

    
    

    
  
(Re)fix checked in 2000-02-18 19:42-0800.

Changing TM M10 -> M14.
Status: REOPENED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED
Target Milestone: M10 → M14

    
    

    
  
Verified this on both WinNT and Linux, things look very good now, way to go
David!

Status: RESOLVED → VERIFIED

    
    

    
  
This one's back, se the new attachment. Reopening.

Status: VERIFIED → REOPENED
Resolution: FIXED → ---

    
    

    
  
jst - Are you sure you're not seeing bug 36080 (transitional DTD triggers strict
mode) instead?

    
    

    
  
Duh, that's exactly what I'm seeing, thank's for bringing this to my attention.

Changing back to fixed. Sorry for the spam.
Status: REOPENED → RESOLVED
Closed: 25 years ago → 24 years ago
Resolution: --- → FIXED

    
    

    
  
... and verified.
Status: RESOLVED → VERIFIED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: