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
(deleted), text/html
Details
An even simpler example w/o table.
(deleted), text/html
Details
patch described in comments
(deleted), patch
Details | Diff | Splinter Review
New testcase, simplified version of www.citec.fi
(deleted), 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
Attached file An even simpler example w/o table. (deleted) —
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.
Attached patch patch described in comments (deleted) — — Splinter Review
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: