1306213 - Make flex items explicitly resolve "align-self:auto" against flex container's "align-items" (rather than against parent style-context)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

As noted in bug 1304012 comment 31, we resolve "align-self:auto" against the parent style-context right now, via this code (where |this| is a FlexItem):
    mAlignSelf = aFlexItemReflowInput.mStylePosition->UsedAlignSelf(
                   mFrame->StyleContext()->GetParent());
https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/layout/generic/nsFlexContainerFrame.cpp#1720


*Usually* this means we resolve it against the flex container.  But for tables, the flex item will be the nsTableWrapperFrame, whose parent style-context is the table's style context (rather than the flex container's).

So we end up misrendering the align-self:auto chunks of https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-table.xhtml
(At least, we do after we've applied bug 1304012's fixes. I'm not quite sure why we don't misrender them right now; I think our align-self inheritance hacks must save us somehow, but I need to think through it a bit further.)

Anyway -- on a flex item, clearly the intent is that "align-self:auto" should resolve (at used-value-time) to the flex container's align-items value, regardless of style-context parentage.  Filing this bug on fixing that.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1306213: When resolving a flex item's "align-self: auto", use the flex container (not style-context parent) as the "align-items" source.

(Normally, the style-context parent will *be* the flex container's style
context, so this patch won't change behavior at all. But if a flex container
has a "display:table" child, then there's an extra style context in the
inheritance chain (due to how style inheritance works for nsTableWrapperFrame).
And we don't want that extra style context to mess up the ability of a flex
container's "align-items" property to actually align the flex items.)

Review commit: https://reviewboard.mozilla.org/r/82034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82034/

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8796035 [details]
Bug 1306213: When resolving a flex item's "align-self: auto", use the flex container (not style-context parent) as the "align-items" source.

https://reviewboard.mozilla.org/r/82034/#review80626

Comment 3

[Daniel Holbert [:dholbert]]

Thanks! I triggered a Try run; I'll land after that completes, if there are no surprises.

Comment 4

[Daniel Holbert [:dholbert]]

Try is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b9f6e010b9
(opt testrun is done, debug testrun is mostly-done)

I'll optimistically assume the rest of the test runs will be fine, and land.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d40e2abb42d6
When resolving a flex item's "align-self: auto", use the flex container (not style-context parent) as the "align-items" source. r=mats

Comment 6

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=378ff29f2737

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> So we end up misrendering the align-self:auto chunks of [...] flexbox-align-self-horiz-001-table.xhtml
> (At least, we do after we've applied bug 1304012's fixes. I'm not quite sure
> why we don't misrender them right now; I think our align-self inheritance
> hacks must save us somehow, but I need to think through it a bit further.)

Ah, right -- it makes sense that this works "correctly" in current Nightly (before the patch here).  Basically, it works because we resolve the value earlier on (in nsRuleNode) and we never end up actually using the passed-in nsStyleContext, for nsTableWrapperFrame.

That's because nsTableWrapperFrame has "align-self:inherit", and we have a special case to handle "align-self:inherit inheriting auto" (which clones align-items from the grandparent), in a special case within nsRuleNode.cpp.  (That special case is getting removed in bug 1304012.)

So in current trunk, our nsTableWrapperFrame-flex-item has its StylePosition()->mAlignSelf **already resolved** to some non-"auto" value (via the nsRuleNode.cpp inheritance code which aggressively resolves it when it detects that we're inheriting "auto").

So it doesn't actually matter which parent-style-context we pass to the "UsedAlignSelf()" method (technically called ComputedAlignSelf() in current trunk), because that style context is not needed/used at all.  But after bug 1304012, we just let "auto" be inherited as itself, so we *do* end up needing & using the passed-in nsStyleContext in the UsedAlignSelf() method. And it needs to be the nsStyleContext for the flex container, as noted above.

Comment 8

[Carsten Book [:Tomcat]]

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