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

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 2 bugs, {regression})

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53 fixed)

Details

()

Attachments

(4 attachments)

Assignee

Description

3 years ago
[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.
Assignee

Comment 2

3 years ago
(In current Nightly, testcase 1 renders with "aaa" and "bbb" superimposed)
Assignee

Updated

3 years ago
Blocks: 1238668
Assignee

Updated

3 years ago
Flags: needinfo?(dholbert)
tracking for 52 as new webcompat regression
Priority: -- → P3
Assignee

Updated

3 years ago
Depends on: 1321698
Do we think we'll get a fix for 52?
Assignee

Comment 5

3 years ago
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.)
Daniel, merge day is around the corner, do you still consider to create a fix for aurora 52 ?
Assignee

Updated

3 years ago
Blocks: 1328792
Assignee

Comment 7

3 years ago
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/
Attachment #8823937 - Flags: review?(mats)
Attachment #8823938 - Flags: review?(mats)
Attachment #8823939 - Flags: review?(mats)
Assignee

Comment 10

3 years ago
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
Flags: needinfo?(dholbert)
Assignee

Comment 11

3 years ago
mozreview-review
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

3 years ago
mozreview-review
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
Attachment #8823937 - Flags: review?(mats) → review+

Comment 13

3 years ago
mozreview-review
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
Attachment #8823938 - Flags: review?(mats) → review+

Comment 14

3 years ago
mozreview-review
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
Attachment #8823939 - Flags: review?(mats) → review+

Comment 15

3 years ago
Pushed by dholbert@mozilla.com:
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
Assignee

Comment 17

3 years ago
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.
Attachment #8823937 - Flags: approval-mozilla-aurora?
Assignee

Comment 18

3 years ago
(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 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+
Attachment #8823937 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.