Last Comment Bug 738007 - Non-functional tweaks to nsCSSFrameConstructor::FlushAccumulatedBlock
: Non-functional tweaks to nsCSSFrameConstructor::FlushAccumulatedBlock
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
Depends on:
Blocks: css3-flexbox
  Show dependency treegraph
 
Reported: 2012-03-21 12:58 PDT by Daniel Holbert [:dholbert]
Modified: 2012-03-22 18:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (5.89 KB, patch)
2012-03-21 12:58 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-03-21 12:58:24 PDT
Created attachment 608066 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-03-21 21:21:53 PDT
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] 2012-03-22 10:21:29 PDT
(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 [:bz] (still a bit busy) 2012-03-22 10:24:59 PDT
> 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] 2012-03-22 10:36:01 PDT
(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] 2012-03-22 11:06:08 PDT
(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] 2012-03-22 11:17:52 PDT
I also filed a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16483 (overkill perhaps)
Comment 7 Marco Bonardo [::mak] 2012-03-22 18:15:00 PDT
https://hg.mozilla.org/mozilla-central/rev/149297a4bbc2

Note You need to log in before you can comment on or make changes to this bug.