Closed Bug 371925 Opened 13 years ago Closed 13 years ago

Rendering problems for tfoot

Categories

(Core :: Layout: Tables, defect)

1.8 Branch
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: info, Assigned: bzbarsky)

References

Details

(Keywords: regression, verified1.8.0.11, verified1.8.1.3)

Attachments

(5 files)

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.
Attached file Code that doesn't work
Attached file Code that works
(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)
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.
Status: UNCONFIRMED → NEW
Component: General → Layout: Tables
Ever confirmed: true
Keywords: regression
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout.tables
Hardware: PC → All
Version: unspecified → 1.8 Branch
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.
Blocks: 337124
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.
Attached patch Trunk fixSplinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #256656 - Flags: superreview?(roc)
Attachment #256656 - Flags: review?(bernd_mozilla)
All that's different here is the context.
Attached patch ReftestsSplinter Review
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.
Attachment #256656 - Flags: review?(bernd_mozilla) → review+
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
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?
> 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.
(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...
Yeah, the revision numbers listed in the diff are still the branch ones...
Comment on attachment 256656 [details] [diff] [review]
Trunk fix

r+ on the remainder
Attachment #256656 - Flags: superreview?(roc) → superreview+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 256657 [details] [diff] [review]
Branch version of the fix

We should fix this regression on the branches too...
Attachment #256657 - Flags: approval1.8.1.3?
Attachment #256657 - Flags: approval1.8.0.11?
Flags: blocking1.8.1.3?
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...
Indeed
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.3+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.11+
Comment on attachment 256657 [details] [diff] [review]
Branch version of the fix

a=mconnor on behalf of 1.8 drivers for branch landing
Attachment #256657 - Flags: approval1.8.1.4?
Attachment #256657 - Flags: approval1.8.1.3+
Attachment #256657 - Flags: approval1.8.0.12?
Attachment #256657 - Flags: approval1.8.0.11+
Fixed on the branches.
Boris, would it suffice to run the test case here to verify this bug?
I think so, yes.
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

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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.