Closed Bug 920158 Opened 6 years ago Closed 6 years ago

Make nsCSSFrameConstructor::ConstructFrameFromItemInternal handle frames that should suppress floating of descendants.

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #809329 - Flags: review?(dholbert)
FWIW this assert was added due to a request from bz here:

https://bugzilla.mozilla.org/show_bug.cgi?id=666041#c45
Maybe we should really move this assert *inside* of PushFloatContainingBlock()?  That would be extending its effect, but in a useful way.
Ah.  Now I recall what the point of this assert is.  Thank you for the link in comment 2.

If ShouldSuppressFloatingOfDescendants() then pushing nothing or pushing the frame is wrong.  The right thing to do is to push null.

So if we can have such frames coming through here now we should remove the assert and do something more like what nsCSSFrameConstructor::ProcessChildren does.
Attached patch patch (obsolete) — Splinter Review
That seems better.
Attachment #809329 - Attachment is obsolete: true
Attachment #809329 - Flags: review?(dholbert)
Attachment #809346 - Flags: review?(dholbert)
Comment on attachment 809346 [details] [diff] [review]
patch

[I'm assuming you hadn't seen comment 4 yet when you posted this.]

Looks like we need to call  PushFloatContainingBlock(nullptr, ...) , basically matching the code here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9061
Attachment #809346 - Flags: review?(dholbert)
Attached patch patch (obsolete) — Splinter Review
(In reply to Daniel Holbert [:dholbert] from comment #6)
> [I'm assuming you hadn't seen comment 4 yet when you posted this.]

Indeed.

This could probably do with a code comment if the reason for this code is easily forgotten, but I will need some help on the wording if I'm to add one.
Attachment #809346 - Attachment is obsolete: true
Attachment #809352 - Flags: review?(bzbarsky)
bz: does it also make sense to add an assertion to PushFloatContainingBlock as dholbert suggested?
Comment on attachment 809352 [details] [diff] [review]
patch

r=me
Attachment #809352 - Flags: review?(bzbarsky) → review+
Summary: Move an assertion in nsCSSFrameConstructor::ConstructFrameFromItemInternal into the block to which it applies to avoid bogus failures → Make nsCSSFrameConstructor::ConstructFrameFromItemInternal handle frames that should suppress floating of descendants.
Attachment #809352 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8533f319940f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.