847211 - "ASSERTION: wrong kind of child frame" with <input type=file> restyled to display:flex

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: wrong kind of child frame: 'aIsBlock == f->IsBlockOutside()', file layout/generic/nsLineBox.cpp, line 42

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is this a regression?

Comment 3

[Mats Palmgren (inactive)]

Attachment: frame dump

Fwiw, the child frame styles have changed to display:block somehow.

Comment 4

[Mats Palmgren (inactive)]

Daniel, does the flex box code restyle the 'display' of its children somewhere?

Fwiw, a static example with the same styles results in a correct frame tree
(with the children having display:inline).

Comment 5

[Daniel Holbert [:dholbert]]

Yes, in ApplyStyleFixups.

Bug 812822 added a way for elements to opt out of this, though. (e.g. for elements that have a fixed frame-type, regardless of their "display" value)

We presumably just need to use that here.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: backtrace of ApplyStyleFixups call

Here's the backtrace of the ApplyStyleFixups call that converts the children's display to "block" for this bug's testcase.

Comment 7

[Daniel Holbert [:dholbert]]

So we get a call to nsFrameManager::ReResolveStyleContext with aFrame == nsFileControlFrame, whose children are the ones that we need to prevent ourselves from blockifying.

So, during that call (when we're iterating across the children), we probably need to figure out that those children are an anonymous subtree, and pass that information to our nsStyleSet::ReparentStyleContext() call, so that *it* can pass eSkipFlexItemStyleFixup in its flags to GetContext.

Or something like that.

Comment 8

[Daniel Holbert [:dholbert]]

Or: maybe the "skip flex item fixup" state is something persistent that we should keep track of on the style context, and assume it still applies even when the style context is reparented...

That makes me a bit uncomfortable... It seems wrong to preserve this information across style-reparenting events, because this state is inherently *dependent* on our parent.  i.e. we want it to apply to the anonymous children of an <input type="file"> element, but if those children were to somehow be reparented outside of that <input>, then we would *not* want to preserve that "skip flex style fixup" state on them.

HOWEVER - I suspect we might be guaranteed that anonymous children will never be reparented outside of their parents... I imagine they could only be re-"ancestored", so to speak -- not truly reparented.  If that's indeed the case, then it should be fine to cache this state on the style context.

Comment 9

[Jesse Ruderman]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Is this a regression?

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/63a6466d8c46
user:        Daniel Holbert
date:        Tue Feb 26 21:44:55 2013 -0800
summary:     Bug 844529: Disable flex-item style fixup when resolving style for anonymous content in nsCSSFrameConstructor::ProcessChildren(). r=bz

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Is this a regression?

I guess you are referring to my <input type='file'> changes. They are unfortunately blocked by a11y issues.

Comment 11

[Daniel Holbert [:dholbert]]

I don't know if that's what he was referring to, but it looks like this ended up being a regression from the cset in comment 9, so I don't think your <input type='file'> changes interact with this -- this is a style-system / flexbox bug.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This implements the strategy that I mentioned in comment 7.

I also added an assertion of the same "IsRootOfAnonymousSubtree" condition, in the spot where we initially resolve style for this sort of element, to be sure that we're consistently applying the "skip flex item style fixup" on the same elements on initial-style-resolution vs. style-re-resolution.

Comment 13

[Daniel Holbert [:dholbert]]

https://tbpl.mozilla.org/?tree=Try&rev=cec3bc437e55

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 737603 [details] [diff] [review]
fix v1

r=me

Comment 15

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d23145d1cd5f

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/d23145d1cd5f