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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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 hidden (mozreview-request)

Comment 2

3 years ago
mozreview-review
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
Attachment #8796035 - Flags: review?(mats) → review+
Assignee

Comment 3

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

Comment 4

3 years ago
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

3 years ago
Pushed by dholbert@mozilla.com:
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
Assignee

Comment 7

3 years ago
(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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d40e2abb42d6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.