1320484 - `display:-webkit-box` abspos children were broken when we updated to latest css flexbox spec language

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

[Filing this for webcompat issue https://github.com/webcompat/web-bugs/issues/3794 ]

STR:
 1. Visit https://m.walmart.com.br/lampada-ultraled-5w-golden-a60-6500k-branca/4091385/pr

 2. Look at the gray-backgrounded section headings partway down the page ("Descrição", "Características", ...)

EXPECTED RESULTS:
There should be an icon to left of the text.

ACTUAL RESULTS:
The icon and the text are superimposed.


The setup is:
 <div style="display: -webkit-box">
   <icon>
   <abspos label>
 </div>


Before bug 1269045, we'd drop a placeholder for the abspos label, to the right of the icon.  After bug 1269045, we put the placeholder at the upper-left of the container (and align it using code in bug 1269046).

Apparently -webkit-box containers need the older behavior. We should probably restore the old code that was removed in bug 1269045, but only make it happen for -webkit-box containers.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (should render as "aaabbb")

Comment 2

[Daniel Holbert [:dholbert]]

(In current Nightly, testcase 1 renders with "aaa" and "bbb" superimposed)

Comment 3

[Julien Cristau [:jcristau]]

tracking for 52 as new webcompat regression

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Do we think we'll get a fix for 52?

Comment 5

[Daniel Holbert [:dholbert]]

Yes. (Sorry for the delay here - the impact seems pretty small so far, and the fix should be pretty straightforward/uncontroversial, so I didn't prioritize getting this out as much as I might've otherwise.)

I plan to have a patch here in the next couple days, which we can then uplift to Aurora52.  (And it looks like we've got another month of Aurora52, too, per https://wiki.mozilla.org/RapidRelease/Calendar -- it says the next branch day is 1/24.)

Comment 6

[Astley Chen (inactive)]

Daniel, merge day is around the corner, do you still consider to create a fix for aurora 52 ?

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1320484 part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*.

This patch doesn't affect behavior at all -- it just refactors the
sanity-checking function "FrameWantsToBeInAnonymousItem()", so that the next
patch in this series can give it a new special case that checks a state bit on
the container frame.

This patch renames "parent" to "container" in this function's variable-names,
for clarity, because when this function returns true, the flex/grid container
is actually NOT expected to be the parent of aFrame.  Rather, it's expected to
be the grandparent, and the anonymous flex/grid item would be the parent.  So,
"aContainerFrame"/"containerType" is a bit more accurate (representing the
flex/grid container for aFrame).

Also worth mentioning: this patch makes FrameWantsToBeInAnonymousItem() perform
its own local GetType() call, instead of accepting an already-queried GetType()
result from the caller (as it previously did).  Technically this could cause a
slight perf hit, but it doesn't really matter since this is in "#ifdef DEBUG"
sanity-checking code anyway.  We could keep the nsIAtom* as an additional arg
to avoid this new call, but it seems better to fall on the side of simplicity &
just look up GetType() independently, rather than complicating the function
signature with an extra arg.

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

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1320484 part 2: Wrap abspos placeholders in anonymous flex items, in flex containers that are really emulating legacy -webkit-box containers.

This changeset is intended to revert the logic from "Bug 1269045 part 3" [1],
*specifically* for legacy "-webkit-box"/"-webkit-inline-box" flex containers.

[1]  https://hg.mozilla.org/mozilla-central/rev/707b2ab5879d

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

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1320484 part 3: Add reftest for simple positioning of abspos child inside of a -webkit-box container.

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

Comment 10

[Daniel Holbert [:dholbert]]

Sorry for the delay. Small/targeted patches posted now.

NOTES, FOR REVIEW PURPOSES:
* As noted above, the high-level idea here is to semi-revert bug 1269045's changes, *just* for -webkit-box containers.  We want to wrap placeholders in anonymous boxes again inside of those containers.
* And in fact, we only need to revert "part 3" of that bug ( https://hg.mozilla.org/mozilla-central/rev/707b2ab5879d ). The other parts don't need any special changes/reverting, because:
  * Bug 1269045 part 1 & 2 were about adding code to handle nsPlaceholderFrames as children (and that code will simply be harmless in -webkit-boxes, because they won't have any nsPlaceholderFrames as direct children).
  * Bug 1269045 part 1 also removed some unused nsPlaceholderFrame::GetRealFrameFor() calls in our "order"-sorting code, which only existed in the first place for speculative/code-consistency purposes[1]. Since that removed code was unused, we don't need to add it back.
  * Bug 1269045 part 4 was just removing an unneeded "flex or grid" arg, and we still don't need that arg (so we don't need to revert that changeset) because all we care about is "is this a webkit-box container", and we've got another arg that tells us that specifically.

Bug 1269045 part 3 is the part that we *do* want to semi-revert, since that's what gets placeholder-children wrapped in anonymous flex items (which then nsFlexContainerFrame plays nicely with, just like any other flex item).  So, that's what the main patch here ("part 2", comment 8) does.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=877890#c5

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8823938 [details]
Bug 1320484 part 2: Wrap abspos placeholders in anonymous flex items, in flex containers that are really emulating legacy -webkit-box containers.

https://reviewboard.mozilla.org/r/102408/#review102778

::: layout/base/nsCSSFrameConstructor.cpp:12789
(Diff revision 1)
> -      mStyleContext->StyleDisplay()->IsInlineOutsideStyle()) {
> -    // In a -webkit-box, all inline-level content gets wrapped in an anon item.
> +    if (mStyleContext->StyleDisplay()->IsInlineOutsideStyle()) {
> +      // In a -webkit-box, all inline-level content gets wrapped in anon item.
> -    return true;
> +      return true;
> -  }
> +    }
> +    if (!(mFCData->mBits & FCDATA_DISALLOW_OUT_OF_FLOW) &&
> +        aState.GetGeometricParent(mStyleContext->StyleDisplay(), nullptr)) {

In case it's not obvious: this block of code comes directly from https://hg.mozilla.org/mozilla-central/rev/707b2ab5879d#l1.41.

And the "aState.GetGeometricParent" check is really just a check for "is-this-frame-abspos/fixedpos". (The "!FCDATA_DISALLOW_OUT_OF_FLOW && non-null geometricparent" dance is an idiom that gets used repeatedly to check for abspos/fixedpos frames in nsCSSFrameConstructor.)

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8823937 [details]
Bug 1320484 part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*.

https://reviewboard.mozilla.org/r/102406/#review103214

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8823938 [details]
Bug 1320484 part 2: Wrap abspos placeholders in anonymous flex items, in flex containers that are really emulating legacy -webkit-box containers.

https://reviewboard.mozilla.org/r/102408/#review103220

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8823939 [details]
Bug 1320484 part 3: Add reftest for simple positioning of abspos child inside of a -webkit-box container.

https://reviewboard.mozilla.org/r/102410/#review103222

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfea3f3068f0
part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*. r=mats
https://hg.mozilla.org/integration/autoland/rev/f42dc6b82a4d
part 2: Wrap abspos placeholders in anonymous flex items, in flex containers that are really emulating legacy -webkit-box containers. r=mats
https://hg.mozilla.org/integration/autoland/rev/b7289e864f43
part 3: Add reftest for simple positioning of abspos child inside of a -webkit-box container. r=mats

Comment 16

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/cfea3f3068f0
https://hg.mozilla.org/mozilla-central/rev/f42dc6b82a4d
https://hg.mozilla.org/mozilla-central/rev/b7289e864f43

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8823937 [details]
Bug 1320484 part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*.

Requesting approval to uplift this bug's patches to Aurora.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1269045
[User impact if declined]: Layout breaking on some sites that use -webkit-box
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet, but I'll make sure it is soon.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1321698.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is just restoring the pre-regression behavior (the behavior that we've been shipping up until bug 1269045).  (Kind of like a backout, but just for a restricted situation -- for -webkit-box containers).
[String changes made/needed]: No.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> [Has the fix been verified in Nightly?]: Not yet, but I'll make sure it is soon.

Just verified the fix myself in latest nightly on Android.
* Nightly 53 2017-01-06 gives ACTUAL RESULTS.
* Nightly 53 2017-01-07 gives EXPECTED RESULTS.

So, the fix did indeed work. (2017-01-07 is the first build to include it.)

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8823937 [details]
Bug 1320484 part 1: Improve documentation for debug-only function FrameWantsToBeInAnonymousItem(), & change its arg from nsIAtom* to nsIFrame*.

fix for -webkit-box layout, aurora52+

Comment 20

[Daniel Holbert [:dholbert]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d51decb665c07ee712aee1c4044471b1607ef84e
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/1912201626c16373c3b5ce8206cd24b93f6fdc22
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/1f3bb12e7ea66181f264cb8f5f4ab125f6b56fbf