Bug 3702 – Web page is laid out incorrectly after printing with print-specific style sheet

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[John Sullivan]

This is also in Radar as <rdar://problem/3706259>

To reproduce:
1. Load the following web page into Safari: <http://cgi.bisc.udel.edu/safari/>
2. Select File > Print... (or hit Command-P)
3. Click the Preview button (or Print)
4. After the Print dialogue box disappears, drag the scrollbar up and down on the page.

Most of the content is squeezed into a thin area on the left side of the page, instead of being the same 
width as it was before printing.

[Alexey Proskuryakov]

Looks like some elements get "display: none" from the printing stylesheet, and never gain their width back... Possibly because of the following:

table.main {
	table-layout: fixed;
	width: 725px;
<...skipped...>
}

[Alexey Proskuryakov]

Created attachment 5805 [details]
reduced test case

This reduced test case only works in ToT, because it uses AddRule/DeleteRule, but the problem appears to be the same as with printing.

[Alexey Proskuryakov]

Created attachment 5809 [details]
proposed fix

[Darin Adler]

Comment on attachment 5809 [details]
proposed fix

This doesn't look right to me.

This code is iterating all the columns in one of the table sections, but then it's cutting back the number of columns in the table itself. But there can be other table sections, and they may have different numbers of columns.

Also, it's not good design to have the table section resizing the table's column data structures. The table should maintain the size of its own. And a function named ensureRows should not change the columns data structure around.

Finally, I don't understand how this fixes the bug -- what goes wrong without this code? How are the column data structures supposed to be set up by RenderTable? What's going wrong with that in this case?

[Alexey Proskuryakov]

> But there can be other table sections, and they may have different numbers of columns.

  The standard explicitly says "The THEAD, TFOOT, and TBODY sections must contain the same number of columns", but browsers apparently do not obey :(. I'll have to rework the fix.

>And a function named ensureRows should not change the columns data structure around.

  Hmm, I didn't change anything in ensureRows().

> Finally, I don't understand how this fixes the bug

  When cells are added to the table (e.g. during initial parsing, via appendChild or by unsetting display: none), RenderTableSection::addCell() is called. It is obviously optimized for initial parsing, and simply adds new cells to the last row of the grid, calling appendColumn() as necessary. Later, recalcCells() rearranges this havoc - but it doesn't undo the effect of appendColumn().

  So, FixedTableLayout::layout() thinks that there are more columns in the table than there actually are, and reserves space for those extra columns.

This is how the grid looks in this test like after recalcCells (without my fix):
RenderTableSection: 
  grid=(2,8)
  (0,0,1,1) (0,1,1,1) (0,2,1,1) (0,3,1,1) 0x0null cell 0x0null cell 0x0null cell 0x0null cell 
  0x0null cell 0x0null cell 0x0null cell 0x0null cell 0x0null cell 0x0null cell 0x0null cell 0x0null cell 

[Alexey Proskuryakov]

Created attachment 5841 [details]
revised fix

[Darin Adler]

Comment on attachment 5841 [details]
revised fix

The way the new recalcSections code iterates the sections is incorrect and unnecessarily complicated. Instead, iterate all children (starting with the first child, not firstBody), call the loop variable child instead of body, handle all table sections, and don't special case the head or the foot.

Other than this complaint, the patch looks great!

(Patch has some tab characters in it. Please don't check in with tabs.)

[Alexey Proskuryakov]

Created attachment 5844 [details]
revised fix

> Instead, iterate all children (starting with the
> first child, not firstBody), call the loop variable child instead of body,
> handle all table sections, and don't special case the head or the foot.

  /me blushes and hopes he didn't overlook something equally stupid this time :(

> (Patch has some tab characters in it. Please don't check in with tabs.)

  These files have a lot of tabs, would you like me to replace them all when committing?

[Darin Adler]

(In reply to comment #8)
> These files have a lot of tabs, would you like me to replace them all when
> committing?

I think it's OK to do that; it will mess up history a bit, but I think it's worth it. If you do it, you should also remove the "allow-tabs" keyword: "svn pd allow-tabs <filename>".

[Darin Adler]

Comment on attachment 5844 [details]
revised fix

r=me