[webkit-reviews] review denied: [Bug 103329] [CSS Grid Layout] Properly support for z-index on grid items : [Attachment 233000] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 24 13:36:30 PDT 2014
Benjamin Poulain <benjamin at webkit.org> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 103329: [CSS Grid Layout] Properly support for z-index on grid items
https://bugs.webkit.org/show_bug.cgi?id=103329
Attachment 233000: Patch
https://bugs.webkit.org/attachment.cgi?id=233000&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.
More information about the webkit-reviews
mailing list