371925 - Rendering problems for tfoot

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Plucera]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

Using code without new line between <table> and following <thead> causes dynamic styles for t-foot not to render. Adding a new line causes it to work.. 

Reproducible: Always

Steps to Reproduce:
1.Use test case
2.
3.
Actual Results:  
Tfoot hides -> shows -> no change

Expected Results:  
Tfoot hides -> shows -> hides -> shows etc.

Comment 1

[Plucera]

Attachment: Code that doesn't work

Comment 2

[Plucera]

Attachment: Code that works

Comment 3

[Plucera]

(In reply to comment #0)
May add that this was discovered with the last release 2.0.0.2 and 1.5.0.10. Previous releases didn't show this error (though not tested)

Comment 4

[Ria Klaassen (not reading all bugmail)]

This appears to work in trunk.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

Comment 5

[Mats Palmgren (inactive)]

I can confirm that it works in Firefox 1.5.0.9 but not in 1.5.0.10,
and  that it works in Firefox 2.0.0.1 but not in 2.0.0.2. All on Linux.

Comment 6

[Martijn Wargers (dead)]

In the trunk build, the table gets higher, which also doesn't seem correct. I guess a new bug should be filed on that?

This regressed on branch between 2007-01-11 and 2007-01-15, which is when the branch patch for bug 337124 was checked in.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the problem is that we end up (bogusly) with a null aPrevSibling, which means we put the tfoot at the end of the list... and then this confuses GetPrimaryFrameFor() later so we never find a frame for it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk fix

So the major problem was that we started lastIndex at 0, which meant that if the correct prevsibling is at index 0 we didn't find it (since the comparisons use a strict '>').  Starting that off at -1 fixes the bug.

The other change just seems like a good idea, though -- we really shouldn't be even looking at the indices if aPrevSibling is already ok, and the test that was used to tell whether it's ok was too conservative, imo.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch version of the fix

All that's different here is the context.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Reftests

Comment 11

[Bernd]

Comment on attachment 256656 [details] [diff] [review]
Trunk fix

that PRInt32 lastIndex = 0; is embarrassing.


>whether it's ok was too conservative

hmm, do you expect nonconservative patches from me??? you will probably not get them ;-)

I fail to see why is that to conservative. r+ on the embarrassing part but please provide a reason for the other change. I don't see the improvement from it.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 256656 [details] [diff] [review]
Trunk fix

>RCS file: /cvsroot/mozilla/layout/tables/nsTableFrame.cpp,v
>retrieving revision 3.616.12.7
>diff -u -p -d -8 -r3.616.12.7 nsTableFrame.cpp

Boris, did you attach the wrong trunk patch?
This looks like a diff from a 1.8 branch tree?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I fail to see why is that to conservative. 

In this case the two frames involved have display values table-footer-group and table-header-group respectively.  Those are different, so we end up doing all that index stuff. But they're supposed to end up on the same frame list anyway, so we don't need to do it (and arguably shouldn't, since the index stuff will give the wrong answer in some cases).

> This looks like a diff from a 1.8 branch tree?

It was; then I hand-edited it to apply to trunk reasonably cleanly.

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #13)
> > This looks like a diff from a 1.8 branch tree?
> 
> It was; then I hand-edited it to apply to trunk reasonably cleanly.

I used the 'diff' link to look at the surrounding context and was
confused to see branch code...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah, the revision numbers listed in the diff are still the branch ones...

Comment 16

[Bernd]

Comment on attachment 256656 [details] [diff] [review]
Trunk fix

r+ on the remainder

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 256657 [details] [diff] [review]
Branch version of the fix

We should fix this regression on the branches too...

Comment 19

[Boris Zbarsky [:bzbarsky]]

mconnor asked for a risk assessment.  I think this is a very safe fix -- I can't see how it could regress the original crash, and it shouldn't have any other effects...

Comment 20

[Bernd]

Indeed

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 256657 [details] [diff] [review]
Branch version of the fix

a=mconnor on behalf of 1.8 drivers for branch landing

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed on the branches.

Comment 23

[juan becerra [:juanb]]

Boris, would it suffice to run the test case here to verify this bug?

Comment 24

[Boris Zbarsky [:bzbarsky]]

I think so, yes.

Comment 25

[Carsten Book [:Tomcat]]

verified fixed for 1.8.0.11 and 1.8.1.3 using the testcase and Builds:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.11pre) Gecko/20070308 Firefox/1.5.0.11pre
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.3pre) Gecko/2007030900 BonEcho/2.0.0.3pre

Tfoot hides -> shows -> hides -> shows etc. - and is working fine

Comment 26

[Martijn Wargers (dead)]

I filed bug 373327 for the issue I mentioned in comment 6, which is a trunk only issue.
I can confirm this bug is fixed for me on the latest 1.8.1 branch build.