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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

FWIW this assert was added due to a request from bz here:

https://bugzilla.mozilla.org/show_bug.cgi?id=666041#c45

Comment 3

[Daniel Holbert [:dholbert]]

Maybe we should really move this assert *inside* of PushFloatContainingBlock()?  That would be extending its effect, but in a useful way.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

That seems better.

Comment 6

[Daniel Holbert [: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

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

(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.

Comment 8

[Jonathan Watt [:jwatt]]

bz: does it also make sense to add an assertion to PushFloatContainingBlock as dholbert suggested?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 809352 [details] [diff] [review]
patch

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Re comment 8: Probably a good idea, yes.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch for checkin [r=bz]

Comment 12

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8533f319940f

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8533f319940f