Closed Bug 425972 Opened 14 years ago Closed 14 years ago

[effective columns] td element with bgcolor no longer paints along full width of table


(Core :: Layout: Tables, defect, P2)






(Reporter: beltzner, Assigned: dbaron)




(Keywords: regression, testcase)


(5 files)

Attached image top fx3, bottom fx2
(summary could probably use a morph from someone smarter than me)

 - Go to URL above
 - look at the red line

 - red line goes across entire page

 - red line only goes as far as the table content above it (the image)

This is a regression from Firefox 2. Not sure how serious it is. 

The relevant HTML from the page is:

  <table width="100%" border="0" cellpadding="2" cellspacing="0">
  <td><img width="1" height="2" alt="" /></td>
  <td valign="top" width="1%">
  <a href=''>
  <img src='youtube/logo_youtube.gif'
               alt="Google" />

  <td height="15px">

<!-- this is the td that's no longer drawing properly -->

  <td bgcolor="#ff3333" height="4px">

<!-- this is the td that's no longer drawing properly -->


  <td colspan="2"><img width="1" height="5" alt="" /></td>
  <table border="0" width="90%" align="center" cellpadding=5 cellspacing=1>
  <td width="75%" valign="top">
  <div style="font-size:83%;">

Sign Up for YouTube with Your Google Account
New to YouTube?
YouTube is a way to get your videos to the people who matter to you.
With YouTube you can:
  Upload, tag and share your videos worldwide

  Browse millions of original videos uploaded by community members
  Find, join and create video groups to connect with people with similar interests
  Customize your experience with playlists and subscriptions

  Integrate YouTube with your website using video embeds or APIs
Sign in with your Google account, and sign up with YouTube!
  <td style="padding-left: 10px;" valign="top" align="center">
I am reducing it
Attached file reduced testcase
Attached file more reduced
we give space to the virtual column that is created by the colspan=2 on the last row: In the good old days we had a special algorithm avoiding just this. It got removed when the reflow branch landed.
Flags: blocking1.9?
Searching for effective columns on 1.8 branch will show how much effort we spent before on this. (IE, safari, FF2 make the bar wide, FF3 and opera not).
Keywords: testcase
(fwiw, this was reported to me by "emo" in #granparadiso!)
Summary: td element with bgcolor no longer paints along full width of table → [effective columns] td element with bgcolor no longer paints along full width of table
Priority: -- → P2
+'ing; renom/comment if someone disagrees..
Flags: blocking1.9? → blocking1.9+
Effective columns for wrongly designed tables (colspans that span into the void) is a concept that is not  mentioned at . Introducing them will make the code certainly more complicated. Its a question to dbaron: Do we really want this, my impression from the careful removal of any traces of this is: no.
Well, it seems like our current behavior is honoring what the author asked for more accurately than the old code was.  Given that the author created a table with 2 columns, we're sizing them to match the constraints given, rather than forcing them to 0 just because no cells originated in the last column.

That said, the removal wasn't intentional (I rewrote the code after reverse engineering it after giving up on trying to understand it), but I don't see why the code in question is desirable.

That said, is it really this special behavior for columns only created by being spanned into that's causing the differences between us and other browsers?  It looks like this is a regression since the reflow branch landing, and probably relatively recent.  An actual regression range might be useful here.  I suspect this is a regression from one of the more recent changes dholbert made.
>So looks like bug 413286 is what led to the change in behavior.

It did backing  out the changes makes the column wide again.
 *    b. (NOTE: this case is for BTLS_FINAL_WIDTH only) otherwise, if any
 *   columns without a specified coordinate width or percent width have
 *   zero pref width, equally between these [numNonSpecZeroWidthCols]

this exactly happening here.
So, perhaps we need a special sub-case under "b" as mentioned in Comment 13, to prevent us from expanding empty cells from bogus colspans?

i.e. maybe we'd like to ignore (*not* expand) the "implied" empty cell here:
   <tr><td bgcolor="#ff3333" width="1%" height="4px"></td></tr>
   <tr><td colspan="2"></td></tr>

...but we would NOT want to ignore the explicit empty cell here, per bug 413286:
   <tr><td bgcolor="#ff3333" width="1%" height="4px"></td><td></td></tr>
   <tr><td colspan="2"></td></tr>
so in this second case, the empty cell still *should* expand.
Do other browsers make the bogus colspans distinction?  Or do they simply not have the behavior at all?  (Be careful to test with cellpadding=0, to keep empty cells having zero width.)

Why would it need to be a sub-case rather than an additional condition for the existing case?
(In reply to comment #15)
> Why would it need to be a sub-case rather than an additional condition for the
> existing case?

No reason -- I think that's really what I meant to say.
In the old layout there was a thing that I called virtual columns, columns without a originating cell. We suppressed them without mercy. There are however issues with this, see bug 187550. 

Testing for columns without a originating cell would remove the special casing.
This is attachment 312522 [details], with a cell added in the second column.  This testcase regressed in the same regression range as attachment 312522 [details].
OK, given that IE7 shows a difference between these two testcases, it seems like the right thing to do here is to condition the growing done by the fix for bug 413286 (i.e., the FLEX_FLEX_LARGE_ZERO pass) on mTableFrame->GetNumCellsOriginatingInCol(col) returning nonzero.
(I should be able to cook up a patch tomorrow if nobody beats me to it.)
Vlad/DBaron why is this a blocker?
Well, it's a layout regression from a pretty recent change (we may not entirely know what it broke yet), and it should be fixable by partially reverting that change (for one very specific case).  Probably not truly a blocker, though.
Assignee: nobody → dbaron
Attached patch patchSplinter Review
This makes us match WinIE on the original URL, on attachment 312519 [details], attachment 312522 [details], and attachment 314048 [details].  This patch includes variants of the last two as reftests (with == tests against references for both, and != between them).

Ran reftests -- no unexpected unexpecteds (though the usual noise on my machine).
Attachment #314266 - Flags: superreview?(roc)
Attachment #314266 - Flags: review?(dholbert)
Attachment #314266 - Flags: superreview?(roc) → superreview+
Attachment #314266 - Flags: review?(dholbert) → review+
Checked in to trunk.
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.