286797 - [BC] some style changes (color) do not trigger update of border chunks (see comment #9)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Julien "_FrnchFrgg_" RIVAUD]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.6) Gecko/20050306 Firefox/1.0.1 (Debian package 1.0.1-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.6) Gecko/20050306 Firefox/1.0.1 (Debian package 1.0.1-2)

When a "last resort" choice is applied for a border in the collapsed border
model (only color is different), whichever color is choosen, the border should
be wholly in one single color. It is not the case.

Furthermore, if set dynamically, painting problems arise, that is the choice of
color isn't even the same for a given part of borders, depending on the
invalidate region.

See testcase to fully understand what is meant.

Sorry if it is a duplicate, I searched a lot to seek a similar bug, but there
are a lot of painting bugs and I wasn't able to find one that I could detect as
certainly related.

Reproducible: Always

Steps to Reproduce:
Follow instructions in the testcase
Actual Results:  
See testcase

Expected Results:  
See testcase

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Detailed testcase of comment #0

Testcase spoken of in comment #0, as promised :o)

Comment 2

[Julien "_FrnchFrgg_" RIVAUD]

Same bug exists also under Windows XP.

Comment 3

[Julien "_FrnchFrgg_" RIVAUD]

Feel free to set a more fitting (or appaling) title for this bug, as I am still
not used to your habits.

_FrnchFrgg_

Comment 4

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Testcase with far less obscure description

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

This bug needs a more fitting title, since it primarily concerns collapsed
border model, with border conflict based solely on color, and set dynamically.
The title is obscure, and seems to cover a far wider problem, but I don't know
what to put here to be concise and comprehensive at the same time.

_FrnchFrgg_

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Another testcase, to see that the yellow bleeds along the whole table

This testcase shows that not only the neighbouring cell is concerned, but the
border from one end of the table to the other.

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

Bug shows both in quirks and standard mode.

Comment 8

[Julien "_FrnchFrgg_" RIVAUD]

Why is there a #undef DEBUG_TABLE_CELLMAP at the beginning of layout/tables/nsTableCell.h ? It makes impossible the use of
ac_add_options --enable-debug=-DDEBUG_TABLE_CELLMAP,
which I find far more practical and clean than modifying the source.

Is there a good reason ?

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

I found something interesting :
In nsTableFrame::PaintBCBorders, a BCMapBorderIterator is used to loop through the border chunks of the table.

Those border chunks are grouped together when they are following each other, with the same style (size, color, etc).

The problem of dynamic part of attachment 195735 [details] is that the change of border color does not trigger a change in the way the border chunks are grouped. Thus the whole border will have a single color, which is the one exposed by the leftmost (or topmost) border chunk that intersects the dirty area.

Comment 10

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Screenshot showing chunks layout

I modified my build so that it used a unique color for each (grouped) chunk.

The screenshot (attachment) is what I get, regardless of whether I have clicked on the body or not.

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

If I force a CalcBCBorder() before any border paint (at the beginning of PaintBCBorders, for instance), then the dynamic testcase behaves exactly like the static one. (still wrong, but for another reason).

Comment 12

[Julien "_FrnchFrgg_" RIVAUD]

(In reply to comment #11)
> If I force a CalcBCBorder() before any border paint (at the beginning of
> PaintBCBorders, for instance), then the dynamic testcase behaves exactly like
> the static one. (still wrong, but for another reason).
> 

Correction : Before the call to CalcBCBorder, I call SetBCDamageArea on (O, O, GetColCount(), GetRowCount()). Without, nothing happens (regarding this bug).

I hope that helps, even if I may not be able to find the right place to add an update of BC borders.

Comment 13

[Julien "_FrnchFrgg_" RIVAUD]

Static case bug seems to be bug 325074.

Comment 14

[fantasai]

Attachment: another testcase

Not sure, but this testcase might be suffering from the same problem.

Comment 16

[Bernd]

Attachment: patch

Boris, I feel somehow embarrassed that I strong armed with you at https://bugzilla.mozilla.org/show_bug.cgi?id=258377#c52 ff

There is is this http://mxr.mozilla.org/mozilla-central/source/layout/tables/celldata.h#480
SetTopStart thing. Basically as I said color does not affect the winner, but I missed that it effects in our painting where a segment starts. This is wrong by design especially for dynamic cases like here.

Once the painting is sane, one could remove this and revert to the initial patch that you reviewed in bug 258377. However given the glacial speed in layout tables I would rather go with this now.

Comment 17

[Bernd]

s/dependence/dependency/ in the patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 345920 [details] [diff] [review]
patch

File a bug on making this better and mark it dependent on FrenchFrgg's rewrite?

Comment 19

[Bernd]

Attachment: patch for the upper left corner in the second table

This is so brain damaged, that I just post the patch, I thought it might be something easy. This thing should be rewritten, the patch adapts the bitfield width to the content width http://mxr.mozilla.org/mozilla-central/source/layout/tables/celldata.h#177 its difficult to stuff 10 into 3 bits.... OMG.

Comment 20

[Bernd]

but then 33 bits do not fit into a 32 PRUInt32....

Comment 21

[Bernd]

I checked in the fix for the dynamic case http://hg.mozilla.org/mozilla-central/rev/1381d3b9ce74, I filed the followup bug 452319. That keeps track of the backout of this code. I attached the 2 table from https://bugzilla.mozilla.org/attachment.cgi?id=195735 to the static bug 325074.