Bug 103329 - [CSS Grid Layout] Properly support for z-index on grid items
Summary: [CSS Grid Layout] Properly support for z-index on grid items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2012-11-26 17:11 PST by Julien Chaffraix
Modified: 2014-06-26 00:30 PDT (History)
25 users (show)

See Also:


Attachments
Patch (10.62 KB, patch)
2014-05-28 14:04 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (500.88 KB, application/zip)
2014-05-28 15:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (500.27 KB, application/zip)
2014-05-28 16:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (471.81 KB, application/zip)
2014-05-28 22:01 PDT, Build Bot
no flags Details
Patch (11.42 KB, patch)
2014-06-12 15:36 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (14.20 KB, patch)
2014-06-24 15:44 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-11-26 17:11:52 PST
Per specification, section 8 "Drawing Order of Grid Items", the z-index controls the drawing order. Also the grid element and grid items should generate stacking contexts

We should get most of the feature for free with having RenderLayers.
Comment 1 Manuel Rego Casasnovas 2014-04-30 04:19:49 PDT
We would need to port the changes in Blink linked from this bug: https://code.google.com/p/chromium/issues/detail?id=234200
Comment 2 Manuel Rego Casasnovas 2014-05-28 14:04:25 PDT
Created attachment 232211 [details]
Patch
Comment 3 Build Bot 2014-05-28 15:29:26 PDT
Comment on attachment 232211 [details]
Patch

Attachment 232211 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4515958177660928

New failing tests:
css3/blending/repaint/blend-mode-turn-off-isolation.html
Comment 4 Build Bot 2014-05-28 15:29:31 PDT
Created attachment 232217 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-05-28 16:29:12 PDT
Comment on attachment 232211 [details]
Patch

Attachment 232211 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5750338220982272

New failing tests:
css3/blending/repaint/blend-mode-turn-off-isolation.html
Comment 6 Build Bot 2014-05-28 16:29:20 PDT
Created attachment 232219 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2014-05-28 22:01:21 PDT
Comment on attachment 232211 [details]
Patch

Attachment 232211 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6689821514792960

New failing tests:
css3/blending/repaint/blend-mode-turn-off-isolation.html
Comment 8 Build Bot 2014-05-28 22:01:29 PDT
Created attachment 232233 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Manuel Rego Casasnovas 2014-06-12 15:36:51 PDT
Created attachment 233000 [details]
Patch
Comment 10 Manuel Rego Casasnovas 2014-06-13 03:38:46 PDT
rosca, could you please take a look to the rebaseline for css3/blending/repaint/blend-mode-turn-off-isolation.html is it right? Thanks.
Comment 11 Ion Rosca 2014-06-16 06:20:43 PDT
This shouldn't break anything on blending side. However, this is some extra paint effort on static elements forcing zIndex to 0 due to a 'isolation: isolate;' property.
Comment 12 Manuel Rego Casasnovas 2014-06-23 15:17:10 PDT
Ping reviewers. Thanks.
Comment 13 Benjamin Poulain 2014-06-24 13:36:51 PDT
Comment on attachment 233000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233000&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:676
> +    // StyleResolver has ensured that zIndex is non-auto only if it's applicable.

I would consider a series of assertions here to ensure that comment is true.
Otherwise a mistake in the future could cause us to overpaint like crazy.

> Source/WebCore/rendering/style/RenderStyle.cpp:678
> +    if (m_box->zIndex() != other->m_box->zIndex()
> +        || m_box->hasAutoZIndex() != other->m_box->hasAutoZIndex())

This can be on one line, no need to use Google's short line style.

> LayoutTests/ChangeLog:13
> +        * fast/css-grid-layout/grid-item-z-index-change-repaint-expected.html: Added.
> +        * fast/css-grid-layout/grid-item-z-index-change-repaint.html: Added.

I would add more tests similar to this:

Here you test if changing the z-index cause a correct repaint.

There is one area of the patch that is not tested by this: going in and out of grid position. I would add tests ensuring
1) An element supports the z-index once it gets a grid positioning.
2) It repaints accordingly.
Comment 14 Manuel Rego Casasnovas 2014-06-24 15:43:03 PDT
Comment on attachment 233000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233000&action=review

Thanks for the review. I'm uploading a new version of the patch.

>> Source/WebCore/rendering/style/RenderStyle.cpp:676
>> +    // StyleResolver has ensured that zIndex is non-auto only if it's applicable.
> 
> I would consider a series of assertions here to ensure that comment is true.
> Otherwise a mistake in the future could cause us to overpaint like crazy.

I guess you're talking here about some assert checking that if z-index is not auto we're in positioned element or a grid item.
However, I haven't found how to know if we're in a grid item (in RenderStyle) as we would need to check the display value of its parent.
That's the reason why I'm not adding any assert in the new version of the patch.
Comment 15 Manuel Rego Casasnovas 2014-06-24 15:44:11 PDT
Created attachment 233755 [details]
Patch
Comment 16 Benjamin Poulain 2014-06-25 13:51:57 PDT
Comment on attachment 233755 [details]
Patch

Thanks!
Comment 17 WebKit Commit Bot 2014-06-26 00:30:29 PDT
Comment on attachment 233755 [details]
Patch

Clearing flags on attachment: 233755

Committed r170474: <http://trac.webkit.org/changeset/170474>
Comment 18 WebKit Commit Bot 2014-06-26 00:30:39 PDT
All reviewed patches have been landed.  Closing bug.