Closed Bug 478811 Opened 11 years ago Closed 11 years ago

[FIX]IsTableRelated check in GetAbsoluteContainingBlock is incorrect

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

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.
Attached file Testcase
Attached file Reference
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #362654 - Flags: superreview?(roc)
Attachment #362654 - Flags: review?(bernd_mozilla)
Attachment #362654 - Flags: superreview?(roc) → superreview+
Attachment #362654 - Flags: review?(bernd_mozilla) → review+
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??? ;-)
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
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.