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

Status: RESOLVED FIXED

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

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: top fx3, bottom fx2

(summary could probably use a morph from someone smarter than me)

STR:
 - Go to URL above
 - look at the red line

Expected:
 - red line goes across entire page

Actual:
 - 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">
  <tr>
  <td><img width="1" height="2" alt="" /></td>
  </tr>
  <tr>
  <td valign="top" width="1%">
  <a href='http://www.youtube.com/'>
  <img src='youtube/logo_youtube.gif'
               border="0"
               align="left"
               alt="Google" />
  </img>

  </a>
  </td>
  </tr>
  <tr>
  <td height="15px">
  </tr>
  <tr>

<!-- 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 -->

  </tr>

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

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

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

  Integrate YouTube with your website using video embeds or APIs
  </li>
</ul>
<b>
Sign in with your Google account, and sign up with YouTube!
</div>
  </td>
  <td style="padding-left: 10px;" valign="top" align="center">

Comment 1

[Bernd]

I am reducing it

Comment 2

[Bernd]

Attachment: reduced testcase

Comment 3

[Bernd]

Attachment: 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.

Comment 4

[Bernd]

http://developer.mozilla.org/en/docs/Table_Layout_Strategy#Effective_columns

Comment 5

[Bernd]

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).

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

(fwiw, this was reported to me by "emo" in #granparadiso!)

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

+'ing; renom/comment if someone disagrees..

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bernd, can you take this?

Comment 9

[Bernd]

Effective columns for wrongly designed tables (colspans that span into the void) is a concept that is not  mentioned at http://dbaron.org/css/intrinsic/#col-s-min . 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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

It looks like this regressed between 2008-02-21-01 and 2008-02-22-01.  The checkins:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-21+01&maxdate=2008-02-22+01&cvsroot=%2Fcvsroot

So looks like bug 413286 is what led to the change in behavior.

Comment 12

[Bernd]

>So looks like bug 413286 is what led to the change in behavior.

It did backing  out the changes makes the column wide again.

Comment 13

[Bernd]

 *    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.

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 16

[Daniel Holbert [:dholbert]]

(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.

Comment 17

[Bernd]

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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: variant of "more reduced" with cell in second column

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].

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I should be able to cook up a patch tomorrow if nobody beats me to it.)

Comment 21

[Mike Schroepfer]

Vlad/DBaron why is this a blocker?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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).

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.