285034 - Collapsing table borders don't paint consistently

Status: RESOLVED WONTFIX

(Core :: Layout: Tables, defect)

Description

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

When PaintBCBorders uses the damage rect to find the row and column to start
painting at, we get into trouble with painting looking different depending on
what the damage rect is. I think this is partly to do with spanning cells, but I
haven't fully debugged it.

Always starting damageArea at the top left of the table fixes the bug. IMHO this
optimization isn't worth it. I'll attach a patch to turn it off. If you think
the optimization is worth it, then feel free to fix the underlying problem :-).
The problem shows up in several testcases, e.g.
layout/html/tests/table/marvin/backgr_position-table.html

Comment 1

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

Attachment: fix

This is a simple fix, the only question is whether it's really OK to just turn
off this optimization. Note that we still get the benefit of stopping painting
when the rows or columns go beyond the damage rect, so painting the top of a
huge table is still no problem.

I'm also correcting the erasure of verInfo to erase the whole allocated array.
Actually I suspect, looking at the code, that verInfo could just be length
damageRect.width and the last element is not currently used, but I'm not 100%
sure.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176498 [details] [diff] [review]
fix

sr=bzbarsky if bernd oks this.

Comment 3

[Bernd]

This will make scrolling slower. As we will not only paint a small strip but the
whole table if I understand it correctly. There is pretty much effort in the
code to get the damage area to avoid just this.

Comment 4

[Bernd]

> IMHO this optimization isn't worth it.
Why do we have a paint damage area then at all in our code? 

The correct patch, instead of  hacking around without proper commenting why this
is necessary, would be to paint the table cell borders by the table cells.

If you feel that you need this for your testing, feel free to fix this, this
would really help.

I will not have time for this, as I will be busy on a foreseeable future with
crash fixes.

Comment 5

[Bernd]

I mean I can help you with this but are not able to fix this by myself in a
foreseeable future. I am somehow reluctant to pass hacks into the code just to
make a test/compiler happy. If you think that you *really* need this hack
instead of a fix then please comment: 
a) that it is a hack
b) why we need this hack
c) what the proper fix would require

Comment 6

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

Comment on attachment 176498 [details] [diff] [review]
fix

alright, I don't really need this.