With the attached testcase, the first cell should be the width of the image within it. However, since the reflow branch landed, its width is being miscalculated. For some reason, the second row must contain the long string of unbroken text. If there are spaces in it, the width gets computed properly.

The code at http://lxr.mozilla.org/seamonkey/source/layout/tables/BasicTableLayoutStrategy.cpp#398 should apply a similar logic to pct width as to pref width.

These tests render identically prior to the reflow branch landing and should once again once this bug is fixed.

I cleaned up the code a bit to make the tests more readable

Comment on attachment 257603 [details] [diff] [review] Cleaned up tests [checked in] Checked in after testing in pre- and post-reflow-branch builds.

Reftest checked in marked as failing. Remember to change reftest.list once this bug is fixed.

It seems like we need to apply the effects of the large percentage effect to the pref width before doing spanning cell width distribution. (I wonder if there's anything we need to do for the small percentage effect.) (I wonder if we need to do it each iteration.)

I ran into a variation on this bug today. Here's another set of tests to cover that one.

I think bug 379361 may be a dup of this bug. (Not marking it yet 'cause I'm not sure.)

Comment on attachment 259152 [details] [diff] [review] More reftests Daniel, if you're going to be the one working on this, can you go ahead and review & check in this second set of tests too?

I'd suggest a few changes, to clean up the tests / make the two files more similar. First, the nested table isn't necessary -- in 368504-2.html, you can replace: <table cellpadding=0 cellspacing=0><tr><td><div style="width: 20em">&nbsp;</div></td></tr></table> with just the innermost div: <div style="width: 20em">&nbsp;</div> And since we're making that change, you might change 368504-2-ref.html from <tr><td colspan="2">&nbsp;</td></tr> to <tr><td colspan="2"> <div>&nbsp;</div> </td></tr> thereby making the two files *exactly* the same, except for the 'style="width: 20em"'. That way, it's very clear what's causing the difference in rendering. With those changes, I'd give it a review+.

Also, you might consider replacing the image with a single character (e.g. 'a') for simplicity, since there's nothing especially image-dependent in this bug. (Not a major deal, but simpler is better as far as reftests are concerned)

Posting a new testcase, which reveals the underlying problem in this bug. (which is actually *different* from the problem in 379361) The issue here is with distributing a colspan cell's width between columns when one of the columns has percent-width. It looks like we don't take the percent width into account at all at this stage. Instead, given two columns, we divide up the preferred width of the colSpan ("StringOfReallyLongUnbrokenText") between the two columns according to the ratio of their own preferred widths (which are equal in this case, so we get equal-sized columns). None of this code takes the percent width into account, but it should. Compare attachment 270388 [details] in IE vs. Trunk for an example. Note that we do take percentages into account later on, when we're balancing extra table width between the columns, as shown in attachment 270389 [details]. (Compare in IE/FF2 vs Trunk) SIDE NOTE: Use IE to view correct behavior of attachment 270388 [details], because FF2.0 mis-handles this case. It gets confused because table width is 1px, so it shrinks the 100% cell as small as it can, despite the fact that this doesn't make the table any smaller because all the width ends up going to the other cell. This is a bug in FF 2.0 with respect to IE and Opera, and to a lesser extent Konqueror/Safari (which show the same buggy behavior as trunk). This should probably be filed as a bug in 1.8 Branch at some point.

Reftests changed as requested

Comment on attachment 270398 [details] [diff] [review] More reftests round 2 [checked in] Looks good to me. Sending to dbaron for superreview / checkin.

Comment on attachment 270398 [details] [diff] [review] More reftests round 2 [checked in] Cancel that -- gonna find someone who's not very busy (not dbaron :)) to check it in at some point, since it's not an especially high-priority patch. I'll make sure it's in by Monday night.

Checked in "More reftests round 2": Checking in bugs/368504-2-ref.html; /cvsroot/mozilla/layout/reftests/bugs/368504-2-ref.html,v <-- 368504-2-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/368504-2.html,v done Checking in bugs/368504-2.html; /cvsroot/mozilla/layout/reftests/bugs/368504-2.html,v <-- 368504-2.html initial revision: 1.1 done Checking in bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.103; previous revision: 1.102 done

Comment on attachment 270398 [details] [diff] [review] More reftests round 2 [checked in] Thanks kherron!

Just posted another testcase, with 2 reference cases. I'll probably add these as reftests, too, because Opera passes both of ryanvm's posted reftests but fails this one. Note that the testcase and reference 1 both use white text so that they'll appear the same when made into reftests. (To see the text, highlight the region below the first row.)

patch version of my "Yet Another Testcase" reftest.

I've got a (simple) fix, which is this: *Disable the main part of ComputeColumnIntrinsicWidths, for any colspan that contains a percent-width column.** This seems to be what FF2 does (from looking at a few testcases), and this fix makes us pass the reftests for this bug without breaking any other layout reftests. Posting a slightly hand-edited patch implementing this fix. I had to hand-edit the patch a bit because this fix requires and interacts with code from my not-yet-landed patch for Bug 379361. (and my patchcraft is still humble -- I hear interdiff could help, but I couldn't get it to do what I wanted.) Steps to use this patch: 1. Apply attachment 270630 [details] [diff] [review], the proposed patch for Bug 379361 (unless it's already been landed when you read this) 2. Apply this patch that I'm posting, using fuzz 3 ("patch -F3") The only thing left out of this patch is (cosmetic) indentation within the new "if" block. Not adding that right now to keep the patch simple. (because of the aforementioned complications of code overlap with attachment 270630 [details] [diff] [review])

(In reply to comment #28) > Created an attachment (id=271161) [details] > (and my patchcraft is still humble -- I > hear interdiff could help, but I couldn't get it to do what I wanted.) Turns out I needed to use "interdiff -w". Here's a less hacked-together patch. This one doesn't require a fuzz factor in the patch command. (But attachment 270630 [details] [diff] [review] is still a prereq.) I'm still leaving out the indentation within the new "if" region, so as not to complicate this patch & its dependence on the patch to bug 379361.

(In reply to comment #29) > I'm still leaving out the indentation within the new "if" region, so as not to > complicate this patch & its dependence on the patch to bug 379361. In case it wasn't clear before -- I'll post a finalized version of this patch (with indentation in "if" region) after the patch for bug 379361 lands. I'm not posting that yet because of code intersection between the two bugs.

Posting the updated version of my previous patch, now that bug 379361 has landed. However, I've found that this patch breaks a variant testcase (which I'm posting next), so it's not a good final patch.

Posting a modified version of "new testcase" (attachment 270389 [details]) that gets broken by my patches so far. It's using 50% with, rather than 100%.

Posting a series of tests with varying percentage-width on a column within a colspan. Out of FF 2, IE7, Trunk, Opera, and Konqueror, the only two browsers that render this test-page the same are Trunk and Konqueror, and they're both wrong. (They both show the buggy behavior that this bug is about.)

Here's a patch with a more effective strategy than my previous patches on this bug. This patch effectively makes us distribute the colspan's excess min-width and pref-width according to the percentages of the percent-width columns that make up the colspan. I'll post a better explanation soon.

no-whitespace-changes version of previous patch

Ok -- it turns out there were a few edge cases where my pool-distribution patch didn't do the right thing. Hence, obsoleted that patch. I've written a new & improved patch, and I'm pretty sure it's the right way to go. It basically separates out the bulk of BasicTableStrategy::ComputeColumnWidths into a helper function, which is now also called by ComputeColumnIntrinsicWidths. The helper replaces the bulk of the work done in ComputeColumnIntrinsicWidths. (I should give credit to dbaron for suggesting this idea to me a while ago -- at the time, I'd still thought that the pool-distribution strategy implemented in my previous patch would work, but this new way's definitely better. :) ) The new patch passes reftests, including the reftests on this bug. I may wait to post this patch until after bug 367673's patch is reviewed and lands, because that patch and this one intermix a good deal in their contextual regions and it's a bit tricky to separate them out.

(In reply to comment #37) > I may wait to post this patch until after bug 367673's patch is reviewed and > lands Ok, that patch landed has now landed. Here's a draft patch for this bug, with the guts of ComputeColumnWidths separated out into a "ComputeColumnWidthHelper" function, which is called by ComputeColumnIntrinsicWidths.

I have one question on advice for implementation. PROBLEM: How best to make ComputeColumnWidthHelper have different behaviors for min, pref, and final widths. POTENTIAL SOLUTIONS: a) Create a new enum with values like {min, pref, final}, and use a switch statement, based on which value was passed in. (as implemented in draft patch) b) Co-opt nsLayoutUtils::IntrinsicWidthType, adding "FINAL_WIDTH" to it, and maybe changing its name to just "WidthType", since "Final" isn't an intrinsic width. This enum is currently only used by nsLayoutUtils::IntrinsicForContainer, and I'd add a NOTREACHED that'd be hit if FINAL_WIDTH was ever passed in to that function. c) Don't use an enum at all, but instead, pass in some function pointer that takes arguments (nsColFrame* colFrame, nscoord newWidth)". Apply the width via a call to this passed-in function pointer. (And the callees would choose between three different function pointers to pass in -- one for pref width, one for min width, and one for final width) dbaron (and anyone else), what do you suggest?

Of those three, (a) seems best. What's the basic idea of what you're doing in this patch?

(In reply to comment #40) > Of those three, (a) seems best. Ok. > What's the basic idea of what you're doing in this patch? Basic idea: To co-opt the behavior of ComputeColumnWidths and apply it to the "distribute a span's width to its columns" section of ComputeIntrinsicWidths Quick Rationale: The problems of "distributing table width to table columns" and "distributing span width to spanned columns" are pretty much the same problem, but we've got different code for the two right now. (and the intrinsic-width-distributing code doesn't do what we'd like in all cases, like in this bug's testcases)

Re-posting the patch, with these changes: - Updated context to remove bitrot - Implemented solution (a) from comment 39. I just prepended "BASIC_" to the names of my new enum's values to prevent confusion with the values of nsLayoutUtils::IntrinsicWidthType. The resulting enum is as follows: enum BasicWidthType { BASIC_MIN_WIDTH, BASIC_PREF_WIDTH, BASIC_FINAL_WIDTH }; - Fixed spacing-computation calculations at the beginning of ComputeColumnWidthHelper. (The spacing calculation was wrong in my prev patch) - Fixed loops in ComputeColumnWidthHelper by replacing "aColCount" with "aFirstCol + aColCount" as the loop-counter's upper-bound. This patch passes the layout reftests, and it fixes the reftests for this bug.

One more change: - removed a xxxdholbert comment about adding suggested solution (c) on comment 39. (using a function object)

You need to add the reftests already on this bug (and maybe more) -- or change the status of any that are already checked in. BasicTableLayoutStrategy.h: + // NOTE: Using prefix "BASIC" to avoid overlapping names with + // the values of nsLayoutUtils::IntrinsicWidthType + enum BasicWidthType { BASIC_MIN_WIDTH, + BASIC_PREF_WIDTH, + BASIC_FINAL_WIDTH }; + You're probably better off using BTLS rather than BASIC, because that's more clearly the name of the class (or random garbage), whereas people might think BASIC has its English meaning. + PRBool aSpanHasSpecifiedWidth = PR_FALSE); Given that only one call uses the default value, I think you're better off not having a default value and just passing the argument explicitly at that one caller. Default values for parameters tend to be confusing; better to avoid them when they don't provide significant benefit. I'd also rename ComputeColumnWidthHelper to something indicating what it does. Maybe DistributeWidthToColumns? In ComputeColumnWidthHelper: + aWidth = NSCoordSaturatingSubtract(aWidth, subtract, nscoord_MAX); only one space after the = - * The goal of this function is to allocate |width| to the columns + * The goal of this function is to allocate |aWidth| to the columns * by making an appropriate SetFinalWidth call to each column. You also need to fix this comment to explain what this function now does. - col_width = nscoord(float(width) * pct); - nscoord col_min = colFrame->GetMinCoord(); - if (col_width < col_min) - col_width = col_min; + col_width = PR_MAX(nscoord(float(aWidth) * pct), + colFrame->GetMinCoord()); Probably better to leave this as is -- it's generally better to keep arguments of macros as simple as possible, since they can be (and in this case are) evaluated multiple times. Hopefully the optimizer will fix things most of the time, though. (It can be dangerous when things have side-effects.) space -= col_width - col_width_before_adjust; - + NS_ASSERTION(col_width >= colFrame->GetMinCoord(), "assigned width smaller than min"); Don't insert trailing whitespace. + // Update aWidth if we need to + if (aWidth < guess_min) { + if (aWidthType == BASIC_FINAL_WIDTH) { + NS_NOTREACHED("bad width"); + } else { + // NOTE: Changing argument + // This is acceptable in the min / pref-coord case. In these + // cases, aWidth is the min/pref width of the spanning cell, + // and guess_min is the sum of the spanned columns' min widths. + // The ultimate min/pref width of the overall span will be the + // larger of these two. + aWidth = guess_min; + } + } It seems like you could put this inside the |if (aWidth < guess_min_pct)| and that you could just make the NS_NOTREACHED an NS_ASSERTION(aWidthType != BASIC_FINAL_WIDTH, ...) and not bother with an if/else for BASIC_FINAL_WIDTH I'm still working on comparing the old code to the new code. But the biggest problem I've found so far is that you're removing a call to AddSpanPrefPercent but not adding a new one. Was that intentional? Or should you leave the necessary code to do that in? Have you run existing reftests?

Comment on attachment 285185 [details] [diff] [review] patch (using ComputeColumnWidthHelper) v3 ...and if nothing tests what I think you broke, you should add a test for that, too.

I'm also a little concerned about performance here. Maybe the best way to improve that would be, in the case where you adjust aWidth that I commented on above, instead of what I suggested, return early, since you don't need to do any more work. (I don't think you do, anyway.)

Here's patch v3 again, un-bitrotted so it will apply to current trunk. (I'll address review comments & post an updated patch soon)

Here's a work-in-progress patch, addressing most of the review comments from comment 44. Specific responses below: > You need to add the reftests already on this bug (and maybe more) -- or change > the status of any that are already checked in. Still to-do > You're probably better off using BTLS rather than BASIC, Done > Given that only one call uses the default value, I think you're better > off not having a default value and just passing the argument explicitly > at that one caller. Done > I'd also rename ComputeColumnWidthHelper to something indicating what it > does. Maybe DistributeWidthToColumns? Done > only one space after the = Done (oops) > You also need to fix this comment to explain what this function now > does. Still to-do > Probably better to leave this as is -- it's generally better to keep > arguments of macros as simple as possible ... Done > Don't insert trailing whitespace. Done (oops) > It seems like you could put this inside the |if (aWidth < > guess_min_pct)| and that you could just make the NS_NOTREACHED an > NS_ASSERTION(aWidthType != BASIC_FINAL_WIDTH, ...) and not bother with > an if/else for BASIC_FINAL_WIDTH (In reply to comment #46) > I'm also a little concerned about performance here. Maybe the best way to > improve that would be, in the case where you adjust aWidth that I commented on > above, instead of what I suggested, return early, since you don't need to do > any more work. (I don't think you do, anyway.) Still to-do > I'm still working on comparing the old code to the new code. But the > biggest problem I've found so far is that you're removing a call to > AddSpanPrefPercent but not adding a new one. Was that intentional? Or > should you leave the necessary code to do that in? Partly done. No, it was not intentional. This patch re-inserts the necessary code, wrapped up in a function called DistributePctToSpannedColumns. I intend to improve / better-integrate the DistributePctToSpannedColumns functionality in an upcoming (final? :)) patch. > Have you run existing reftests? (In reply to comment #45) > ...and if nothing tests what I think you broke, you should add a test for that, > too. When I first posted patch v3, it passed the reftest suite. Today, the un-bitrotted version of patch v3 passes everything except for 403519-1.html and 403519-2.html (which were checked-in after patch v3 was first posted). It fails those tests partly due to nscoord_MAX arithmetic and partly due to the not-calling-AddSpanPrefPercent issue. (Interestingly, the AddSpanPrefPercent issue actually broke the reference case, not the testcase, because the reference case is the one with 100%-width on the colspan.) Both of those issues are fixed in this new patch. For my next patch, I'll include a new reftest that more directly tests the colspan-percent-width-distribution functionality. This work-in-progress patch currently passes the reftest suite. (including the checked-in reftests for this bug)

Here's a bundle of new reftests. This reftest-patch: - Changes the status of the already-checked-in reftests - Includes a slightly modified version of the previously-posted "Yet Another Testcase" reftest (attachment 270659 [details]) - Includes two new reftests based on attachment 275163 [details] ("multitest") - Includes two new reftests for the "table-width" directory, to test colspan percent-width distribution. For reference, here's information about which of these tescases pass in various patching states: Current Trunk: - All 368504-* FAIL (except 368504-3b.html -- see note below) - All colspan-percent-distribution-* pass Current Trunk with Patch v3: - All 368504-* pass - All colspan-percent-distribution-* FAIL Current Trunk with Patch v4: - All 368504-* pass - All colspan-percent-distribution-* pass * Note: 368504-3b.html is the only 368504-* reftest that passes in current trunk (before any patching). This is expected -- it's based on "Yet Another Testcase reference 1" and is in there partly as a sanity check, and partly as an intermediate testcase between 368504-3a.html and the more simplified 368504-3-ref.html

Cleaned up the 368504-[345] reftests a bit (replaced a block of text with a fixed-width div in 368504-3, to prevent dependency on font sizes, and removed some unnecessary divs in 368504-4 and 368504-5)

Dave, minor request. In the future, can you do your diffs from the /mozilla directory please? Thanks :-)

(In reply to comment #51) > Dave, minor request. In the future, can you do your diffs from the /mozilla > directory please? Thanks :-) (Assuming s/Dave/Daniel/) :) I think I remember dbaron recommending I do diffs from the deepest directory possible, though I may be mistaken. Since dbaron reviews lots of my patches, I'll probably lean towards doing what he prefers -- dbaron, do you prefer /mozilla-rooted diffs or deepest-directory-rooted diffs?

Comment on attachment 296075 [details] [diff] [review] more reftests, round 3b Here's a summary of the new reftests. They all basically check that we honor percent-widths within colspans, without divvying out too much of the colspan's min-width to the wrong spanned columns before doing percentage calculations. + 368504-3a.html + 368504-3b.html + 368504-3-ref.html These are based on the "Yet Another Testcase" attachments on this bug. + 368504-4.html + 368504-4-ref.html + 368504-5.html + 368504-5-ref.html These testcases are a bit more rigorous. Across a wide range of percentages, they basically assert that a percent-width on a column will be honored, using the table's width as a basis for the percentage, even if the column is part of a wide colspan. 368504-4 is in a situation where the table is shrink-wrapped to the width of the colspanning cell. (In the -ref case, instead of shrinkwrapping to the colspan's width, I just assign a width to the table and replace the colspanning cell with two individual cells) 368504-5 has no shrinkwrap, so small- and large-percentage effects are visible -- the tables grow to accomodate percentages. (In the -ref case, I set a -min-width on the table to replace the colspan's width, and I replace the colspanning cell with two individual cells) + colspan-percent-distribution-1.html + colspan-percent-distribution-1-ref.html Asserts that a colspanning cell with 100% width behaves exactly the same as a normal cell with 100% width, if you can't see cells in other rows. (the 2nd row gets zero height, in this case.) + colspan-percent-distribution-2.html + colspan-percent-distribution-1-ref.html Asserts that a 100%-wide colspanning cell behaves the same as two side-by-side 50%-wide normal cells, with respect to the width effects on cells in the next row.

Comment on attachment 296075 [details] [diff] [review] more reftests, round 3b I think you should just check in reftests without review, so I'm just largely rubber-stamping this. (If you want me to review it more closely, let me know.) One comment, though: >+ /* cellspacing=0 cellpadding=0 */ >+ table { border-collapse: collapse; } >+ table td { padding: 0; } Really, "cellpadding=0" is equivalent to "td, th { padding: 0 }" and "cellspacing=0" is equivalent to "table { border-spacing: 0 }". border-collapse:collapse is an entirely different model, and there's no need for the descendant selector "table td". It's really ok either way, but the comment should match what's below it...

(In reply to comment #54) > (From update of attachment 296075 [details] [diff] [review]) > I think you should just check in reftests without review, so I'm just largely > rubber-stamping this. (If you want me to review it more closely, let me know.) Ok, thanks! > One comment, though: > ... > Really, "cellpadding=0" is equivalent to "td, th { padding: 0 }" and > "cellspacing=0" is equivalent to "table { border-spacing: 0 }". Ah, thanks for the correction -- I was misled by http://csssky.blogspot.com/2007/04/css-and-cellspacing0-cellpadding0.html I'll fix that when I check these in.

Here are the new reftests, as landed. I just made the cellspacing/cellpadding CSS change, per dbaron's comment, and marked the 368504-* ones as failing.

(patch v5 --> v5b: removing an unrelated/unnecessary whitespace change)

Requesting review on patch v5b. Here are my responses to the prior review comments that I'd marked "to-do" in comment 48: > > You need to add the reftests already on this bug (and maybe more) -- or change > > the status of any that are already checked in. > Done. (New reftests already checked in, and patch v5b changes their status.) > > You also need to fix this comment to explain what this function now > > does. > Done > > It seems like you could put this inside the |if (aWidth < > > guess_min_pct)| and that you could just make the NS_NOTREACHED an > > NS_ASSERTION(aWidthType != BASIC_FINAL_WIDTH, ...) and not bother with > > an if/else for BASIC_FINAL_WIDTH > (In reply to comment #46) > > I'm also a little concerned about performance here. Maybe the best way to > > improve that would be, in the case where you adjust aWidth that I commented on > > above, instead of what I suggested, return early, since you don't need to do > > any more work. (I don't think you do, anyway.) Done -- I turned this into an assertion & an early return, as suggested. You're right -- we don't need to do any more work in that case. > > you're removing a call to > > AddSpanPrefPercent but not adding a new one. Was that intentional? Or > > should you leave the necessary code to do that in? Done -- this patch adds the function "DistributePctWidthToSpannedColumns" which encompasses the colspan-percent-distribution behavior. This function is basically a slimmed down version of the code that originally handled this at lines 361-455: http://mxr.mozilla.org/seamonkey/source/layout/tables/BasicTableLayoutStrategy.cpp#361

FWIW, I've been running the last couple days with v4 and v5b and haven't noticed any obvious regressions so far.

Comment on attachment 296419 [details] [diff] [review] patch v5b I'm not sure const-ness is really meaningful here -- there's not a whole lot currently stored on the BasicTableLayoutStrategy, although there could be more in the future if we cached more on it. So const-ness could be a little confusing -- at least partly because the frames manipulated are never const. So probably better not to make these two new methods const methods and change one existing one to const. header file: + // colspan's min & pref width. BTLW_FINAL_WIDTH is intended BTLS, not BTLW. It might be nice to make the names and parameters of DistributeWidthToColumns and DistributePctWidthToSpannedColumns a little more consistent -- i.e., have "Spanned" in neither or both, and use the aFirstCol/aColCount naming for both. + // FIRST LOOP: For the colspan in question, determine... + // A) the number of columns that don't have a percentage width + // B) the total preferred width of those columns + // C) how much of the colspan's percent width is left for those columns + PRInt32 nonPctColCount = 0; // (A) above + nscoord nonPctTotalPrefWidth = 0; // (B) above + // Note: The arg aSpanPrefPct will be used as (C) above. Seems clearer to just write: // First loop to determine: PRInt32 nonPctColCount = 0; // number of spanned columns without % width nscoord nonPctTotalPrefWidth = 0; // total pref width of those columns // and to reduce aSpanPrefPct by columns that already have % widths rather than dealing with (A) and (B) and (C). + if (aSpanPrefPct <= 0.0f) { + // No percent-width on the colspan is left over to distribute. + // We're done. + return; + } You can also return early if nonPctColCount is 0, since that means there's no place you can distribute it to. (And given that, you can remove the assert later that nonPctColCount != 0.) + if (scolFrame->GetPrefPercent() == 0.0f && + aSpanPrefPct != 0.0f) { I don't think you need this aSpanPrefPct != 0.0f check here. It may be possible for it to be zero in edge cases due to rounding error, but the main point of that check in the old code was to exclude the cases where you're not calling this function at all. - FLEX_FLEX_LARGE, // above (4) above, case (a) - FLEX_FIXED_LARGE, // above (4) above, case (b) - FLEX_PCT_LARGE, // above (4) above, case (c) - FLEX_ALL_LARGE // above (4) above, case (d) + FLEX_FLEX_LARGE, // above (4), case (a) + FLEX_FIXED_LARGE, // above (4), case (b) + FLEX_PCT_LARGE, // above (4), case (c) + FLEX_ALL_LARGE // above (4), case (d) These were actually intended as written, but "greater than (4) above" would probably be clearer. + if (aWidth < guess_min) { + // Note: It's ok to have aWidth < guess_min if we're + // distributing colspan width. In that case, aWidth is the + // min or pref width of the spanning cell, and guess_min is + // the sum of the spanned columns' min widths. The ultimate + // min or pref width of the overall span will be the larger + // of these two. + NS_ASSERTION(aWidthType != BTLS_FINAL_WIDTH || + aWidth == guess_min, + "Table width is less than the " + "sum of its columns' min widths"); + // Return early -- we don't have any extra space to distribute. + return; + } You can't return early if aWidthType is BTLS_FINAL_WIDTH. But the second half of the assertion is clearly never going to be true because of the condition it's inside. You want to drop the second half of the assertion (after the ||). You may also want to change the condition to be if (aWidthType != BTLS_FINAL_WIDTH && aWidth <= guess_min) and then move the equivalent of the assertion to outside the condition. In fact, I'd think you could even change it to: if ((aWidthType == BTLS_MIN_WIDTH && aWidth <= guess_min) || (aWidthType == BTLS_PREF_WIDTH && aWidth <= guess_pref)) although I'm not 100% sure that's ok (you should check). However, if aSpanHasSpecifiedWidth is true, you actually do need to call AddSpanCoords on all the spanned columns with (0, 0, PR_TRUE) before returning early so that the cells act as though they have a specified width (and thus they don't expand in a FLEX_FLEX_LARGE expansion of the table during final width calculation). (Worth adding a test for this too.) - float c = float(space) / float(basis.c); - basis.c -= col_width; - col_width += NSToCoordRound(float(col_width) * c); + if (space == nscoord_MAX) { + basis.c -= col_width; + col_width = nscoord_MAX; + } else { + float c = float(space) / float(basis.c); + basis.c -= col_width; + col_width += NSToCoordRound(float(col_width) * c); + } You could leave the "basis.c -=" below the if/else since it's common. + case BTLS_MIN_WIDTH: + { + // Update pref coord, if necessary, to keep it >= the + // new min coord + nscoord updatedPref = PR_MAX(colFrame->GetPrefCoord(), + col_width); + colFrame->AddSpanCoords(col_width, updatedPref, + aSpanHasSpecifiedWidth); + } + break; You can just pass col_width for both parameters and drop updatedPref since the accumulation will be done later. (Right?) + case BTLS_PREF_WIDTH: + { + // Look up current min coord, to pass into AddSpanCoords + // with new pref coord + nscoord min = colFrame->GetMinCoord(); + colFrame->AddSpanCoords(min, col_width, + aSpanHasSpecifiedWidth); + } + break; And here, for the same reason, you can just pass 0 instead of min. r+sr=dbaron with that

Here's the patch, with final review comments addressed. I'm pretty sure this is final, though I'm running it through reftests one more time before landing. Responses to review comments: > I'm not sure const-ness is really meaningful here Done -- removed those consts. > + // colspan's min & pref width. BTLW_FINAL_WIDTH is intended > > BTLS, not BTLW. Done -- good catch. > It might be nice to make the names and parameters of > DistributeWidthToColumns and DistributePctWidthToSpannedColumns a little > more consistent -- i.e., have "Spanned" in neither or both, and use the > aFirstCol/aColCount naming for both. Done (removed "Spanned" and used aFirstCol/aColCount naming) > Seems clearer to just write: > > // First loop to determine: > PRInt32 nonPctColCount = 0; // number of spanned columns without % width > nscoord nonPctTotalPrefWidth = 0; // total pref width of those columns > // and to reduce aSpanPrefPct by columns that already have % widths Done > > + if (aSpanPrefPct <= 0.0f) { > + // No percent-width on the colspan is left over to distribute. > + // We're done. > + return; > + } > > You can also return early if nonPctColCount is 0, since that means > there's no place you can distribute it to. Done > (And given that, you can remove the assert later that nonPctColCount != > 0.) Actually, I can't / shouldn't -- nonPctColCount gets decremented at the end of the loop that surrounds this assert. So even if nonPctColCount is above zero at the return-early check, it could be reduced to zero during the loop, triggering this assert. > + if (scolFrame->GetPrefPercent() == 0.0f && > + aSpanPrefPct != 0.0f) { > > I don't think you need this aSpanPrefPct != 0.0f check here. It may be > possible for it to be zero in edge cases due to rounding error, but the > main point of that check in the old code was to exclude the cases where > you're not calling this function at all. Done > These were actually intended as written, but "greater than (4) above" > would probably be clearer. Done > + if (aWidth < guess_min) { > + // Note: It's ok to have aWidth < guess_min if we're > + // distributing colspan width. In that case, aWidth is the > + // min or pref width of the spanning cell, and guess_min is > + // the sum of the spanned columns' min widths. The ultimate > + // min or pref width of the overall span will be the larger > + // of these two. > + NS_ASSERTION(aWidthType != BTLS_FINAL_WIDTH || > + aWidth == guess_min, > + "Table width is less than the " > + "sum of its columns' min widths"); > + // Return early -- we don't have any extra space to distribute. > + return; > + } > > You can't return early if aWidthType is BTLS_FINAL_WIDTH. But the > second half of the assertion is clearly never going to be true because > of the condition it's inside. You want to drop the second half of the > assertion (after the ||). > > You may also want to change the condition to be > if (aWidthType != BTLS_FINAL_WIDTH && aWidth <= guess_min) > and then move the equivalent of the assertion to outside the condition. > > In fact, I'd think you could even change it to: > > if ((aWidthType == BTLS_MIN_WIDTH && aWidth <= guess_min) || > (aWidthType == BTLS_PREF_WIDTH && aWidth <= guess_pref)) > > although I'm not 100% sure that's ok (you should check). Done. This is ok -- it passes reftests. > However, if aSpanHasSpecifiedWidth is true, you actually do need to call > AddSpanCoords on all the spanned columns with (0, 0, PR_TRUE) before > returning early so that the cells act as though they have a specified > width (and thus they don't expand in a FLEX_FLEX_LARGE expansion of the > table during final width calculation). (Worth adding a test for this > too.) > > - float c = float(space) / float(basis.c); > - basis.c -= col_width; > - col_width += NSToCoordRound(float(col_width) * c); > + if (space == nscoord_MAX) { > + basis.c -= col_width; > + col_width = nscoord_MAX; > + } else { > + float c = float(space) / float(basis.c); > + basis.c -= col_width; > + col_width += NSToCoordRound(float(col_width) * c); > + } > > You could leave the "basis.c -=" below the if/else since it's common. Nope -- it needs to stay there, because col_width (the thing being subtracted) changes in the final line of each clause. > You can just pass col_width for both parameters and drop updatedPref > since the accumulation will be done later. (Right?) > ... > And here, for the same reason, you can just pass 0 instead of min. Good point -- Done, with comments explaining why it's ok to pass in those arguments. > r+sr=dbaron with that Great! Thanks very much for the review.

just tweaked a few comments in this final patch. This passes reftests. Will check in later tonight, when tree is quiet.

You missed this one in comment 61: > However, if aSpanHasSpecifiedWidth is true, you actually do need to call > AddSpanCoords on all the spanned columns with (0, 0, PR_TRUE) before > returning early so that the cells act as though they have a specified > width (and thus they don't expand in a FLEX_FLEX_LARGE expansion of the > table during final width calculation). (Worth adding a test for this > too.)

Though, actually, I'm having trouble writing a test for it, since I forgot about the "mHasSpecifiedCoord && " in AccumulateSpanIntrinsics. But I'm also not sure you're ok without it.

Ah, ok -- thanks for catching that; sorry I missed it. I'll look into it tomorrow morning and get back to you.

This patch addresses the review note that I missed, mentioned in comment 66. It also includes a new reftest 368504-6.html that tests this issue, as well. Pre-patching: we pass 368504-6.html After applying patch v5d: we fail 368504-6.html After applying patch v6: we pass 368504-6.html

Comment on attachment 297428 [details] [diff] [review] interdiff between patches 5d and 6 dbaron, could you let me know if this interdiff of the last change looks good?

We discussed this in person, and what I now think is that: * the interdiff in attachment 297428 [details] [diff] [review] is not needed * the fact that it did anything was a sign of an existing bug in our code. Basically, mSpanHasSpecifiedWidth shouldn't exist, and AddSpanCoords should check (but never modify) mHasSpecifiedWidth (i.e., we should completely drop the clause that modifies it) This fits better with the model where specified widths on a spanning cell can't change a column from not having a specified width to having a specified width. dholbert said on IRC that both WinIE and Safari failed the new test (i.e., did what we did pre-attachment 297428 [details] [diff] [review]). We probably want to test for that...

(In reply to comment #72) > Basically, mSpanHasSpecifiedWidth shouldn't exist, and AddSpanCoords should > check (but never modify) mHasSpecifiedWidth (i.e., we should completely drop > the clause that modifies it) This new patch adds this modification to patch v5d. A reftest for this behavior is coming up next.

This reftest partly catches the bug dbaron mentioned in comment 72. It fails in Trunk + Patch v6 It passes in Trunk + Patch v7 However, the reftest passes in trunk before patching, though, so I'd ideally like to add one more reftest that would catch this bug in pre-patched trunk.

Patch v7 and "another reftest" checked in. RCS file: /cvsroot/mozilla/layout/reftests/bugs/368504-6-ref.html,v done Checking in reftests/bugs/368504-6-ref.html; /cvsroot/mozilla/layout/reftests/bugs/368504-6-ref.html,v <-- 368504-6-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/368504-6.html,v done Checking in reftests/bugs/368504-6.html; /cvsroot/mozilla/layout/reftests/bugs/368504-6.html,v <-- 368504-6.html initial revision: 1.1 done Checking in reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.319; previous revision: 1.318 done Checking in tables/BasicTableLayoutStrategy.cpp; /cvsroot/mozilla/layout/tables/BasicTableLayoutStrategy.cpp,v <-- BasicTableLayoutStrategy.cpp new revision: 3.265; previous revision: 3.264 done Checking in tables/BasicTableLayoutStrategy.h; /cvsroot/mozilla/layout/tables/BasicTableLayoutStrategy.h,v <-- BasicTableLayoutStrategy.h new revision: 3.69; previous revision: 3.68 done Checking in tables/nsTableColFrame.cpp; /cvsroot/mozilla/layout/tables/nsTableColFrame.cpp,v <-- nsTableColFrame.cpp new revision: 3.98; previous revision: 3.97 done Checking in tables/nsTableColFrame.h; /cvsroot/mozilla/layout/tables/nsTableColFrame.h,v <-- nsTableColFrame.h new revision: 3.72; previous revision: 3.71 done .