Last Comment Bug 232754 - [FIX]negative margins behave strangely inside table
: [FIX]negative margins behave strangely inside table
: regression
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.7alpha
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Jet Villegas (:jet)
Depends on:
Blocks: 194804 198283 217817 229634
  Show dependency treegraph
Reported: 2004-01-31 11:22 PST by Christopher Anderton
Modified: 2004-02-14 12:39 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

reporter's testcase (431 bytes, text/html)
2004-01-31 18:16 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details
Possible patch (1.44 KB, patch)
2004-01-31 20:02 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Better patch (11.32 KB, patch)
2004-01-31 21:08 PST, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
testcase (459 bytes, text/html)
2004-02-02 11:31 PST, Bernd
no flags Details
testcase where our behavior definitely doesn't match IE (568 bytes, text/html)
2004-02-02 20:56 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details
testcase without doctype (576 bytes, text/html)
2004-02-02 21:22 PST, Bernd
no flags Details

Description Christopher Anderton 2004-01-31 11:22:33 PST
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7a) Gecko/20040111 Firebird/0.8.0+

When using the css padding function in cunjuction with a bakground-color style
in tables, Firebird does not respect the specified width. 

Reproducible: Always
Steps to Reproduce:
1. When loading pages with code as in the demo URL.  
Actual Results:  
Rendering errors

Expected Results:  
No rendering errors

Mozillazine forum thread about this:

Comment 1 Brian Earley 2004-01-31 11:38:03 PST
This did not happen on the 0.7 milestone build, but it does happen sometime
after that...

Another example image:

I notice this bug happening in this build:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040128
Comment 2 Brian Earley 2004-01-31 12:29:12 PST
I think this is probably present in all of the latest versions of Gecko, so the
product should probably be "Browser"
Comment 3 Michael Lefevre 2004-01-31 12:54:23 PST
I see the same thing in Mozilla build 2004013008 (win2k), so it's not a Firebird
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 13:32:43 PST
I believe this may be invalid, but the rules for table cells handling overflow
and resizing as needed are pretty wacky....
Comment 5 Michael Lefevre 2004-01-31 15:34:48 PST
I don't know if it's valid or not, but worth noting that (according to the
mozillazine discussion) our rendering behaviour here has changed since 1.6 branched.
Comment 6 David Baron :dbaron: ⌚️UTC-10 2004-01-31 18:16:45 PST
Created attachment 140335 [details]
reporter's testcase
Comment 7 David Baron :dbaron: ⌚️UTC-10 2004-01-31 18:30:07 PST
I still don't see any reason that the gap should be there.

The regression was between 2003-12-08-14-trunk and 2004-01-08-09-trunk.
Comment 8 Brian Earley 2004-01-31 19:04:23 PST

There's also my test case (first post of the thread). I just figured I'd also
add a more complex one for testing (in which the padding error happens within a
div, but where the div is inside a table.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 19:10:42 PST
Well, the behavior changed between 2004-01-05-08 and 2004-01-06-10.  Could be
bug 227819 or bug 226954 (probably the former).
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 19:23:01 PST
Yeah, the behavior changed with bug 227819
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 19:50:50 PST
The relevant change was the change to ComputeBlockBoxData() to call
AdjustComputedWidth() instead of doing it by hand.  The problem is the last part
of AdjustComputedWidth() which does weird things with widths in tables if
paddings or borders are set...
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 20:02:22 PST
Created attachment 140342 [details] [diff] [review]
Possible patch

This fixes the testcase we have here.

I would rather do this than revert the ComputeBlockBoxData() change, since
other callers of this code could well have the same issues...

That said, I just tried removing this code altogether and that does not seem to
regress bug 175455 (which is what the code was added for).  Do we still need
this code?
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 20:03:46 PST
bernd, roc, see the question in comment 12
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 21:08:47 PST
Created attachment 140347 [details] [diff] [review]
Better patch

I've gone ahead and removed the code bug 175455 added.	With dbaron's recent
changes to make mew calculations for incremental reflows work the same way as
they do for initial reflows, it does not seem to be needed.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 21:13:22 PST
Comment on attachment 140347 [details] [diff] [review]
Better patch

Bernd, David, what do you think?
Comment 16 Bernd 2004-01-31 23:19:40 PST
Boris does your patch also fix the issues in bug 198283?
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2004-01-31 23:29:42 PST
It fixes the "right border suddenly not visible if you change value in dropdown"
issues on the first few testcases, yes.  I'm not sure what other issues there
are in that bug, since none of the testcases clearly explain what one should
look for...
Comment 18 Brian Earley 2004-02-01 13:30:20 PST
Any chance of getting this checked in before the 0.8 Firebird milestone?
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2004-02-01 13:40:16 PST
The changes in question happened after FB 0.8 branched.  So I very much doubt FB
0.8 is affected unless someone royally screwed up merging to the FB branch.
Comment 20 Bernd 2004-02-02 11:31:08 PST
Comment on attachment 140347 [details] [diff] [review]
Better patch

the purpose of this stuff is to avoid that 100% divs with borders will overflow
the cell border (they do this for divs, I know), this patch will regress this.
see attached testcase and
Comment 21 Bernd 2004-02-02 11:31:51 PST
Created attachment 140427 [details]
Comment 22 David Baron :dbaron: ⌚️UTC-10 2004-02-02 12:38:01 PST
That case should overflow the table cell.  Many other cases are handled by
ComputeShrinkwrapMargins.  The code bz is removing is ridiculous and was put in
for other reasons than what you cite.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2004-02-02 18:52:18 PST
Bernd, the code was added in bug 175455 (where your reviewed it), for reasons
having nothing to do with the testcase you posted... Like dbaron, I believe that
overflowing the table cell is the correct thing to happen in that testcase.  Why
do you think something else should be happening?
Comment 24 Bernd 2004-02-02 20:50:44 PST
Currently we are compatible to IE with the rendering with the patch we arent.
Comment 25 David Baron :dbaron: ⌚️UTC-10 2004-02-02 20:56:53 PST
Created attachment 140479 [details]
testcase where our behavior definitely doesn't match IE
Comment 26 Bernd 2004-02-02 21:21:35 PST
I did use the quirks mode intentionally in the testcase. If the doctype is
removed in attachment 140479 [details] IE will render only the 110% percent case  over the
table cell border.
Comment 27 Bernd 2004-02-02 21:22:29 PST
Created attachment 140480 [details]
testcase without doctype
Comment 28 David Baron :dbaron: ⌚️UTC-10 2004-02-02 21:35:55 PST
But that's because IE treats 'width' as border-box in its quirks mode.  If we
want to do that, we should do it for all widths.  However, I haven't seen good
evidence that it's needed for significant numbers of real web pages, which
should be the standard for a quirk.  So you're just confusing issues.
Comment 29 Bernd 2004-02-02 22:07:28 PST
No, I am not, I got a patch for review, it did not pass the regression tests. It
was not mentioned that it will change the rendering. I rised a testcase
demonstrating that there is a issue. I have seen enough pages with 100% width on
a div and borders that expect to be bound by the table cell. Even IE considers
this worth a quirk. This patch will make us differ in quirks mode from IE for
the 100% case. If we (you) decide thats good to differ than its just fine with
me, I like code removal. But if we do this then please let us make clear that
the quirks bugs that arrise from this checkin will be marked as invalid or wontfix.
Comment 30 Brian Earley 2004-02-03 03:24:40 PST
In my testcase from the MozillaZine thread, even IE renders the page correctly
(along with Opera and old versions of Gecko), but not new versions of gecko.
Comment 31 Hixie (not reading bugmail) 2004-02-03 03:33:05 PST
Opera quite happily overflows the cells in that testcase, FYI. (It seems to
assume overflow:hidden on cells, fwiw.)
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2004-02-03 10:25:26 PST
Checked in.
Comment 33 Brian Earley 2004-02-03 12:19:47 PST
Would this happen to fix my testcase as well...? (Can't test it yet obviously)
I'm not sure exactly what was fixed, with the confusion that seemed to be going
on in here.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2004-02-03 12:36:23 PST
(In reply to comment #33)
> Would this happen to fix my testcase as well...?

I have no idea, since you haven't provided a testcase.

> I'm not sure exactly what was fixed, with the confusion that seemed to be going
> on in here.

What was fixed what that blocks inside table cells could be sized incorrectly if
they had negative margins or percentage widths.
Comment 35 Brian Earley 2004-02-03 12:37:58 PST
Good, that sounded like my problem.

I didn't upload a test case, though I probably should have. My testcase was in
the MZ thread. Oh well.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2004-02-03 12:41:52 PST
Yeah, that one is fixed.  And yes, uploading testcases is the way to go -- it
makes testing them a one-click affair instead of having to copy, paste, deal
with the Mozilla bugs in copy/paste, etc.
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2004-02-14 12:39:10 PST
Note that this also fixed a bunch of issues we had with 100%-width replaced
inlines in table cells (the bugs I just added as dependencies).

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