1386654 - [css-flex] Misaligned absolute child in a fixed flex parent

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Peter Hozak]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

parent {
  position: fixed;
  display: flex;
  align-items: center;
  justify-content: center;
  width: 200px;
  height: 200px;
  background: yellow;
}

child {
  position: absolute;
  width: 100px;
  height: 100px;
  background: green;
}

https://jsfiddle.net/4hgqp0bn/


Actual results:

Initially, the child element is centered around the top left corner.

If a page is resized (e.g. press F12), the positioning becomes correct (centered in the parent).

(Similar if using "flex-end"... but easy workaround: remove position: absolute;)


Expected results:

Correct position from the start.

Comment 1

[Daniel Holbert [:dholbert]]

I think this is responsible for the text labels being misaligned at Project Fi website, too: https://fi.google.com/about/

(That site has a positioned child of a positioned flex container; and the layout is broken on first load (the text is shifted up and to the left), but fixed if I resize the window.)

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Taking, since I got a patch for this.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

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

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

https://reviewboard.mozilla.org/r/211502/#review217326

Thanks for taking this! I thought this would be more complex to fix. :D

r=me with the tests moved as noted below:

::: testing/web-platform/meta/MANIFEST.json:108516
(Diff revision 2)
> +   "css/css-flexbox/position-absolute-containing-block-001.html": [
> +    [
> +     "/css/css-flexbox/position-absolute-containing-block-001.html",

As discussed on IRC, let's put the new test files in https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/ (and add them to reftest.list there).


That way, these tests won't miss out on the extra assertion-checking & performance that WPT is currently missing per bug 1263568 comment 2.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

https://reviewboard.mozilla.org/r/211502/#review217326

> As discussed on IRC, let's put the new test files in https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/ (and add them to reftest.list there).
> 
> 
> That way, these tests won't miss out on the extra assertion-checking & performance that WPT is currently missing per bug 1263568 comment 2.

Done!

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

Welp, had to change a bit the code because the CB size we get is the padding box, not the content box.

Anyway, mind taking another look at the interdiff, and sanity-check the reftest move?

Thanks a lot!

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

https://reviewboard.mozilla.org/r/211502/#review217342

::: layout/generic/nsAbsoluteContainingBlock.cpp:459
(Diff revision 4)
> +      alignAreaSize = aAbsPosCBSize.ConvertTo(pcWM, aAbsPosCBWM);
> +      // aAbsPosCBSize is the padding-box, so substract that.
> +      alignAreaSize -=
> +        aPlaceholderContainer->GetLogicalUsedPadding(pcWM).Size(pcWM);

s/that/padding/

::: layout/reftests/w3c-css/submitted/flexbox/reftest.list:217
(Diff revision 4)
> +# Flexbox as an absolute containing block.
> +
> +== position-absolute-containing-block-001.html position-absolute-containing-block-001-ref.html

stray blank line between comment & tests, I think?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941234 [details]
Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211502/diff/4-5/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6538de3b6137
Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code. r=dholbert

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b7db2853306
followup: Update WPT expectations to account for new passes. r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1467b520d20
followup: Update more WPT expectations since I had to rebase on top of a WPT sync. r=me

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6538de3b6137
https://hg.mozilla.org/mozilla-central/rev/0b7db2853306
https://hg.mozilla.org/mozilla-central/rev/d1467b520d20