Closed Bug 1449326 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: smakinson, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
Could you please add a reduced test case that shows the problem?
Flags: needinfo?(smakinson)
smakinson@gmail.com, please add a reduced test case that shows the problem and reopen this bug.
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
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);
}
Flags: needinfo?(smakinson)
Oh and the most of the class names come from http://tachyons.io
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
Status: RESOLVED → UNCONFIRMED
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Resolution: INCOMPLETE → ---
Version: 60 Branch → Trunk
Component: CSS Parsing and Computation → Layout
From the regression range, it looks like suspiciously relevant to bug 1209697.
Blocks: 1209697
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.
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...
Assignee: nobody → emilio
Flags: needinfo?(emilio)
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!
Attachment #8968854 - Attachment is obsolete: true
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 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!
Flags: needinfo?(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/#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.
(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.)
(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.)
Flags: needinfo?(emilio)
Flags: needinfo?(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/#review243842

LGTM! Remember to mark issues as addressed so that MozReview will let this autoland. :)
Attachment #8968945 - Flags: review?(dholbert) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f82fe06f98b
Account for min- / max- block size changes too in the flex caching code. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/3f82fe06f98b
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is this patch worth backporting to 60 in the last week or the cycle or can it ride the 61 train?
Flags: needinfo?(emilio)
Flags: in-testsuite+
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
Flags: needinfo?(emilio)
Attachment #8968945 - Flags: approval-mozilla-beta?
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.
Attachment #8968945 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.