Bug 87186 – Add a Setting to make position:fixed form a new stacking context

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

Add a Setting to make position:fixed form a new stacking context

[James Robinson]

Created attachment 143407 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 143407 [details]
Patch

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

> Source/WebKit/chromium/public/WebSettings.h:153
> +    virtual void setFixedPositionCreatesStackingContext(bool) = 0;

Do you think we should alphabetize this list?

[James Robinson]

Comment on attachment 143407 [details]
Patch

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

>> Source/WebKit/chromium/public/WebSettings.h:153
>> +    virtual void setFixedPositionCreatesStackingContext(bool) = 0;
> 
> Do you think we should alphabetize this list?

I don't know, I couldn't discern any existing organization in this list so I just added this to the end.

[Adam Barth]

Ok.  I can post an alphabetization patch after you land this one.

[James Robinson]

Committed r118095: <http://trac.webkit.org/changeset/118095>

[WebKit Review Bot]

Re-opened since this is blocked by 87201

[Kenneth Rohde Christiansen]

Comment on attachment 143407 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:2052
>  #ifdef FIXED_POSITION_CREATES_STACKING_CONTEXT
>          || style->position() == FixedPosition
> +#else
> +        || (style->position() == FixedPosition && e->document()->page()->settings()->fixedPositionCreatesStackingContext())
>  #endif

Why not just turn this on for Qt? Yael?

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (From update of attachment 143407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143407&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:2052
> >  #ifdef FIXED_POSITION_CREATES_STACKING_CONTEXT
> >          || style->position() == FixedPosition
> > +#else
> > +        || (style->position() == FixedPosition && e->document()->page()->settings()->fixedPositionCreatesStackingContext())
> >  #endif
> 
> Why not just turn this on for Qt? Yael?

I mean I believe we are the only ones using FIXED_POSIT... but I guess we can use the setting instead

[Arvid Nilsson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 143407 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143407&action=review
> > 
> > > Source/WebCore/css/StyleResolver.cpp:2052
> > >  #ifdef FIXED_POSITION_CREATES_STACKING_CONTEXT
> > >          || style->position() == FixedPosition
> > > +#else
> > > +        || (style->position() == FixedPosition && e->document()->page()->settings()->fixedPositionCreatesStackingContext())
> > >  #endif
> > 
> > Why not just turn this on for Qt? Yael?
> 
> I mean I believe we are the only ones using FIXED_POSIT... but I guess we can use the setting instead

The BlackBerry port also opts in to FIXED_POSIT... these days. But we'd also be happy to use the setting instead.

[James Robinson]

I think it'd be a good idea for other ports to move over to the Setting, but I'll leave that to decision up to them.

[Yael]

(In reply to comment #8)
> (From update of attachment 143407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143407&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:2052
> >  #ifdef FIXED_POSITION_CREATES_STACKING_CONTEXT
> >          || style->position() == FixedPosition
> > +#else
> > +        || (style->position() == FixedPosition && e->document()->page()->settings()->fixedPositionCreatesStackingContext())
> >  #endif
> 
> Why not just turn this on for Qt? Yael?

Please see https://bugs.webkit.org/show_bug.cgi?id=83980#c3

[James Robinson]

In chromium we need to have this be a setting to verify compatibility issues.  I think Simon's other comment is out of date, requiresCompositingForPosition() is aware of the acceleratedCompositingForFixedPositionEnabled compositing trigger which should cover this.

[James Robinson]

Created attachment 143649 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143649 [details]
Patch for landing

Clearing flags on attachment: 143649

Committed r118263: <http://trac.webkit.org/changeset/118263>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ádám Kallai]

This test fails on Qt. 

 * fast/block/positioning/fixed-position-stacking-context.html

Diff:

--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/block/positioning/fixed-position-stacking-context-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/block/positioning/fixed-position-stacking-context-actual.txt 
@@ -8,6 +8,6 @@
 layer at (0,0) size 100x100
   RenderBlock (positioned) zI: 1 {DIV} at (0,0) size 100x100 [bgcolor=#FF0000]
 layer at (0,0) size 100x100
+  RenderBlock (positioned) zI: 2 {DIV} at (0,0) size 100x100 [bgcolor=#008000]
+layer at (0,0) size 100x100
   RenderBlock (positioned) zI: 1 {DIV} at (0,0) size 100x100 [bgcolor=#FF0000]
-layer at (0,0) size 100x100
-  RenderBlock (positioned) zI: 2 {DIV} at (0,0) size 100x100 [bgcolor=#008000]

We got a red rectangle on Qt platform. Could you check it, please?

[Kristóf Kosztyó]

I skipped the test in r118346: <http://trac.webkit.org/changeset/118346> to paint the bot green.

[James Robinson]

(In reply to comment #17)
> This test fails on Qt. 
> 
>  * fast/block/positioning/fixed-position-stacking-context.html
> 
> Diff:
> 
> --- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/block/positioning/fixed-position-stacking-context-expected.txt 
> +++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/block/positioning/fixed-position-stacking-context-actual.txt 
> @@ -8,6 +8,6 @@
>  layer at (0,0) size 100x100
>    RenderBlock (positioned) zI: 1 {DIV} at (0,0) size 100x100 [bgcolor=#FF0000]
>  layer at (0,0) size 100x100
> +  RenderBlock (positioned) zI: 2 {DIV} at (0,0) size 100x100 [bgcolor=#008000]
> +layer at (0,0) size 100x100
>    RenderBlock (positioned) zI: 1 {DIV} at (0,0) size 100x100 [bgcolor=#FF0000]
> -layer at (0,0) size 100x100
> -  RenderBlock (positioned) zI: 2 {DIV} at (0,0) size 100x100 [bgcolor=#008000]
> 
> We got a red rectangle on Qt platform. Could you check it, please?

Since you are using the #define to control this behavior instead of the Setting, you'll need Qt-specific baselines for this test.

[Iain Merrick]

*** Bug 85067 has been marked as a duplicate of this bug. ***