Bug 83980 - Fixed background is scrolling in http://www.nieuwecode.nl/ in Qt webkit2
Summary: Fixed background is scrolling in http://www.nieuwecode.nl/ in Qt webkit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-14 10:19 PDT by Yael
Modified: 2012-04-24 16:46 PDT (History)
10 users (show)

See Also:


Attachments
Patch. (11.28 KB, patch)
2012-04-20 15:45 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (11.28 KB, patch)
2012-04-20 15:48 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (7.60 KB, patch)
2012-04-20 16:22 PDT, Yael
simon.fraser: review-
Details | Formatted Diff | Diff
Patch. (8.26 KB, patch)
2012-04-21 12:15 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (8.40 KB, patch)
2012-04-22 08:31 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (7.90 KB, patch)
2012-04-24 11:21 PDT, Yael
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (7.88 KB, patch)
2012-04-24 12:53 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-04-14 10:19:28 PDT
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.
Comment 1 Yael 2012-04-20 15:45:22 PDT
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.
Comment 2 Yael 2012-04-20 15:48:24 PDT
Created attachment 138180 [details]
Patch.

Fixed a typo in the changelog. sorry :)
Comment 3 Simon Fraser (smfr) 2012-04-20 16:04:07 PDT
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?
Comment 4 Yael 2012-04-20 16:09:03 PDT
(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
Comment 5 Yael 2012-04-20 16:22:09 PDT
Created attachment 138187 [details]
Patch.

Removed the setting, and added #ifdef PLATFORM(QT) instead.
Comment 6 Simon Fraser (smfr) 2012-04-20 17:12:56 PDT
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).
Comment 7 Yael 2012-04-21 12:15:38 PDT
Created attachment 138253 [details]
Patch.

Update the patch based on comment #6.
Comment 8 Noam Rosenthal 2012-04-21 15:36:58 PDT
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.
Comment 9 Yael 2012-04-22 08:31:29 PDT
Created attachment 138271 [details]
Patch.

Added comment in WebCore.pri .
Comment 10 Noam Rosenthal 2012-04-22 08:51:52 PDT
I'm ok with this. smfr?
Comment 11 Simon Fraser (smfr) 2012-04-24 10:55:10 PDT
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.
Comment 12 Yael 2012-04-24 11:21:54 PDT
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.
Comment 13 Simon Fraser (smfr) 2012-04-24 11:40:25 PDT
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.
Comment 14 Yael 2012-04-24 12:53:22 PDT
Created attachment 138624 [details]
Patch for landing.
Comment 15 Yael 2012-04-24 12:56:46 PDT
(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.
Comment 16 Yael 2012-04-24 16:45:46 PDT
Committed r115134: <http://trac.webkit.org/changeset/115134>