Summary: | [CSS Grid Layout] Properly support for z-index on grid items | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | Layout and Rendering | Assignee: | Manuel Rego Casasnovas <rego> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | allan.jensen, benjamin, buildbot, bunhere, cdumez, commit-queue, darin, donggwan.kim, esprehn+autocc, glenn, gyuyoung.kim, jfernandez, koivisto, kondapallykalyan, macpherson, menard, ojan, rego, rniwa, rosca, sergio, simon.fraser, svillar, tony, xan.lopez |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Bug Depends on: | |||
Bug Blocks: | 60731 | ||
Attachments: |
Description
Julien Chaffraix
2012-11-26 17:11:52 PST
We would need to port the changes in Blink linked from this bug: https://code.google.com/p/chromium/issues/detail?id=234200 Created attachment 232211 [details]
Patch
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 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 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 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 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 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
Created attachment 233000 [details]
Patch
rosca, could you please take a look to the rebaseline for css3/blending/repaint/blend-mode-turn-off-isolation.html is it right? Thanks. 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. Ping reviewers. Thanks. 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 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. Created attachment 233755 [details]
Patch
Comment on attachment 233755 [details]
Patch
Thanks!
Comment on attachment 233755 [details] Patch Clearing flags on attachment: 233755 Committed r170474: <http://trac.webkit.org/changeset/170474> All reviewed patches have been landed. Closing bug. |