Closed Bug 718877 Opened 12 years ago Closed 12 years ago

Make nsContainerFrame::StealFramesAfter virtual, and provide an impl for nsBlockFrame

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: dholbert, Unassigned)

References

Details

MOTIVATION:
In flexbox layout, we'll have anonymous blocks to wrap continuous inline non-atomic content (e.g. stretches of text), and these anonymous blocks behave as flexbox items.

If a block-level element is inserted into the middle of a run of text, we need to split the anonymous block at that point, creating two anonymous blocks.

Conversely, if a block-level element is removed and there are anonymous wrapper-blocks on either side of it, we need to merge the two anonymous blocks into one.

I've got us detecting & correcting these issues inside of nsFlexboxFrame::Reflow (doesn't yet exist in m-c).  So, I basically need a way for the flexbox to remove some or all child frames from an anonymous block.

nsContainerFrame::StealFramesAfter *would* be exactly perfect for this, but it doesn't work for block frames right now.  It's only got one impl -- in nsContainerFrame.cpp -- and that impl complains if it's called on a block frame:
> 1255   NS_ASSERTION(GetType() != nsGkAtoms::blockFrame, "unexpected call");


SO -- WHAT I NEED:
I need a version of StealFramesAfter that works on nsBlockFrames.  From talking to mats briefly on IRC, I think we need to make StealFramesAfter virtual and add a custom impl in nsBlockFrame.  This custom impl will need to understand line lists (which the nsContainerFrame impl does not, of course).
(Right now, I've got a hackaround that effectively implements StealFramesAfter(someChild) via repeated calls to StealFrame() for someChild & its entire next-sibling chain.  This works, but it's got O(n^2) performance, because each StealFrame() call has to walk through our child frames from the beginning, so it's not a good solution long-term.  It could very well also have undesirable results in edge cases that I haven't run up against yet.)

(Note that StealFrame() is already virtual, and nsBlockFrame does already have a custom impl of it.  I imagine nsBlockFrame's StealFramesAfter() impl will look very much like the existing StealFrame() impl (ideally sharing a lot of code)).
(In reply to Daniel Holbert [:dholbert] from comment #0)
> I've got us detecting & correcting these issues inside of
> nsFlexboxFrame::Reflow (doesn't yet exist in m-c).  So, I basically need a
> way for the flexbox to remove some or all child frames from an anonymous
> block.

To be clear, the two key situations (and processes) are as follows.  In both situations, part (c) needs something like StealFramesAfter() to be exposed on nsBlockFrame.

(1) BLOCK INSERTION:
 a) Start out:
>       [ { textA textB } ]
 b) A block is inserted between textA & textB, resulting in this (temporary) frame state:
>       [ { textA BLOCK textB } ]
 c) nsFlexboxFrame::Reflow detects the BLOCK inside the anonymous flexbox item (bad), and
    corrects to the following, using a call to textA->parent->StealFramesAfter(BLOCK)
    (which would returns a nsFrameList containing BLOCK and textB, which we'd then munge)
>       [ { textA } BLOCK { textB } ]



(2) BLOCK REMOVAL (basically the reverse of the above):
 a) Start out:
>       [ { textA } BLOCK { textB } ]
 b) BLOCK is removed from the document, resulting in this (temporary) frame state:
>       [ { textA } { textB } ]
 c) nsFlexboxFrame::Reflow detects the two consecutive anonymous flexbox items (bad), and
    corrects to the following, using a call to textB->parent->StealFramesAfter(textB)
    (which would returns a nsFrameList containing all of that frame's children -- in this
    case, just textB -- and then we'd AppendFrameList onto textA->parent)
>       [ { textA textB } ]


LEGEND FOR DIAGRAMS ABOVE:
 [...] = flexbox
 {...} = anonymous wrapper-block
 BLOCK = block-level element
Stealing frames can be a bit complicated due to floats, right?

If we do make this work, it should help with blocks-of-inlines too.
(In reply to Boris Zbarsky (:bz) from comment #3)
> Stealing frames can be a bit complicated due to floats, right?

I'm very willing to believe it. :)

But -- at least for flexboxes, I don't think floats could cause any issues here, beacuse:
> ‘float’ and ‘clear’ compute to their initial values on a flexbox item
 Source: http://dev.w3.org/csswg/css3-flexbox/#display-flexbox
 (note: "flexbox item" = direct child of a flexbox, basically)

Of course, nsBlockFrame::StealFramesAfter shouldn't assume it's being called from a flexbox, so we can't *depend* on there being no floats.  But we could perhaps start with something like this, if float-handling is more trouble than it's worth right now:
> nsBlockFrame::StealFramesAfter(args)
> {
>   if (GetStyleContext()->GetPseudo() != nsCSSAnonBoxes::anonymousFlexboxItem) {
>     // I'm not an anonymous flexbox item -- I might have floats.
>     // Just call superclass method (which will fail an assertion) 
>     // (This is what will already happen in existing code anyway.)
>     return nsContainerFrame::StealFramesAfter(args);
>   }
>
>   // Proceed, safely assuming that there are no floats.
>   [etc]
> }
(if it's not clear -- per comment 2, I personally only intend to call nsBlockFrame::StealFramesAfter on anonymous wrapper-blocks (a.k.a. anonymous flexbox items))
No, I meant floats that are kids of the block whose kids you're stealing.
(In reply to Boris Zbarsky (:bz) from comment #6)
> No, I meant floats that are kids of the block whose kids you're stealing.

Yup -- in my case, the block whose kids I'm stealing will always be an anonymous flexbox item, which means its kids are all (content-tree-wise) direct children of a flexbox.  So, none of the stolen kids (or their siblings) will be floated.
After the split, should the textA/textB be next/prev-in-flow
with each other, and the two wrapper-blocks also?

If so, perhaps you could do something like this:
1. create a next-in-flow (NIF) for the wrapper-block (FIF)
  [ { textA BLOCK textB } {} ]
2. call FIF->PushLines(line-of-textA)
  [ { textA (OverflowLines: BLOCK textB) } {} ]
3. NIF->DrainOverflowLines
  [ { textA } { BLOCK textB } ]
4. NIF->StealFrame(BLOCK) (cheap since it's on the first line)
  [ { textA } { textB } ]
5. ReparentFrame(BLOCK, NIF, flexbox)
6. flexbox->InsertFrames(BLOCK)
  [ { textA } BLOCK { textB } ]
(In reply to Mats Palmgren [:mats] from comment #8)
> After the split, should the textA/textB be next/prev-in-flow
> with each other, and the two wrapper-blocks also?

Nope, sadly.  This is basically when a <div/> is inserted between </u> and <i> here:
>  <div id="flexbox"><u>textA</u><i>textB</i></div>
producing:
>  <div id="flexbox"><u>textA</u><div>BLOCK</div><i>textB</i></div>

textA and textB aren't next/prev-in-flow of each other after the BLOCK insertion.
However, if I can easily sever the next/prev-in-flow relationship, then maybe comment something like comment 8 would still be the way to go...?  (with a "OK everyone update your NIF relationships now" step at the end)

Ultimately, at the end of comment 8, I want:
 - textA & textB themselves have no next/prev-in-flow
 - textB wrapperBlock <--> BLOCK <--> textB_wrapper_block (normal frame sibling relationship)
Actually, maybe the steps I outlined doesn't link up frames as next/prev-in-flows
except for the top frames, so just unlinking those might do what you want.
> So, none of the stolen kids (or their siblings) will be floated.

I'm thinking this situation:

  <div style="display: flexbox">
    Text
    <div></div>
    <span><span style="float: right">A float</span>More text</span>
  </div>

and then removing the div.  What happens with the inner span?
Sorry -- I was totally wrong in comment 4 & comment 7 about floats not being an issue.

The float/clear properties are only nerfed on *flexbox items*, which doesn't include spans (and other inline non-replaced content).

So -- to answer your question -- the inner span should be floated to the right of its anonymous wrapper. It also would float even if it weren't "inner", like here:
  <div style="display: flexbox">
    Text
    <div></div>
    <span style="float: right">A float</span>More text
  </div>

If the span had float:left, then after the div is removed, I'd expect the span to jump to the left of "Text" (after the anonymous wrappers are merged).
OK -- I sent some clarification messages to www-style on float-in-flexboxes, and the results are:
 (a) float is now just ignored on flexbox items (instead of computing to its initial value)
 (b) If we've got a floated span as the direct child of a flexbox, it'll become "display:block" (since float does that to inline content) and be treated as a flexbox item, but its float property otherwise has no effect.
 (c) If we've got a floated span *inside another span* in a flexbox (as in Comment 12), then the outer span will be wrapped, and the float will be honored & float to the left edge of the wrapper.

So basically -- comment 12 is still correct, except for the <<It also would float even if it weren't "inner">> part, per point (b).

www-style references:
 http://lists.w3.org/Archives/Public/www-style/2012Jan/1091.html (for points (a) & (b)
 http://lists.w3.org/Archives/Public/www-style/2012Jan/1150.html (for point (c))
OK.  So you will need to either reframe the flexbox (or at least some subset of its kids) or have a way of reparenting floats.
(to be clear, "reframe" = "rebuild the frames for"?)
I think nsBlockFrame::PushLines does reparent floats, so I still think the steps
in comment 8 should work if you unlink next-in-flow to the new wrapper.
I'm not sure PushLines will be sufficient in general...

It make sense in comment 8 with a inserted, since the block triggers a line-break and that's the split point.  But we have to also have to split at the middle of a line, around inline atomic content like <img> or <embed> (which also create flexbox items).
> It make sense in comment 8 with a inserted
er, "with a _block_ inserted"
In that case I think you could start with a SplitLine and then use the same procedure.
Resolving this as INVALID, since it's no longer necessary for flexbox -- rather than dynamically shifting children within anonymous flexbox items' frames, I'm now detecting situations that would trigger that at the frame-constructor level and simply rebuilding the flexbox's frames when that happens. Attachment 611598 [details] [diff] has the code for this.  (This is similar to what we do for pseudoframes within tables.)

(I'm assuming INVALID is the right resolution here... It's not WONTFIX because maybe someday we'll need to do this -- rather, it's that my prior reason for fixing this is no longer valid.)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.