1220621 - table cell (td or th) not filling table row, after appending cells to tr with js (table-layout: fixed)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Omu]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

you can try both of these links
first for thead th:
http://jsbin.com/cemiwujoxa/1/edit?html,css,js,output
second for tbody td:
http://jsbin.com/pahanajaco/1/edit?html,css,js,output

I'm filling tr with td from 10 cells, emptying the tr and adding 3 after a small delay



Actual results:

cells don't fill the row, they take the smallest width possible in the left side

(only in ff, my version 41.0.2 windows 7 x64, i7 2670qm)


Expected results:

cells should fill the table row

Comment 1

[Omu]

just noticed that emptying colgroup first and thead after will make it behave properly

Comment 2

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

Attachment: jsbin.cemiwujoxa.1.html

Took me a minute to find File -> Download in the JSBin UI

Comment 3

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

Attachment: jsbin.pahanajaco.1.html

Comment 4

[Boris Zbarsky [:bzbarsky]]

Thank you for the bug report.

The basic problem is that when we remove a <col> the table code replaces its colframe with an anonymous one.  See the AppendAnonymousColFrames(1) call in nsTableFrame::RemoveCol.  That makes sense if we would now have fewer columns than we have cells in a row, but doesn't make sense in situations like this one where we have more columns than cells in a row.

Even then, in auto table layout it ends up not mattering because the empty columns just get 0 width.  But in fixed layout they do get space.

Working on a fix.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: When removing a <col>, only create an anonymous colframe to replace it if one is really needed

Comment 6

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

Comment on attachment 8687511 [details] [diff] [review]
When removing a <col>, only create an anonymous colframe to replace it if one is really needed

>+        // All of our colframes correspond to actual <col> tags.  It's possible
>+        // that we still have more of them than we do cells, but we might have
>+        // one less.  Handle the latter case by first asking the cellmap to drop
>+        // its last col if it doesn't have any actual cells in it and then
>+        // calling MatchCellMapToColCache to handle the case when there _were_
>+        // cells in it and hence we need to add an anonymous col after all.

It took me a while to understand this comment.

First (but perhaps less importantly), I think:
  still have more of them than we do cells, but we might have one less.
could be:
  still have at least as many <col> tags than logical columns from cells,
  but we might have one less.


More importantly, maybe split the last sentence in half by inserting a
period before "and then", and then restructure the second half to be:

  Then call MatchCellMapToColCache to append an anonymous column if it's
  needed; this needs to be after RemoveColsAtEnd since it will determine
  need for a new column frame based on the width of the cell map.

r=dbaron


I'm curious whether RemoveColsAtEnd handles colspan="0" (span all
columns) correctly; I suspect it doesn't, though haven't tested.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed up the comments to make them more readable.

> I'm curious whether RemoveColsAtEnd handles colspan="0" (span all
> columns) correctly; I suspect it doesn't, though haven't tested.

Code inspection suggests that it does not in that the last column will have a nonzero mNumCellsSpan.  Want a followup bug on that?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and the updated comment says:

        // All of our colframes correspond to actual <col> tags.  It's possible
        // that we still have at least as many <col> tags as we have logical
        // columns from cells, but we might have one less.  Handle the latter
        // case as follows: First ask the cellmap to drop its last col if it
        // doesn't have any actual cells in it.  Then call
        // MatchCellMapToColCache to append an anonymous column if it's needed;
        // this needs to be after RemoveColsAtEnd, since it will determine the
        // need for a new column frame based on the width of the cell map.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50c1b2cb6ad1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50c1b2cb6ad1

Comment 11

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

(In reply to Boris Zbarsky [:bz] from comment #7)
> > I'm curious whether RemoveColsAtEnd handles colspan="0" (span all
> > columns) correctly; I suspect it doesn't, though haven't tested.
> 
> Code inspection suggests that it does not in that the last column will have
> a nonzero mNumCellsSpan.  Want a followup bug on that?

I filed bug 1225901.