Closed Bug 291854 Opened 15 years ago Closed 11 years ago

[FIX]Push float containing blocks inside ProcessChildren

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 3 obsolete files)

Right now we have code all over the place to push float containing blocks.  We
should really just do this inside ProcessChildren (and probably inside
CreateAnonymousContent).
Blocks: 85872
Attached patch First cut at a patch (obsolete) — Splinter Review
Untested so far; need to test tables.  Also, this has fieldset issues and major
XTF issues.
Afri, you know this XTF stuff, right?  Could you please look at the XXX comments
near the end of the patch and let me know what you think?
(In reply to comment #2)
> Afri, you know this XTF stuff, right?  Could you please look at the XXX comments
> near the end of the patch and let me know what you think?

Sorry, I don't really understand the insertionFrame comment either. This is
bryner's code (bug#269023). 
I asked bryner about this yesterday and he said that checkin was just preserving
the existing algorithm as much as it could and that he didn't know why it was
that way to start with...  And indeed, the code used to just put the kids in the
primary frame for the insertion content.  This probably crashed if the anon
content was an animated image and the insertion content was a table cell, as it
happens.  That should certainly crash right now, as the code is written.
Blocks: 282173
One other thought I had was that perhaps the processing of anonymous kids should
just be done at the end of ProcessChildren so it gets the containing block love,
in case the anon kids happen to be floated.
Depends on: 472138
OS: Linux → All
Hardware: x86 → All
Summary: Push float containing blocks inside ProcessChildren → [FIX]Push float containing blocks inside ProcessChildren
Attached patch Fix (obsolete) — Splinter Review
OK, so the following things have improved since the first cut:

1)  XTF frame construction went away
2)  We fixed bug 85872, which made it easy to make this work because it's OK to
    pass in the inner frames of table cells and fieldsets, as we do now, to
    ProcessChildren.

I've made sure this passes reftests and doesn't add any crashtest asserts.
Attachment #181809 - Attachment is obsolete: true
Attachment #355433 - Flags: superreview?(roc)
Attachment #355433 - Flags: review?(roc)
+   * @param aFrame the frame to use as the parent frame for the new in-flow
+   *        kids. Note that this need not be its own content insertion frame,

When can aFrame not be its own content insertion frame?
Oh, hmm.  I thought that could happen with ContentInserted/ContentAppended, but we do make sure to return the insertion frame for those.

Given that, I suspect that this is always the content insertion frame.  I'm happy to document and assert that.
Attached patch With that change (obsolete) — Splinter Review
Attachment #355433 - Attachment is obsolete: true
Attachment #355466 - Flags: superreview?(roc)
Attachment #355466 - Flags: review?(roc)
Attachment #355433 - Flags: superreview?(roc)
Attachment #355433 - Flags: review?(roc)
Attachment #355466 - Flags: superreview?(roc)
Attachment #355466 - Flags: superreview+
Attachment #355466 - Flags: review?(roc)
Attachment #355466 - Flags: review+
And this passes crashtest/reftest
Attachment #355466 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/5d99fcca32ab
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.