Fixed background is scrolling in http://www.nieuwecode.nl/ in Qt webkit2

[Yael]

The page at http://www.nieuwecode.nl/ has a fixed position div with a background image, but it does not have z-index.
Currently, we do not create a graphics layer for fixed elements that don't have z-index, and that causes them to scroll.
Changing this in RenderLayerCompositor will fix this issue.

[Yael]

Created attachment 138179 [details]
Patch.

When a fixed position element does not have z-index explicitly specified, it does not create a stacking context. This results in fixed elements scrolling with the content layer.
This patch allows creating a stackig context for fixed positioned elements. Added a manual test because this patch takes effect only during scrolling.

[Yael]

Created attachment 138180 [details]
Patch.

Fixed a typo in the changelog. sorry :)

[Simon Fraser (smfr)]

Comment on attachment 138180 [details]
Patch.

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

> Source/WebCore/css/CSSStyleSelector.cpp:2009
> +                                   || style->hasTransformRelatedProperty() || style->hasMask() || style->boxReflect() || style->hasFilter() || style->isPositioned() || (m_checker.document()->settings()->fixedElementsCreateStackingContext() && style->position() == FixedPosition)

Does fixedElementsCreateStackingContext really need to be a setting, or can this just be an #ifdef for your platform?

[Yael]

(In reply to comment #3)
> (From update of attachment 138180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138180&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2009
> > +                                   || style->hasTransformRelatedProperty() || style->hasMask() || style->boxReflect() || style->hasFilter() || style->isPositioned() || (m_checker.document()->settings()->fixedElementsCreateStackingContext() && style->position() == FixedPosition)
> 
> Does fixedElementsCreateStackingContext really need to be a setting, or can this just be an #ifdef for your platform?
Thanks for reviewing :)
I'll make it #ifdef

[Yael]

Created attachment 138187 [details]
Patch.

Removed the setting, and added #ifdef PLATFORM(QT) instead.

[Simon Fraser (smfr)]

Comment on attachment 138187 [details]
Patch.

I think this goes too far in the other direction :)

We try to avoid seemingly arbitrary platform #ifdefs, because they make the code very confusing and hard to maintain.

This also interacts with code in RLC::requireLayerForPosition(), so that needs to be communicated in the code somehow (perhaps by a FIXED_POSITION_CREATES_STACKING_CONTEXT #ifdef).

[Yael]

Created attachment 138253 [details]
Patch.

Update the patch based on comment #6.

[Noam Rosenthal]

Comment on attachment 138253 [details]
Patch.

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

> Source/WebCore/WebCore.pri:199
> +    DEFINES += ENABLE_FIXED_POSITION_CREATES_STACKING_CONTEXT=1

Add a comment.

[Yael]

Created attachment 138271 [details]
Patch.

Added comment in WebCore.pri .

[Noam Rosenthal]

I'm ok with this. smfr?

[Simon Fraser (smfr)]

Comment on attachment 138271 [details]
Patch.

Is there precedent for ENABLE flags being defined only for certain platforms like this (rather than via Platform.h)? This really isn't a feature, which is what ENBALE flags are normally used for. It's just a hack. A more localized

#if PLATFORM(foo)
#define FIXED_POSITION_CREATES_STACKING_CONTEXT 1
#endif

at the top of the file might be better.

[Yael]

Created attachment 138607 [details]
Patch.

As discussed with smfr on IRC, we should define the flag ENABLE_FIXED_POSITION_CREATES_STACKING_CONTEXT at the top of CSSStyleSelector.cpp. That will make it more visible to others.

[Simon Fraser (smfr)]

Comment on attachment 138607 [details]
Patch.

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

Why does the test have to be manual?

> Source/WebCore/css/CSSStyleSelector.cpp:152
> +#define ENABLE_FIXED_POSITION_CREATES_STACKING_CONTEXT 1

Just FIXED_POSITION_CREATES_STACKING_CONTEXT please, it's not an ENABLE flag.

[Yael]

Created attachment 138624 [details]
Patch for landing.

[Yael]

(In reply to comment #13)
> (From update of attachment 138607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138607&action=review
> 
> Why does the test have to be manual?
> 
This is a manual test because the problem shows only during scrolling. When scrolling stops, the background gets adjusted. I am not sure how to show this issue in a layout test.


> > Source/WebCore/css/CSSStyleSelector.cpp:152
> > +#define ENABLE_FIXED_POSITION_CREATES_STACKING_CONTEXT 1
> 
> Just FIXED_POSITION_CREATES_STACKING_CONTEXT please, it's not an ENABLE flag.
ok.

[Yael]

Committed r115134: <http://trac.webkit.org/changeset/115134>