The default bug view has changed. See this FAQ.

Non-functional tweaks to nsCSSFrameConstructor::FlushAccumulatedBlock

RESOLVED FIXED in mozilla14

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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
Attachment #608066 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
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?).
Attachment #608066 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 2

5 years ago
(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.
> 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.
(Assignee)

Comment 4

5 years ago
(Landed this bug's patch in any case, since it's not flexbox-specific: 
 https://hg.mozilla.org/integration/mozilla-inbound/rev/149297a4bbc2
)
Target Milestone: --- → mozilla14
(Assignee)

Comment 5

5 years ago
(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
(Assignee)

Comment 6

5 years ago
I also filed a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16483 (overkill perhaps)
https://hg.mozilla.org/mozilla-central/rev/149297a4bbc2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.