Bug 96688 - Put position:fixed elements in their own layers and allow them to create a stacking context
Summary: Put position:fixed elements in their own layers and allow them to create a st...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-09-13 13:18 PDT by Beth Dakin
Modified: 2012-09-14 22:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.09 KB, patch)
2012-09-13 13:22 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (3.81 KB, patch)
2012-09-13 13:31 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch with smfr's feedback (3.74 KB, patch)
2012-09-13 13:40 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch with updated test results (53.00 KB, patch)
2012-09-13 15:11 PDT, Beth Dakin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (55.52 KB, patch)
2012-09-14 13:44 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-09-13 13:18:37 PDT
Right now, pages with position:fixed elements are not able to be scrolled by the ScrollingCoordinator. We would like to change that. The first step to give position:fixed elements their own layers and to allow them to create a stacking context. 

<rdar://problem/11467961>
Comment 1 Beth Dakin 2012-09-13 13:22:08 PDT
Created attachment 163948 [details]
Patch
Comment 2 Beth Dakin 2012-09-13 13:31:10 PDT
Created attachment 163951 [details]
Patch
Comment 3 Simon Fraser (smfr) 2012-09-13 13:37:37 PDT
Comment on attachment 163951 [details]
Patch

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

> Source/WebKit/mac/WebView/WebView.mm:1553
> +    settings->setAcceleratedCompositingForFixedPositionEnabled(true);

I don' think we want to force compositing layers for position:fixed in WK1...

> Source/WebKit/mac/WebView/WebView.mm:1554
> +    settings->setFixedPositionCreatesStackingContext(true);

But we do want this one to avoid rendering differences.
Comment 4 Beth Dakin 2012-09-13 13:40:54 PDT
Created attachment 163953 [details]
Patch with smfr's feedback
Comment 5 Beth Dakin 2012-09-13 15:11:40 PDT
Created attachment 163977 [details]
Patch with updated test results
Comment 6 Sam Weinig 2012-09-13 18:34:39 PDT
Comment on attachment 163977 [details]
Patch with updated test results

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

> LayoutTests/ChangeLog:12
> +        A few test result differences. These first three tests actually
> +        render differently now.

So they render incorrectly?  Seeing expected results which is a red-box is very confusing.
Comment 7 WebKit Review Bot 2012-09-13 21:28:23 PDT
Comment on attachment 163977 [details]
Patch with updated test results

Attachment 163977 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13835866

New failing tests:
fast/block/positioning/016.html
fast/block/positioning/025.html
fast/block/positioning/fixed-position-stacking-context.html
compositing/geometry/fixed-position-composited-switch.html
Comment 8 WebKit Review Bot 2012-09-13 22:21:08 PDT
Comment on attachment 163977 [details]
Patch with updated test results

Attachment 163977 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13855287

New failing tests:
fast/block/positioning/016.html
fast/block/positioning/025.html
fast/block/positioning/fixed-position-stacking-context.html
compositing/geometry/fixed-position-composited-switch.html
Comment 9 Beth Dakin 2012-09-14 10:57:17 PDT
(In reply to comment #6)
> (From update of attachment 163977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163977&action=review
> 
> > LayoutTests/ChangeLog:12
> > +        A few test result differences. These first three tests actually
> > +        render differently now.
> 
> So they render incorrectly?  Seeing expected results which is a red-box is very confusing.

They don't render incorrectly; they render differently. The new results are expected given the change. These tests are explicitly doing things that test whether position:fixed creates a stacking context, so now that we are having it create a stacking context, they render differently.

I agree that the red boxes are confusing. However, this is going to necessarily render differently on ports that enable stacking context for fixed pos versus ports that do not. We could try to think of a way to change the tests that makes the differences less confusing.

If this is still confusing: yes, this change might break some web sites. According to jamesr, they have has this enabled in Chromium TOT for a bit now, and they found some bugs with Google sites and got the sites to change. They haven't found anything else major yet. We and they will be on the lookout for bugs.
Comment 10 Beth Dakin 2012-09-14 13:44:09 PDT
Created attachment 164220 [details]
Patch

In this patch I added comments to the files that render red to hopefully provide some clarification whenever people come across these tests later.

I guess chromium bots are failing because they honor test results in the platform/mac directory before looking at the test directory. Not sure what the standard way to deal with that is.
Comment 11 Beth Dakin 2012-09-14 14:25:09 PDT
Committed change with http://trac.webkit.org/changeset/128652
Comment 12 Beth Dakin 2012-09-14 18:38:30 PDT
This caused some test failures on the WK2 bots, which I have fixed. See:

https://bugs.webkit.org/show_bug.cgi?id=96846
http://trac.webkit.org/changeset/128678
Comment 13 Sam Weinig 2012-09-14 22:10:43 PDT
(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 163977 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163977&action=review
> > 
> > > LayoutTests/ChangeLog:12
> > > +        A few test result differences. These first three tests actually
> > > +        render differently now.
> > 
> > So they render incorrectly?  Seeing expected results which is a red-box is very confusing.
> 
> They don't render incorrectly; they render differently. The new results are expected given the change. These tests are explicitly doing things that test whether position:fixed creates a stacking context, so now that we are having it create a stacking context, they render differently.
> 
> I agree that the red boxes are confusing. However, this is going to necessarily render differently on ports that enable stacking context for fixed pos versus ports that do not. We could try to think of a way to change the tests that makes the differences less confusing.
> 
> If this is still confusing: yes, this change might break some web sites. According to jamesr, they have has this enabled in Chromium TOT for a bit now, and they found some bugs with Google sites and got the sites to change. They haven't found anything else major yet. We and they will be on the lookout for bugs.

My suggestion then would be to change all ports to the new stacking mode, even when not using layers for fixed positioning.  And then change the test to show green.  I don't see real benefit in fracturing WebKit here.