positioned/floated/inline flex containers have their children paint in the wrong order. (grandchildren's backgrounds paint on top of later children's backgrounds)

RESOLVED FIXED in mozilla20

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

STR:
 1. Load attached testcase.

EXPECTED RESULTS: All three flex containers look the same -- no red should be visible.

ACTUAL RESULTS: The first flex container is correct; all the rest are wrong & show red.


NOTES: The red div is a child of the first (blue) flex item, with its position adjusted using "margin-left". This red div *should* be completely covered by the second (lime) flex item, aside from a sliver of its orange border.  But currently, in most of the cases covered by this bug's testcase, it's not covered -- it paints *on top* of the second (lime) flex item.

Chrome dev channel & opera-next both show EXPECTED RESULTS. Firefox nightly shows ACTUAL RESULTS.

Marking as dependent on bug 807897 since this bug's patch will probably stack on top of that bug's patch.
Looks like this comes down to whether |aLists| is putting borders & backgrounds in a BlockBorderBackground list vs. a different list.

Blocks do this by making their own DisplayListCollection and then explicitly creating a DisplayListSet which is told to put borders & backgrounds into the BlockBorderBackgrounds list.

https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=7bd96dda75f0&mark=6064-6065#6059

I think we want to do that.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
OS: Linux → All
Hardware: x86_64 → All
Created attachment 679784 [details] [diff] [review]
fix w/ reftest, v1
Attachment #679784 - Flags: review?(dbaron)
(The reftest is essentially a cleaned up & shrunken-to-120px-high version of the attached testcase. I've verified locally that it fails w/o the fix, and passes w/ the fix.)
Comment on attachment 679784 [details] [diff] [review]
fix w/ reftest, v1

I don't see why you need |collection| at all.  Why can't you just construct childLists off of aLists, and then not bother with any MoveTo at all?  (Then again, nsBlockFrame does that too.)

(I also don't see why nsDisplayListCollection has an mBorderBackground list rather than requiring the caller to pick whether its mBorderBackground pointer should point to mBlockBorderBackground or mContent.)

I might be missing something, though.
Created attachment 685431 [details] [diff] [review]
fix w/ reftest, v2

You're right -- collection is unnecessary.

Changed to just construct childLists off of aLists, & confirmed that reftest still passes w/ that change.
Attachment #679784 - Attachment is obsolete: true
Attachment #679784 - Flags: review?(dbaron)
Attachment #685431 - Flags: review?(dbaron)
There is one benefit (if you can call it a benefit) that we get from using a local-variable |collection| -- if one of our BuildDisplayListForChild() calls fails, we'll take the NS_ENSURE_SUCCESS early-return without having messed up |aLists| -- we'll only have messed-up a local variable.

Not sure if that's an intended/important behavior of other BuildDisplayList() implementations, or if it's just a side effect that doesn't really matter.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> There is one benefit (if you can call it a benefit) that we get from using a
> local-variable |collection| -- if one of our BuildDisplayListForChild()
> calls fails, we'll take the NS_ENSURE_SUCCESS early-return without having
> messed up |aLists| -- we'll only have messed-up a local variable.

roc says that aspect doesn't matter:
> <roc> BuildDisplayListForChild etc should really be infallabile
> <roc> so it doesn't really matter
> <roc> so  wouldn't bother with the collection

So, the updated fix (w/o collection) should be fine.
Comment on attachment 685431 [details] [diff] [review]
fix w/ reftest, v2

r=dbaron
Attachment #685431 - Flags: review?(dbaron) → review+
The failure is *only* for the final fixed-pos case, and the difference is very minor.

In both the reference case and the testcase, there's a faint "checkerboard" pattern visible in the lightblue border (on 1st flex item), the slateblue border (on 2nd flex item), and the purple background (on 2nd flex item).  (but not on the blue background of the first flex item, for some reason.

The difference is that in the testcase, that checkerboard pattern begins on the flex container's green border -- whereas in the reference case, that green boarder is just solid green.

===

Sine this test-failure is android-specific, not a regression, and limited in extent (visually imperceptible & only affects fixed-pos content): I'm going reland this with that chunk split out into its own test, which I'll mark as fails-if(Android), and I'll file a followup bug on fixing that.
Actually, I just realized that the reference case doesn't have any of its blocks positioned or anything fancy, so it's possible that comment 11 is just an "all fixed-pos content gets painted differently on android" issue.

Running a try-push with "position:fixed" on that chunk of the reference case to test that theory...
(In reply to Daniel Holbert [:dholbert] from comment #13)
> Running a try-push with "position:fixed" on that chunk of the reference case

That worked -- Android R3 is the relevant test here:
  https://tbpl.mozilla.org/?tree=Try&rev=0dbad69ebd06

Re-pushed with that changed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/34d5413b7ed9
(and I filed bug 816876 on "position:fixed" on the issue that necessitated that change)
(er, I meant to say '...on "position:fixed" rendering differently, the issue that necessitated that change')
https://hg.mozilla.org/mozilla-central/rev/34d5413b7ed9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.