1449326 - align-items: center not kept on child when its changes size

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[smakinson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

a span centered in an element with align-items:center is on hover set to have a min-width and min-height set to 100%. In chrome, safari and IE11 the centered item transitions from the center as it fills the parent.



Actual results:

In firefox the top of the centered item remains where it is and the size grows to outside its parent. 


Expected results:

The centered item should remain centered and grow to fill the parent element.

Comment 1

[Adrian Florinescu [:aflorinescu]]

Could you please add a reduced test case that shows the problem?

Comment 2

[Adrian Florinescu [:aflorinescu]]

[e-mail], please add a reduced test case that shows the problem and reopen this bug.

Comment 3

[smakinson]

Sorry about the delay, here is test case. In trying to recreate it outside a project I realized it is flex-wrap: wrap on a container div making the difference.

https://jsfiddle.net/8xkx8eom/30/


<div class="flex flex-wrap w-100">
  <!-- flex-wrap on this div causes the issue -->
  <div class="flex flex-wrap w-40 h4 mh2 overflow-hidden">
    <a
       href="#"
       class="item flex flex-wrap items-center justify-center link w-100 white"
       >
      <span class="flex flex-wrap items-center justify-center pa2">wrong</span>
    </a>
  </div>
  
  <div class="flex w-40 h4 mh2 overflow-hidden">
    <a
       href="#"
       class="item flex items-center justify-center link w-100 white"
       >
      <span class="flex flex-wrap items-center justify-center pa2">correct</span>
    </a>
  </div>
</div>

.item {
  background: blue;
}

.item span {
  transition: background-color 0.15s ease-in-out, min-height 0.1s ease-out, min-width 0.1s ease-out;
  min-width: 1px;
  min-height: 1px;
  background-color: rgba(248, 127, 23, 0.75);
  box-sizing: border-box;
}

.item:hover span {
  min-width: 100%;
  min-height: 100%;
  background-color: rgba(248, 127, 23, 1);
}

Comment 4

[smakinson]

Oh and the most of the class names come from http://tachyons.io

Comment 5

[Adrian Florinescu [:aflorinescu]]

Thank you for the test case.
Reproduced the issue on Windows 8.1, Osx 10.13:
61.0a1 2018-04-17
60.0b13 2018-04-16
59.0.2 2018-03-23

Not reproducible on:
52.7.3 ESR 2018-03-22

Comment 6

[Adrian Florinescu [:aflorinescu]]

Additionally, I managed to reduce the regression range to:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f985243bb630b2c78cd57731c8d8ab191aa09527&tochange=bf15d4078c2a6db7df37ab466d28a1e075c9eb4d

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: More reduced test-case (without pulling external library and such).

Comment 8

[Emilio Cobos Álvarez (:emilio)]

From the regression range, it looks like suspiciously relevant to bug 1209697.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Something interesting is that, in the reduced test-case, changing min-height for height does fix it, I suspect because of the has-relative-bsize stuff? Hmm... That looks somewhat fishy.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

So, I have a patch, but I don't understand why this happens so far. We're doing two measuring reflows with different min block sizes during the same reflow pass, which is why that breaks. I'm building r/n with --disable-opt to be able to see what goes wrong there...

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Even more reduced test-case, which shows this is not a dynamic change handling issue.

Lol, in this test-case:

 * Chrome gets what I'd think are "expected results".
 * We before my patch did the same, with my patch do something different.
 * Edge does something even more different.

Also, thanks for the original test-case smakinson, was super helpful to isolate this!

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

We may be reflowing the same kid with different (definite / non-definite)
containing block sizes, which changes the min block size in:

  https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/layout/generic/ReflowInput.cpp#3077

In which case we may need to reflow in order to properly align the flex item.

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

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

https://reviewboard.mozilla.org/r/237644/#review243412

Thanks for jumping on this! I'll take a closer look after I get into the office -- but here are a couple test nits that I noticed in my initial skim:

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-flex-wrap-nested-001.html:11
(Diff revision 1)
> +<link rel="help" href="https://drafts.csswg.org/css-flexbox/#flex-wrap-property">
> +<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1449326">
> +<style>
> +div {
> +  display: flex;
> +  flex-wrap: center;

"flex-wrap: center" is not a valid declaration....

I think you mean "flex-wrap:wrap"? (here, as well as in the <title>)

(Having said that: this test fails in Nightly & passes in Chrome in its current state, regardless of this broken CSS.  So maybe "flex-wrap" isn't actually a relevant part of this test, & it should be renamed... Maybe flexbox-definite-sizes-001.html , since I think you're exercising the distinction between when a size is definite vs. indefinite?

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-flex-wrap-nested-001.html:25
(Diff revision 1)
> +.item,
> +.item span {
> +  justify-items: center;
> +  align-items: center;
> +}

Do you really need the ".item span" rule here?  The span is not a flex container, so I don't think this align-items/justify-items styling on the span itself has any effect.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

https://reviewboard.mozilla.org/r/237644/#review243412

> "flex-wrap: center" is not a valid declaration....
> 
> I think you mean "flex-wrap:wrap"? (here, as well as in the <title>)
> 
> (Having said that: this test fails in Nightly & passes in Chrome in its current state, regardless of this broken CSS.  So maybe "flex-wrap" isn't actually a relevant part of this test, & it should be renamed... Maybe flexbox-definite-sizes-001.html , since I think you're exercising the distinction between when a size is definite vs. indefinite?

Whoopsies :-)

> Do you really need the ".item span" rule here?  The span is not a flex container, so I don't think this align-items/justify-items styling on the span itself has any effect.

It used to be when I was reducing this, but I guess now it's no longer the case, so will simplify it, thanks for the early feedback!

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237644/diff/1-2/

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

https://reviewboard.mozilla.org/r/237644/#review243532

::: layout/generic/nsFlexContainerFrame.cpp:1676
(Diff revision 2)
> +    const nscoord mComputedMinBSize;
> +    const nscoord mComputedMaxBSize;
> +
> +    explicit Key(const ReflowInput& aRI)
> +      : mAvailableSize(aRI.AvailableSize())
> +      , mComputedBSize(aRI.ComputedBSize())
> +      , mComputedMinBSize(aRI.ComputedMinBSize())
> +      , mComputedMaxBSize(aRI.ComputedMaxBSize())

[I haven't yet convinced myself that this is precisely the right thing to cache -- poking around at a simplified version of the testcase in GDB to see if anything jumps out at me]

Also, it'd be nice to include a testcase that exercises the "max-{bsize}" dependency here, if we think it's necessary for correctness to cache that as part of the key...

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-definite-sizes-001.html:3
(Diff revision 2)
> +<!doctype html>
> +<meta charset="utf-8">
> +<title>CSS Test: Nested flex containers with overflow: hidden</title>

Better title might be "CSS Test: nested flex containers with height established by 'min-height'

(since 'overflow:hidden' isn't really part of the core thing being tested, AFAICT.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-definite-sizes-001.html:4
(Diff revision 2)
> +<!doctype html>
> +<meta charset="utf-8">
> +<title>CSS Test: Nested flex containers with overflow: hidden</title>
> +<link rel="match" href="flexbox-align-items-center-nested-001-ref.html">

This "match" tag is pointing at the wrong filename (probably from a a few test-filenames ago :)

Should say "flexbox-definite-sizes-001-ref.html"

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-definite-sizes-001.html:13
(Diff revision 2)
> +.item {
> +  width: 100px;
> +  background: red;
> +  justify-items: center;
> +  align-items: center;

Remove "justify-items:center" here. (Doesn't seem to be necessary to trigger the bug.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-definite-sizes-001.html:23
(Diff revision 2)
> +}
> +
> +.item span {
> +  min-height: 100%;
> +  width: 100%;
> +  background-color: green;

s/background-color/background/

for brevity (& for consistency with the other rule above this one which sets "background:red")

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-definite-sizes-001.html:27
(Diff revision 2)
> +  width: 100%;
> +  background-color: green;
> +}
> +</style>
> +<p>Test passes if you see a green 100px x 100px square, and no red</p>
> +<div>

Remove the outermost <div> here -- it doesn't seem to be necessary to trigger the bug.

(It adds yet another layer of flexbox wrapper due to the div { display:flex } rule, making this more complicated than it needs to be.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-definite-sizes-001.html:28
(Diff revision 2)
> +  background-color: green;
> +}
> +</style>
> +<p>Test passes if you see a green 100px x 100px square, and no red</p>
> +<div>
> +  <div style="min-height: 100px; overflow: hidden;">

Remove "overflow: hidden" (here and in the title). It doesn't seem to be necessary to trigger this bug, and it adds an extra layer of complication.

(It does make the bug a little more obvious by virtue of clipping the incorrectly-overflowing green area, but the bug still reproduces [and red gets painted] without it.)

Also: might be worth including a second variant of this test, where this "min-height:100px" is on .item{} **instead of** on the outermost flex container... That still triggers the issue for me, and seems like a substantially different scenario to be worth testing independently.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase using max-height

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #16)
> Also, it'd be nice to include a testcase that exercises the "max-{bsize}"
> dependency here, if we think it's necessary for correctness to cache that as
> part of the key...

Here's a sample testcase that does indeed trigger a version of this bug, using max-height instead of min-height.

I verified that this regressed between 2017-02-02 and 2017-02-03, which confirms that this testcase is part of the same regression (from bug 1209697).

Please do include a testcase like this one.  (Basically equivalent to your original, but with min-height swapped for max-height, and with a giant grandchild added, *and* testing with the fixed max-height in two different places -- on the outer flex container, and then on the inner flex container. Both places trigger the brokenness, similar to how both spots are broken for min-height as noted tat the very end of my previous comment.)

Comment 18

[Daniel Holbert [:dholbert]]

(Given that the bug does reproduce with max-height as well as min-height, I think the code part of the patch is probably reasonable -- holding off on r+, though, pending the updates/additions to tests, which should probably get a once-over review before this lands.)

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237644/diff/2-3/

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

https://reviewboard.mozilla.org/r/237644/#review243842

LGTM! Remember to mark issues as addressed so that MozReview will let this autoland. :)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f82fe06f98b
Account for min- / max- block size changes too in the flex caching code. r=dholbert

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3f82fe06f98b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this patch worth backporting to 60 in the last week or the cycle or can it ride the 61 train?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Is this patch worth backporting to 60 in the last week or the cycle or can
> it ride the 61 train?

Probably worth it since it's not really risky.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1209697
[User impact if declined]: Wrong rendering with flexbox.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes, just did
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it only makes us cache measurements less often.
[String changes made/needed]: none

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8968945 [details]
Bug 1449326: Account for min- / max- block size changes too in the flex caching code.

Fix for minor regression, looks low-risk, let's uplift for beta 15.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/bb01c965834f