738007 - Non-functional tweaks to nsCSSFrameConstructor::FlushAccumulatedBlock

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: patch v1

I'll be customizing nsCSSFrameConstructor::FlushAccumulatedBlock() to work for flexbox as well as MathML.

I made a few non-flexbox-related (non-functional) tweaks to that function & its caller, and I figured it'd be good to make that its own patch, so that the flexbox patches can stay flexbox-specific.

Patch attached. The changes are:
 (a) s/currentBlock/currentBlockItems/ to make it clear it's a nsFrameItems instance
 (b) Make FlushAccumulatedBlock take nsFrameItems& instead of nsFrameItems*, to match the convention. (No one else takes a nsFrameItems*, and FlushAccumulatedBlock was actually having to dereference the pointer to pass it to other functions)
 (c) Use a local variable for the anon pseudoclass. (I'll change this variable's value to something flexbox-specific in the flexbox patch)
 (d) indentation cleanup

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 608066 [details] [diff] [review]
patch v1

r=me

I just realized that this function wraps up the block of an {ib} split with the inlines.  I'm pretty sure that's not the behavior you want for flexbox; I'm not sure why mathml is doing it (and I would argue that mathml is wrong to do it, but check the blame on that IsFrameSpecial check?).

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #1)
> Comment on attachment 608066 [details] [diff] [review]
> patch v1
> 
> r=me
> 
> I just realized that this function wraps up the block of an {ib} split with
> the inlines.  I'm pretty sure that's not the behavior you want for flexbox;

I think it is what the spec calls for, actually.  See this example in the spec:
  <div style="display:flexbox">
[...SNIP...] 
    <!-- flexbox item: anonymous block around inline content -->
    anonymous item 7.1
    <span id="item7.1">
        text 7.2
        <div id="not-an-item7.3">block</div>
        text 7.4
    </span>

with this description:

   Notice that block element "not-an-item7.3" is not a separate flexbox item,
   because it is contained inside an inline element which is being wrapped
   into an anonymous flexbox item.

(^ I fixed a typo in the prose manually, per
 http://lists.w3.org/Archives/Public/www-style/2012Mar/0523.html
so that it makes sense)

So: if we have <span>, we want to wrap it & its entire contents in a single anonymous block.  Its block-level children do not create their own flexbox items.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> See this example in the spec

The example contradicts the normative spec text.  Please raise a spec issue?

If that's the behavior we want for {ib} splits, then we should be able to do the block-wrapping on the frame construction item list instead, which would solve the float problems completely, assuming we want the spec-described float behavior.

Comment 4

[Daniel Holbert [:dholbert]]

(Landed this bug's patch in any case, since it's not flexbox-specific: 
 https://hg.mozilla.org/integration/mozilla-inbound/rev/149297a4bbc2
)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> > See this example in the spec
> 
> The example contradicts the normative spec text.  Please raise a spec issue?

Done: http://lists.w3.org/Archives/Public/www-style/2012Mar/0528.html

Comment 6

[Daniel Holbert [:dholbert]]

I also filed a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16483 (overkill perhaps)

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/149297a4bbc2