Closed Bug 1386654 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Version:
54 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: peter.hozak, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1386654: Handle the special case of a flex frame being the absolute containing block correctly from the CSS align code.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
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.
Component: Untriaged → Layout
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(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.)
See Also: → 1428263
Taking, since I got a patch for this.
Assignee: nobody → 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

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.
Attachment #8941234 - Flags: review?(dholbert) → review+
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 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!
Attachment #8941234 - Flags: review+ → review?(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?
Attachment #8941234 - Flags: review?(dholbert) → review+
Pushed by ecoal95@gmail.com:
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
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b7db2853306
followup: Update WPT expectations to account for new passes. r=me
Pushed by ecoal95@gmail.com:
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
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/6538de3b6137
https://hg.mozilla.org/mozilla-central/rev/0b7db2853306
https://hg.mozilla.org/mozilla-central/rev/d1467b520d20
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: