Closed Bug 1145968 Opened 5 years ago Closed 5 years ago

[css-grid] Make grid items paint as inline-blocks and create a stacking context when 'z-index' != 'auto'

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(3 files, 2 obsolete files)

Attached file Testcase #1 (obsolete) —
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.
Attached patch fix+test (obsolete) — Splinter Review
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
Assignee: nobody → mats
Attachment #8581127 - Flags: review?(dholbert)
Blocks: css-grid, 1107786
Keywords: testcase
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.
Attached patch fix+testSplinter Review
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
Attachment #8581127 - Attachment is obsolete: true
Attachment #8581127 - Flags: review?(dholbert)
Attachment #8581190 - Flags: review?(dholbert)
Attached file Testcase #1
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 ;-)
Attachment #8581125 - Attachment is obsolete: true
Summary: [css-grid] Make grid items paint as blocks → [css-grid] Make grid items paint as inline-blocks and create a stacking context when 'z-index' != 'auto'
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.
Attachment #8581190 - Flags: review?(dholbert) → review+
> 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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/13f447b5f8c2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.