[FIX]IsTableRelated check in GetAbsoluteContainingBlock is incorrect

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

The IsTableRelated check in GetAbsoluteContainingBlock checks for table-related display types and skips frames that have them.  However, when calling ProcessChildren the push or not of the parent as an absolute containing block depends on the code being used to construct the parent.  This means that we end up with different absolute containing blocks on the two codepaths.  Test that shows this coming up, then a fix.
Created attachment 362651 [details]
Testcase
Created attachment 362652 [details]
Reference
Created attachment 362654 [details] [diff] [review]
I think this is the right check
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #362654 - Flags: superreview?(roc)
Attachment #362654 - Flags: review?(bernd_mozilla)
Attachment #362654 - Flags: superreview?(roc) → superreview+

Updated

9 years ago
Attachment #362654 - Flags: review?(bernd_mozilla) → review+

Comment 4

9 years ago
Comment on attachment 362654 [details] [diff] [review]
I think this is the right check

6999     // being table-related frames are not containers for absolutely
7000     // positioned child frames.
7001     const nsStyleDisplay* disp = frame->GetStyleDisplay();
7002 
7003     if (disp->IsPositioned() && !IsTableRelated(disp->mDisplay, PR_TRUE)) {

we have this code in our tree already??? ;-)

Comment 5

9 years ago
since 2005 https://bugzilla.mozilla.org/attachment.cgi?id=198046&action=diff

bug 307076
The code you cite is checking mDisplay.  We want to be checking the frame type.

Thanks for the pointer to bug 307076, btw.  I'll make a testcase out of that.
Pushed http://hg.mozilla.org/mozilla-central/rev/37405bb5ffd3
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 8

9 years ago
It would be cool if the patch would be against trunk and not a intermediate state of your tree, which I am not aware off, which is also not referenced in this bug. The relevant bug is  478754. I am not sure how a review can be accomplished under those conditions. I though that I review a expansion towards the remaining table types. Which was wrong. Nevertheless the patch is correct.
Er, sorry.  I had in fact forgotten that this applied on top of bug 478754...

Thanks for double-checking the patch, by the way.  This stuff is ... delicate.
Depends on: 503364
You need to log in before you can comment on or make changes to this bug.