Closed Bug 232754 Opened 21 years ago Closed 21 years ago

[FIX]negative margins behave strangely inside table

Categories

(Core :: Layout: Block and Inline, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: christopher, Assigned: bzbarsky)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

User-Agent: 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: http://forums.mozillazine.org/viewtopic.php?p=353097#353097 Example: http://www.ing.umu.se/~id01mjn/mozilla.html http://www.ing.umu.se/~id01mjn/mozilla.png
This did not happen on the 0.7 milestone build, but it does happen sometime after that... Another example image: http://www.starcraftsector.com/leech/uts-gecko-changes.jpg I notice this bug happening in this build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040128 Firebird/0.8.0+
Summary: Firebird renders CSS padding wrong in some cases. → Firebird renders CSS padding wrong in some cases.
I think this is probably present in all of the latest versions of Gecko, so the product should probably be "Browser"
I see the same thing in Mozilla build 2004013008 (win2k), so it's not a Firebird issue.
Assignee: blake → nobody
Component: General → Layout: Tables
Product: Firebird → Browser
QA Contact: core.layout.tables
Summary: Firebird renders CSS padding wrong in some cases. → CSS padding wrong in some cases.
Version: unspecified → Other Branch
I believe this may be invalid, but the rules for table cells handling overflow and resizing as needed are pretty wacky....
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.
Keywords: regression
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: CSS padding wrong in some cases. → negative right margin not working inside table
Summary: negative right margin not working inside table → negative margins ignored for max width computation
Summary: negative margins ignored for max width computation → negative margins behave strangely inside table
Component: Layout: Tables → Layout: Block and Inline
QA Contact: core.layout.tables → core.layout.block-and-inline
http://forums.mozillazine.org/viewtopic.php?t=48802 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.
Well, the behavior changed between 2004-01-05-08 and 2004-01-06-10. Could be bug 227819 or bug 226954 (probably the former).
Yeah, the behavior changed with bug 227819
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...
Attached patch Possible patch (obsolete) — Splinter Review
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?
bernd, roc, see the question in comment 12
Attached patch Better patchSplinter Review
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.
Assignee: nobody → bz-vacation
Attachment #140342 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 140347 [details] [diff] [review] Better patch Bernd, David, what do you think?
Attachment #140347 - Flags: superreview?(dbaron)
Attachment #140347 - Flags: review?(bernd_mozilla)
Priority: -- → P1
Summary: negative margins behave strangely inside table → [FIX]negative margins behave strangely inside table
Target Milestone: --- → mozilla1.7alpha
Version: Other Branch → Trunk
Boris does your patch also fix the issues in bug 198283?
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...
Blocks: 198283
Any chance of getting this checked in before the 0.8 Firebird milestone?
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.
Attachment #140347 - Flags: superreview?(dbaron) → superreview+
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 http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug4576.ht ml
Attachment #140347 - Flags: review?(bernd_mozilla) → review-
Attached file testcase
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.
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?
Currently we are compatible to IE with the rendering with the patch we arent.
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.
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.
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.
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.
Opera quite happily overflows the cells in that testcase, FYI. (It seems to assume overflow:hidden on cells, fwiw.)
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.
(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.
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.
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.
Blocks: 229634
Blocks: 217817
Blocks: 194804
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).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: