799647 - Flexbox review-comment-fix got lost, for style-context parent lookup to handle "align-self: auto"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Quoting bug 666041 comment 95)
> > >+  // Resolve "align-self: auto" to parent's "align-items" value.
> [...]
> > This isn't quite right -- you want the element parent, so you want
> > data from what you'd inherit style from.  So instead, you want the
> > assignment to assign
> > mFrame->GetStyleContext()->GetParent()->GetStylePosition()->mAlignItems;
> 
> Fixed, including the ua.css tweak we discussed for tables, to deal with
> their weird style inheritance:
> https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/a5b9e56a9ce5

I accidentally reverted the C++ part of this change (the switch to "mFrame->GetStyleContext()->[etc]" that dbaron suggested above) -- I must've been trying the code with that change & without it, and accidentally qref'd with it reverted, and committed the revert in the child of the cset above:
 https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c7921869526b#l1.29
 ^^^ (cset that incorrectly reverted the change)

Sorry about that! Fix coming shortly -- just want to test to be sure it's good.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v1: fix

Here's the code fix.  This will need some test-tweaks, too -- I'll post those shortly.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v1: reftest for the ua.css declaration about table-outer frames inheriting "align-self" property

The test in this patch isn't exactly a regression-test for this bug here, but it's related enough that I think it makes sense to do it as part of this bug.

DESCRIPTION: The test creates a table-specific copy of our general "test all the align-self values" reftest.  This new copy basically checks that our ua.css rule is being correctly applied.  If I disable the "align-self" line in ua.css, this test fails. (It acts as if all of the flex items have "align-self:auto")

NOTE: ua.css is only parsed once, at startup time.  If the flexbox pref isn't set at that point, then we won't honor the flexbox-specific code in ua.css for the duration of that browsing session.  (and we post to the warning console, per bug 798592) This means that tables will only work correctly as flex items if the flexbox pref is set *before you start firefox*.  So this new reftest is marked as failing for the moment, until we enable the pref in bug 783409.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 669719 [details] [diff] [review]
patch 1 v1: fix

r=dbaron

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 669815 [details] [diff] [review]
patch 2 v1: reftest for the ua.css declaration about table-outer frames inheriting "align-self" property

r=dbaron

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe we could do something to always enable the parsing of the flexbox properties in the UA sheet so that we don't have the weird problems here?

Comment 6

[Daniel Holbert [:dholbert]]

I needed to also tweak one existing test (flexbox-align-self-baseline-horiz-1.xhtml) that this patch breaks basically for the reasons explained at the end of comment 2.  To address this, I broke out the table-specific part of that test into a separate reftest, which I marked as failing-pending-bug-783409.

Part 1 (with that^ tweak):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f750a7533a
Part 2:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4301b89fcf

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2f750a7533a
https://hg.mozilla.org/mozilla-central/rev/5c4301b89fcf