226593 - [FIXr]Dynamic changes to rules attribute do not work

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Testcase coming up.  Using a reframe hint for mRules changes would probably
work, but that really should not be necessary, should it?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

setAttribute(rules'

is that missing ' intentional?

Comment 3

[Boris Zbarsky [:bzbarsky]]

No, it's not... it's a typo.  The rest of the testcase is not affected by that,
though.

Comment 4

[Bernd]

Believe that the testcases at 
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/collapsing_borders/bug41262-2.html
have worked once, this looks like a regression.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Indeed... Looks like changing the "rules" attribute in fact triggered a
framechange hint before the checkin for bug 211308.  See
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHTMLTableElement.cpp&branch=&root=/cvsroot&subdir=mozilla/content/html/content/src&command=DIFF_FRAMESET&rev1=1.123&rev2=1.124

Comment 6

[Bernd]

setting the rules attribute invokes border collapse.

Comment 7

[Bernd]

we need to frame change see: 
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.cpp#1211

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This does fix the bug...

"Invokes border-collapse" is not sufficient here -- there is a rules attr on
the table when the page is loaded (albeit an empty one), so the table starts
out in the collapsing borders model.

So why is a reframe required at all?  We're already doing border-collapse:
collapse.  So changing the rules should be a matter of repainting the borders,
no?  I checked, and the post-resolve callbacks in nsHTMLStyleSheet are being
called and the border data is being reset in ProcessTableRulesAttribute for the
new style context....  do tables somehow cache that border information
somewhere and not let go?  I couldn't find any accesses to mRules anywhere
outside the post-resolve callback code...

I suppose we can check this in if all else fails, but it'd be nice to know
what's going on.  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  I found it.  See nsTableFrame::Reflow -- it checks
NeedToCalcBCBorders() before recalculating them... and the BC border cache is
not invalidated by a change in mRules (since a simple style change reflow does
not invalidate it).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136213 [details] [diff] [review]
This does fix the bug...

I think this is the right patch.  The alternative is to blow away the BC border
cache on every style-change reflow, and that's prohibitively expensive, I would
guess... whereas dynamic changes in the "rules" attribute happen once in a blue
moon.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Taking.  I think we should take this for 1.6... not sure about the state of
1.6b, though.

Comment 12

[Bernd]

Boris could you also move the comment from 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/src/nsHTMLTableElement.cpp&mark=1490#1485
in to your patch? The align caused also a FrameChange hint, is that still the
case or is that another opportunity for a possible regression?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I actually did add a comment to that effect in my patch... I should probably
remove it from nsHTMLTableElement.  Note that the comment in nsHTMLTableElement
is NOT correct, since here we have a case that requires a reframe even though
border-collapse is already in effect.

Align is just mapped into floating, which causes a reframe when turned on/off as
it is.  So that part is fine.

Comment 14

[Bernd]

Comment on attachment 136213 [details] [diff] [review]
This does fix the bug...

just remove the comment from the table element, btw the comment is "exactly"
saying that unfortunetaly this is necessary even if the table is already bc,
(You might need some special education	to be able to get the deeper meaning of
Chris comments, but I got that over the years)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136213 [details] [diff] [review]
This does fix the bug...

Could this please be approved for 1.6?	This fixes a regression in dynamic
changes to the "rules" attribute.  This is a very safe patch -- it just makes
us reconstruct the table layout object when "rules" changes.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 136213 [details] [diff] [review]
This does fix the bug...

a=[e-mail] for 1.6b.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.6b

Comment 18

[Gérard Talbot]

Is this bug (and its fix) having an impact on bug 155507 or maybe bug 172213?
Just asking. Thanks.

Comment 19

[Boris Zbarsky [:bzbarsky]]

No.

Comment 20

[Gérard Talbot]

The table rules are not visible when when clicking the "All borders" button of
attachment 136185 [details]. Seamonkey 1.1a rv: 1.9a1 build 2005092405 under XP Pro SP2 here.

Comment 21

[Boris Zbarsky [:bzbarsky]]

That's a regression from the fix for bug 155507.  Could you please file a bug
and cc me, dbaron, and bernd on it?

Comment 22

[Gérard Talbot]

Done. I filed bug 310100.

Comment 23

[Gérard Talbot]

I have filed bug 310127. Is it ok if I cc you 3 on it? Just asking...