nsGridContainerFrame is a subclass of nsContainerFrame which paints its child frames as inlines by default: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=133ec7304f22#283 The spec is a bit fuzzy on the subject: http://dev.w3.org/csswg/css-grid/#grid-items "grid items are grid-level boxes, not block-level boxes" whatever that means... ("grid-level boxes" are not defined anywhere). http://dev.w3.org/csswg/css-display/#the-display-outside Anyway, we create block frames for grid items so we should paint them as blocks I think. That's what Chrome does, fwiw. The difference is that if you make two grid items overlap we currently paint the background of the upper item above the foreground of the lower item. Instead, we should put the backgrounds of the items on the same level and the foregrounds of the items above both backgrounds. See Testcase #1.

Trivial fix. I already had a placement test that exposed this actually but I didn't reflect on it at the time, and the difference compared with Chrome was so small that I didn't notice it. I've added a test that would have exposed the difference more prominently. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed5c9222158

http://dev.w3.org/csswg/css-grid/#grid-items The example there says "grid item: anonymous block box around inline content" which sort of confirms it. (Although examples in CSS specs are not normative AFAIK.)

Please point out the issue on www-style if you haven't done so already.

(In reply to Mats Palmgren (:mats) from comment #0) > Anyway, we create block frames for grid items so we should paint them > as blocks I think. That's what Chrome does, fwiw. I think we should paint them like *inline* blocks. The relevant spec text is in section 4.4, I think: # The painting order of grid items is exactly the same # as inline blocks [CSS21], except that order-modified # document order is used in place of raw document order, # and z-index values other than auto create a stacking # context even if position is static http://dev.w3.org/csswg/css-grid/#z-order This is identical to the corresponding flexbox spec section: http://dev.w3.org/csswg/css-flexbox-1/#painting So I think grid's BuildDisplayList method should look pretty close to flexbox's BuildDisplayList method, right? (which is here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=c17b8f486fd5#1879 ) In particular, flexbox BuildDisplayList does the following things that are missing here: (1) It uses a "GetDisplayFlagsForFlexItem" function, to handle the "z-index values other than auto" special-case. (We probably want to generify that. Maybe we need a "nsFlexboxAndGridLayoutUtils.h" for such things.) (2) It uses aLists.BlockBorderBackgrounds just before the loop, to fix bug 808767.

(In reply to Daniel Holbert [:dholbert] from comment #4) > In particular, flexbox BuildDisplayList does the following things that are missing here: [...] > (2) It uses aLists.BlockBorderBackgrounds just before the loop, to fix bug 808767. (Specifically: based on bug 808767, it sounds like "display:inline-flex" did not match "display:inline" on its child stacking order, until we added that BlockBorderBackgrounds line. I'd expect that you might have the same inconsistency for "display:inline-grid" vs. "display:grid", with the attached patch in its current state.)

(Sorry, of course I meant to say: [[ "display:inline-flex" did not match "display:flex"]])

Ah, thanks for the spec reference, I missed that somehow. That definitely makes the painting order clear. I agree we should share this code somehow. I think I'll just move it up as protected method nsContainerFrame::BuildDisplayListForGridOrFlexItem for now. We can move it later if we decide to have a dedicated file for such things. But there's still something odd about bug 808767...

Applying this patch and then running: ./mach reftest layout/reftests/flexbox/reftest.list ./mach reftest layout/reftests/w3c-css/submitted/flexbox/reftest.list both those test suites pass for me. So it seems we have no test coverage for bug 808767? Am I missing something?

Huh, you're right -- no reftests fail for me, either. > So it seems we have no test > coverage for bug 808767? I don't think that's quite it -- bug 808767's attached testcase *still passes*, with your tweak. So maybe some other aspect of display list construction has changed to make that line no longer necessary.

I'm going to add stuff to BuildDisplayList() to implement 'order' in bug 1107786 so I'll just copy your code with pride. :-) https://treeherder.mozilla.org/#/jobs?repo=try&revision=2416395cc5e8

So it turns out our current rendering for this test is actually correct and that this is a bug in Chrome. I'll take this as a lesson to read specs more carefully and not glean to much on what Chrome does ;-)

Comment on attachment 8581190 [details] [diff] [review] fix+test Review of attachment 8581190 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mats Palmgren (:mats) from comment #11) > So it turns out our current rendering for this test is actually correct > and that this is a bug in Chrome. Could you file a Chrome bug on it, then? (https://code.google.com/p/chromium/issues/ ) Also: I filed bug 1146043 on removing "childLists", ideally after we figure out what changed to make it unnecessary. (For now, it seems worthwhile to keep grid & flexbox consistent, so I'm fine with copying it, even if it seems to be probably-unnecessary.) r=me with nits addressed: ::: layout/generic/nsGridContainerFrame.cpp @@ +179,5 @@ > + return nsIFrame::DISPLAY_CHILD_FORCE_STACKING_CONTEXT; > + } > + return nsIFrame::DISPLAY_CHILD_FORCE_PSEUDO_STACKING_CONTEXT; > +} > + Please include an XXX comment here to remind us that we should really share this function with flexbox. (Once we've got a place for common flex/grid utility methods, at least -- e.g. once we add align-items/justify-items/etc. support for grid) @@ +1143,5 @@ > + const nsDisplayListSet& aLists) > +{ > + DisplayBorderBackgroundOutline(aBuilder, aLists); > + > + // Our children are all grid-level boxes, which behaves the same as typo: s/behaves/behave/ ::: layout/generic/nsGridContainerFrame.h @@ +35,5 @@ > virtual nsIAtom* GetType() const MOZ_OVERRIDE; > > + void BuildDisplayList(nsDisplayListBuilder* aBuilder, > + const nsRect& aDirtyRect, > + const nsDisplayListSet& aLists) MOZ_OVERRIDE; s/MOZ_OVERRIDE/override/ (ehsan landed a tree-wide renaming on inbound this morning) ::: layout/reftests/css-grid/grid-placement-definite-002-ref.html @@ +4,5 @@ > <title></title> > <style type="text/css"> > +html,body { > + color:black; background:white; font-size:16px; font-family:monospace; > +} Several nits: - the rest of the style rules in this testcase/reference case have 1 decl per line. Consider rewrapping this rule to match that style. - I'm not clear why "color:black; background:white" is useful here -- seems unnecessary/redundant, at the root of the document. (Technically, background defaults to none, but on the html/body elements, I'm not clear on whether there's a difference between none & white. If there is & it's important, it might be worth calling out in a comment here, to make this less mysterious.) - The (modified-by-this-patch) testcase & reference look the same to me, in current Nightly (with layout.css.grid.enabled turned on). So I'm not sure they actually test the patch. So, unless I'm missing something, you might need to include another testcase here to actually test the change.

> I'm not clear why "color:black; background:white" is useful here It's just a bit of standard boilerplate I usually include in tests. I think I saw it recommended somewhere a couple of decades ago (I've forgotten where) to make a test render correctly also when viewed casually by someone with a non-default preference for bg/fg colors. (i.e. in Firefox: Preferences->Content->Colors). I normally include "padding:0;margin:0;" as well to neutralize differences between UAs -- that's where the html,body selector came from. So if you see this kind or "reset rule" in any of my tests that's the background. I see that Meyer has a blog post on it, perhaps that's the origin: http://meyerweb.com/eric/tools/css/reset/ (although his is a bit more aggressive) > The (modified-by-this-patch) testcase & reference look the same to me Right, the added test is just to catch the buggy first version of the patch better. I don't know how to test against the original code here, let me know if you have any suggestions. I have several tests using 'z-index' that fails without this patch though, that will be included in bug 1107786, fwiw.

Ah, ok - thanks for clarifying.