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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix w/ reftest, v1

Comment 4

[Daniel Holbert [:dholbert]]

(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 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Daniel Holbert [:dholbert]]

(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 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 685431 [details] [diff] [review]
fix w/ reftest, v2

r=dbaron

Comment 10

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9430676f5442

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android R3 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e6bda0ba0e

https://tbpl.mozilla.org/php/getParsedLog.php?id=17465640&tree=Mozilla-Inbound

REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.213:30111/tests/layout/reftests/flexbox/flexbox-paint-ordering-1.xhtml | image comparison (==), max difference: 9, number of differing pixels: 415

Comment 12

[Daniel Holbert [:dholbert]]

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.

Comment 13

[Daniel Holbert [:dholbert]]

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...

Comment 14

[Daniel Holbert [:dholbert]]

(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

Comment 15

[Daniel Holbert [:dholbert]]

(and I filed bug 816876 on "position:fixed" on the issue that necessitated that change)

Comment 16

[Daniel Holbert [:dholbert]]

(er, I meant to say '...on "position:fixed" rendering differently, the issue that necessitated that change')

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34d5413b7ed9