622 bytes, text/html
604 bytes, text/html
15.70 KB, text/plain
472 bytes, text/html
32.87 KB, patch
|Details | Diff | Splinter Review|
5.58 KB, patch
|Details | Diff | Splinter Review|
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.13) Gecko/20060414] (release) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:22.214.171.124) Gecko/20061211 SeaMonkey/1.0.7] (release) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:126.96.36.199pre) Gecko/20070215 SeaMonkey/1.1] (nightly) (W2Ksp4) No bug. [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061207 SeaMonkey/1.5a] (nightly, 2006-12-07-08-trunk) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061208 SeaMonkey/1.5a] (nightly, 2006-12-08-08-trunk) (W2Ksp4) Regressed between these two builds. See <http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-07+08&maxdate=2006-12-08+09&cvsroot=%2Fcvsroot>. My first guess would be that bug 300030 is the culprit. Bug: The "3 Admins Techniques" area in the top table overlapses the "Équipes d'Animation" area in the middle table. [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070214 SeaMonkey/1.5a] (nightly) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/2007021504 Minefield/3.0a3pre] (nightly) (W2Ksp4) Bug still there. (FF & SM)
Created attachment 255365 [details] Reference testcase Removing the height=100% from the table styles allows the testcase to render as it does pre-reflow branch and the same as other browsers.
OS: Windows 2000 → All
Hardware: PC → All
Summary: Regressed page layout → Nested tables with height=100% exceed height of parent table
Bug 368621 is similar and has the same regression range. (doctor__j pointed this out in the other bug.)
Component: Layout → Layout: Tables
QA Contact: layout → layout.tables
So the first question is why does the table gets a non auto height if its parent has auto height. In strict mode the pct height should collapse to auto http://www.w3.org/TR/CSS21/visudet.html#propdef-height
Created attachment 258220 [details] reflow log As I suspected we have a special height reflow. IMHO this should never happen in standards mode.
Unfortunately (perhaps), we've had one particular case of special height reflow (table directly inside table cell) in standards mode for ages, and a lot of major sites depend on it (such as gmail). Before the reflow branch landed the code was convoluted enough that it wasn't clear that this was present, but when I tried to remove it it became a problem. Before the reflow branch there were basically three different ways to get a special height reflow: 1) nsTableCellFrame::NotifyPercentHeight, due to a child of a table cell (quirks mode only) 2) nsTableFrame::CheckRequestSpecialHeightReflow called from a row group, row, or cell 3) nsTableFrame::CheckRequestSpecialHeightReflow called from a table, which magically propagated itself up into the nearest ancestor table no matter how far away that ancestor was: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.665&mark=1875-1878#1860 On the reflow branch, I combined (3) into (1) and limited it only to the cases where (1) happens, i.e., where the table is a child of the cell. However, it had to be done in standards mode to keep gmail and some other major sites working. (I think gmail was one of the standards-mode sites involved.) So now the code for (1) has a non-quirk-mode path: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableCellFrame.cpp&rev=3.396&mark=173-178#150 but we're actually less quirky than before. But we've always had special height reflows in standards mode from (2). Note that CVS checkin comments on the reflow branches might be more informative about which sites had problems...
Sorry, ignore the first paragraph of the previous comment -- I meant to delete it once I remembered / worked out the details.
Created attachment 258229 [details] [diff] [review] fix This fixes the issue here. This is identical to the patch for bug 305975 which got backed out by the reflow branch landing. Maybe David knows why he did back out this. I do not. And I do still not understand the reflow branch code well enough to say what is correct. Anyway I hope I isolated the issue.
cool one can add attachments not in sync with the bug (I did not see comment 7 and 8) when attaching.
And it still wasn't quite clear. We've always had special height reflows in standards mode due to (2) and (3), except the code for (3) is now part of the code for (1). And, by combining (3) into (1), I mean that I removed the call to nsTableFrame::CheckRequestSpecialHeightReflow in nsTableFrame::Reflow and changed the bogus skip-tons-of-ancestors code I cited above into an assertion that we never traverse non-table-frames in nsTableFrame::RequestSpecialHeightReflow. And then I added the code to NotifyPercentHeight that checked for table children even in standards mode (and reorganized nsTableCellFrame::NeedsToObserve so that it matched when we really do need to observe, rather than saying so much more often, and fixed it to make the notifications from inner tables work).
Does this bug cover that bug 305975 regressed, or should another bug be filed?
From a reporter/reader point of view, it seems (that we could say) this bug covers/is that regression.
> Does this bug cover that bug 305975 regressed, or should another bug be filed? I don't see that bug 305975 regressed. All that I said is that I proposed the patch before in that bug.
(In reply to comment #14) > I don't see that bug 305975 regressed. Regarding my comment 13, it seems I misunderstood the testcase there: I (re)tested SM 1.8.1 branch and Trunk and they behave identically. No visible regression. Sorry.
Attachment #258229 - Attachment is obsolete: true
Created attachment 268886 [details] testcase (no cellspacing/cellpadding/borders) Created this alternate testcase with no cellspacing/cellpadding/borders, to make the table-size calculations a bit easier to follow when tracing through the code.
So I've added a few bugs as dependencies of this one that fixing when we do and don't do special height reflows should fix as well. I've also written some testcases at http://dbaron.org/css/test/2006/percent-height-in-tables (for bug 359481, bug 381507, and I think this bug as well), but probably some more testcases are needed. I think what needs to happen here is that we need to fix when we do special height reflows of the *descendants* of a table cell. Right now we do it any time we observe a percent height on a child of the cell; IE/Windows does it only some of the time when that happens, depending on the heights on the cell, row, etc. In quirks mode we probably want to match IE/Windows as closely as reasonable, and definitely match it in all the cases where Firefox 2 in quirks mode matched it. In standards mode we probably want to match IE/Windows in all the cases where Firefox 2 in standards mode matched it, but follow the CSS spec in other cases. (The combination of those requirements may or may not imply having differences between quirks mode and standards mode.) Note that in Firefox 2 we probably had different behavior when the child of the cell was a table and when it was not (see comment 7 and comment 11); we may need to emulate that.
Created attachment 269004 [details] [diff] [review] reftest (as patch) Here's a reftest for the bug (to be checked in after the fix) Made a reftest for both quirks and standards mode. For each of those, I also made an additional reference that should not be matched. (with a percentage-height on the cell, which should force a special height reflow.)
Created attachment 269006 [details] [diff] [review] patch This patch adds a wrapper condition to the requesting-special-height-reflow section of nsTableCellFrame::NotifyPercentHeight. This added condition checks if the Cell, Row, RowGroup, and Table are all auto-height. If they *are* all auto-height, then we DON'T do a special-height reflow. Otherwise (e.g. if any have specified/percentage-height), we do. (This matches IE's behavior.) This patch fixes the testcase, and it passes the layout reftest suite.
Could you reuse the AncestorsHaveStyleHeight function in nsTableFrame.cpp to do some of this work? (You could make it a static method on nsTableFrame. And while you're there IsPctStyleHeight and IsFixedStyleHeight should not need to null-check aStylePosition, and should be inline.) Also, it looks like you don't need to set the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bits in NotifyPercentHeight since RequestSpecialHeightReflow does it.
I think we do still need the (In reply to comment #21) > Also, it looks like you > don't need to set the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bits in > NotifyPercentHeight since RequestSpecialHeightReflow does it. This code was already there, and I don't think it's redundant. NotifyPercentHeight sets the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit from aReflowState.parentReflowState up to cellRS, and RequestSpecialHeightReflow picks up from there, setting the bit from cellRS up to its containing table. So, I don't think we're repeating any work there. If you think we can/should safely remove that section anyway, though, I'm happy to do so.
In reply to comment #22) (Ignore the dangling "I think we do still need the" at the beginning of my last comment... I probably need to proofread my posts more. :))
Created attachment 269021 [details] [diff] [review] reftest (updated) Removed ^M characters that somehow got into the reftests, and removed unnecessary descriptive text that wasn't present in the anti-reftests (!=) and made them trivially pass.
Created attachment 269023 [details] [diff] [review] patch (updated, incorporating modified AncestorsHaveStyleHeight)
Attachment #269023 - Flags: review?(dbaron)
The new patch (attachment 269023 [details] [diff] [review]) fixes the testcases on this bug, as well as those for the duplicate bugs. (bug 364124, bug 366233, bug 368621) It also passes the reftest suite, including my new reftest for this bug. (old patch did as well, but new patch is cleaner)
Comment on attachment 269023 [details] [diff] [review] patch (updated, incorporating modified AncestorsHaveStyleHeight) >+ // Return true if any of aReflowState.frame's ancestors within the >+ // containing table have non-auto height. (e.g. pct or fixed height) >+ // The startAtParent flag indicates whether we should start checking >+ // at aReflowState, or start checking at its parent (default behavior) >+ static PRBool AncestorsHaveStyleHeight(const nsHTMLReflowState& aReflowState, >+ PRBool startAtParent = PR_TRUE); Instead of adding |startAtParent|, rename |aReflowState| to |aParentReflowState| and change the existing caller to pass |aReflowState.parentReflowState|. With that, r+sr=dbaron.
Attachment #269023 - Flags: review?(dbaron) → review+
But what about the second and fourth testcases in http://dbaron.org/css/test/2006/percent-height-in-tables that have the word "hello" in them? Those have a height specified on a different cell in the row. (Maybe that should be the subject of a followup bug, though. But this patch does break both of those relative to both IE/Windows and Firefox 2.)
(Note that it would make sense to do that as part of bug 359481, since it becomes much simpler once heights are accumulated for rows just like widths are accumulated for columns, which is the core part of bug 359481.)
Created attachment 269044 [details] [diff] [review] patch (removed |startAtParent| from prev patch) Incorporated dbaron's suggestions from comment 27
Comment on attachment 269044 [details] [diff] [review] patch (removed |startAtParent| from prev patch) (In reply to comment #28) > But what about the second and fourth testcases in > http://dbaron.org/css/test/2006/percent-height-in-tables that have the word > "hello" in them? Those have a height specified on a different cell in the row. Yup -- it looks like I missed one case -- when determining whether do to a special height reflow, IE checks not only ancestors, but also *siblings*, for non-auto-height. (where siblings = cells in the same row) So, I probably should add another condition to nsTableCellFrame::NotifyPercentHeight, to determine whether any cells in the same row have non-auto-height. Maybe we could add a flag on the row to indicate whether a cell in that row has non-auto-height, so that this check doesn't bog down performance too much? (I'm worrying about the nasty case of a row that has many cells, all of which have nsTableCellFrame::NotifyPercentHeight called on them).
Created attachment 269327 [details] [diff] [review] patch (correctly respond to siblings with non-auto-height) Changes since last patch: we also trigger a special-height reflow if the cell in question (that contains a percentage-height element) has a sibling with non-auto-height. If the cell in question has rowspan != 1, no special height reflow occurs. Similarly, siblings with rowspan != 1 don't count towards triggering special height reflow. (This all matches IE 7's behavior) Also: incorporated reftests into this patch, including new reftests to test all these special cases and variations on them.
Created attachment 269414 [details] [diff] [review] patch (from previous, just made minor changes to reftest.list file) The reftest.list in my previous patch (attachment 269327 [details] [diff] [review]) doesn't jive with today's trunk, because of changes in CVS near my edits. (I also noticed that my modifications in that file weren't quite in the right alphabetical position -- I was off by one line at my insertion point) This new patch fixes both of those issues.
So this looks pretty good, but the one change I'd make is I'd rather have the bit on the row frame than on the reflow state. There are spare frame state bits for row frames -- only the two at the top of nsTableRowFrame.h should currently be used (out of the 12 NS_FRAME_IMPL_RESERVED bits). So if you use that instead, you can also set it at the beginning of nsTableRowFrame::Reflow rather than in nsTableRowGroupFrame::Reflow. This keeps the code involved a good bit more concentrated and doesn't put a table-specific concept into nsHTMLReflowState unnecessarily.
(That said, the way you did it isn't unreasonable, but I think it's cleaner as a bit on the frame.)
Created attachment 269427 [details] [diff] [review] patch (using bit on the frame) Agreed, it's cleaner using a bit on the frame. Here's the patch with that change.
(re-tested this final patch on the reftest suite, including the new reftests, and it still passes.)
Created attachment 269716 [details] [diff] [review] dbaron review comments This is a diff of diffs showing my review comments. They're all pretty minor, so I just pulled the patch in my tree and edited it myself so I can land it (after lunch). The changes are: * change "multi-row-spanning" to "row-spanning" in multiple places * rename CheckHasCellWithStyleHeight to InitHasCellWithStyleHeight, since it initializes a bit * eliminate the myRowSpan variable since it's only used once, and it's slightly cheaper to not compute it if we don't need to * convert the else to a continue in InitHasCellWithStyleHeight, so the main flow of the logic isn't excessively indented * convert the 2 nested ifs to an && in InitHasCellWithStyleHeight * reorder the state bits in nsTableRowFrame.h so they're in numerical oder * rewrite the comment above InitHasCellWithStyleHeight
Checked in a few hours ago.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Is this in-testsuite+ now?
11 years ago
(In reply to comment #40) > Is this in-testsuite+ now? Yup.
Flags: in-testsuite? → in-testsuite+
Attachment #269716 - Attachment is patch: true
David, I'm wondering if the notref test cases here are correct. I have a set of patches in bug 10209 which make the notref cases render the same way as the ref cases. I'm not touching the table reflow code, but so far I've tracked it down to the fact that my patch changes mCBReflowState for some frame types. I've modified my patch to restore the old behavior but I just wanted to make sure that the current behavior is the correct one. Webkit and Opera don't seem to agree with us in the rendering.
Our current behavior is totally broken *and* has been reverse-engineered by other browsers. Fixing that is bug 359481, which needs to happen before IE6 disappears from the Web or we'll be stuck with it. I'd be a little uncomfortable with changing it as the side-effect of another patch, though, without thinking through exactly what the change in behavior is.
(In reply to comment #44) > I'd be a little uncomfortable with changing it as the side-effect of another > patch, though, without thinking through exactly what the change in behavior > is. OK, fair enough. :-)
You need to log in before you can comment on or make changes to this bug.