Bug 70103 – [chromium] Implement position:fixed in compositor thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Hin-Chung Lam]

One of the feature missing in compositor thread is position:fixed. This needs to get implemented.

[Hin-Chung Lam]

Created attachment 111002 [details]
Work in progress demo

This is really a work in progress as I'm trying to get familiar with the code. This patch is based on Vangelis's change and added change to CCLayerTreeHostCommon.cpp that implements the idea I described.

The algorithm is pretty simple, as we recursively compute the draw transform, keep a copy with scroll delta translation, then when we hit a layer that doesn't want scrolling then it switches to the unscrolled draw transform. This will keep all the children to have maintain proper transformation.

[Vangelis Kokkevis]

Comment on attachment 111002 [details]
Work in progress demo

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:136
> +

I generally expect that fixed position layers will be the exception rather than the rule. In addition scroll deltas will only happen when running on the thread. I would prefer to pay the penalty of adjusting the transform only when really necessary.  In other words, as we descend the layer hierarchy, if we find a fixed position layer then walk upwards until the enclosing scrolling parent and compute the transform delta.

[WebKit Review Bot]

Attachment 111002 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/page/Settings.cpp', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/WebCompositorImpl.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/src/WebCompositorImpl.cpp:122:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 111002 [details]
Work in progress demo

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

[Hin-Chung Lam]

Created attachment 111784 [details]
simple fixed element

[Hin-Chung Lam]

Created attachment 111785 [details]
fixed element in iframe

[Hin-Chung Lam]

Created attachment 111786 [details]
fixed element with parent transform

[Hin-Chung Lam]

Created attachment 111787 [details]
fixed element with local transform

[Hin-Chung Lam]

Created attachment 111788 [details]
Work in progress, attempt 2

The is the second attempt for implementing position:fixed in compositor thread.

This change assumes a position:fixed layer is a direct descendant from tree root. It walks up the tree to determine the scroll delta from parent and offset the scroll delta.

I've tested this with various test cases I've updated in this bug.

[Vangelis Kokkevis]

Comment on attachment 111788 [details]
Work in progress, attempt 2

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

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:91
> +    return getParentScrollDelta(layer->parent()) + layer->scrollDelta();

We should only be compensating for scrollDelta of the closest enclosing scrolling frame, not all of them.

> WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:184
> +        position += getParentScrollDelta(layer->parent());

I'm afraid I may have mislead you with my previous comments.  You'll need to first transform the frame's scroll delta to the layer's coordinate system before adjusting the position.  What you have here won't work if, for example, one of the ancestors of the fixed position layer is rotated.

[Hin-Chung Lam]

(In reply to comment #10)
> (From update of attachment 111788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111788&action=review
> 
> > WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:91
> > +    return getParentScrollDelta(layer->parent()) + layer->scrollDelta();
> 
> We should only be compensating for scrollDelta of the closest enclosing scrolling frame, not all of them.
> 
> > WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:184
> > +        position += getParentScrollDelta(layer->parent());
> 
> I'm afraid I may have mislead you with my previous comments.  You'll need to first transform the frame's scroll delta to the layer's coordinate system before adjusting the position.  What you have here won't work if, for example, one of the ancestors of the fixed position layer is rotated.

I agree, that matches what I was expected too..

Actually I'm not so sure about separating the logic for transformation of position:fixed elements (i.e. walk up the tree and walk down again to recompute transformation with scroll). I worry that we'll have to duplicate the logic for transformation in two places.. note that calculateDrawTransformsAndVisibilityInternal has side effect and I don't think it's suitable for re-computation of transformation without scroll delta.

On the other hand, if we maintain a nonscrolled transformation, most of the time it'll just be a copy of the scrolled, so we can probably shortcut the extra matrix multiplications.

[Vangelis Kokkevis]

> I agree, that matches what I was expected too..
> 
> Actually I'm not so sure about separating the logic for transformation of position:fixed elements (i.e. walk up the tree and walk down again to recompute transformation with scroll). I worry that we'll have to duplicate the logic for transformation in two places.. note that calculateDrawTransformsAndVisibilityInternal has side effect and I don't think it's suitable for re-computation of transformation without scroll delta.
> 
> On the other hand, if we maintain a nonscrolled transformation, most of the time it'll just be a copy of the scrolled, so we can probably shortcut the extra matrix multiplications.

The code in CCLayerTreeHostCommon computes a "screenspace" transform for every layer, which accounts for the entire chain of transforms from the root to the layer.

To adjust the position of a fixed layer, we need to subtract the scroll offset of the scrolling container from the layer's current position.  However, the layer's position is expressed in the layer's parent coordinate system so we cannot apply the scroll offset directly.  We could however presumably use the screenspace transform of the parent and that of the scrolling container to compute the offset in the right space.

So if the scroll offset expressed in the container's coordinate system is given by vector o(c) and we have the following screen space matrices:

M(c) : screen space matrix for scrolling container
M(p) : screen space matrix for the parent of the fixed layer

Then: o(p) = M(p)^-1 * M(c) * o(c)

should the the scroll offset expressed in the layer's parent coordinate system.  We should be able to subtract o(p) from the current layer position to get it to stay fixed.  

This should avoid additional (and possibly unnecessary) matrix math going down the hierarchy.

[Hin-Chung Lam]

(In reply to comment #12)
> > I agree, that matches what I was expected too..
> > 
> > Actually I'm not so sure about separating the logic for transformation of position:fixed elements (i.e. walk up the tree and walk down again to recompute transformation with scroll). I worry that we'll have to duplicate the logic for transformation in two places.. note that calculateDrawTransformsAndVisibilityInternal has side effect and I don't think it's suitable for re-computation of transformation without scroll delta.
> > 
> > On the other hand, if we maintain a nonscrolled transformation, most of the time it'll just be a copy of the scrolled, so we can probably shortcut the extra matrix multiplications.
> 
> The code in CCLayerTreeHostCommon computes a "screenspace" transform for every layer, which accounts for the entire chain of transforms from the root to the layer.
> 
> To adjust the position of a fixed layer, we need to subtract the scroll offset of the scrolling container from the layer's current position.  However, the layer's position is expressed in the layer's parent coordinate system so we cannot apply the scroll offset directly.  We could however presumably use the screenspace transform of the parent and that of the scrolling container to compute the offset in the right space.
> 
> So if the scroll offset expressed in the container's coordinate system is given by vector o(c) and we have the following screen space matrices:
> 
> M(c) : screen space matrix for scrolling container
> M(p) : screen space matrix for the parent of the fixed layer
> 
> Then: o(p) = M(p)^-1 * M(c) * o(c)
> 
> should the the scroll offset expressed in the layer's parent coordinate system.  We should be able to subtract o(p) from the current layer position to get it to stay fixed.  
> 
> This should avoid additional (and possibly unnecessary) matrix math going down the hierarchy.

I'm actually thinking of a different formula to get the unscrolled screen space matrix:

M(u) : unscrolled screen matrix for the parent of the fixed layer
M(-o(c)) : local transformation matrix that reverse the scroll delta

M(u) = M(c) * M(-o(c)) * M(c)^-1 * M(p)

[Hin-Chung Lam]

Created attachment 113051 [details]
fixed in transformed iframe

[Hin-Chung Lam]

Created attachment 113052 [details]
Patch 3

[Hin-Chung Lam]

Hi Vangelis, I've updated the patch after the discussion we had. Please take a look to see if this is ready for review or I should make more changes.

[Vangelis Kokkevis]

Comment on attachment 113052 [details]
Patch 3

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

Patch looks good.  My main concern is the naming for the GraphicsLayer addition.  I was thinking of splitting this up to a WebKit common patch that just sets the properties on GraphicsLayer and an additional chromium-specific patch that handles scrolling.

> Source/WebCore/platform/graphics/GraphicsLayer.h:362
> +    bool anchorLayer() const { return m_anchorLayer; }

The word "anchor" is already used by "anchorPoint" which makes it confusing to reuse for a different purpose. Let's see what we're able to check into webkit.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:107
> +template<typename LayerType> static IntSize enclosingAncestorScrollDelta(LayerType* layer) {

The name of this function should reflect that it adds up deltas up to the enclosing ancestor.  Maybe something like: cummulativeScrollDeltasFromEnclosingAncestor() ? 

Also, have you considered implementing it as a while() loop instead of a recursion?  It will be faster.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:209
> +    // and the anchor layer.

It would be useful to add a short blurb here as to why there are no other transformations in between.

[Vangelis Kokkevis]

One more observation:  It looks like we're not marking the right layer as a container. The scroll delta gets applied to a layer that's higher in the hierarchy so we miss it.

[Hin-Chung Lam]

Created attachment 113199 [details]
Attempt 4

[Hin-Chung Lam]

Created attachment 113475 [details]
WebCore common changes

[Hin-Chung Lam]

Created attachment 113481 [details]
Patch

[Hin-Chung Lam]

Created attachment 113482 [details]
Chromium port changes

[WebKit Review Bot]

Comment on attachment 113482 [details]
Chromium port changes

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

[Hin-Chung Lam]

Taking reviews off since I'm still making some changes.

[Sami Kyostila]

Taking over from Alpha.

[Sami Kyostila]

Created attachment 138609 [details]
Patch

WIP patch. Still missing layout tests and support for intermediate render surfaces between a fixed layer and its container. Feel free to comment, e.g., the ScrollingCoordinator integration, though.

[Sami Kyostila]

Removing [chromium] from title since the latest patch also touches other WebCore parts. Please let me know if you'd prefer me to split the patch instead.

[WebKit Review Bot]

Attachment 138609 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 138609 [details]
Patch

Attachment 138609 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12529274

[James Robinson]

Comment on attachment 138609 [details]
Patch

Please at a minimum split the cross-platform bits (WebCore up to ScrollingCoordinator interface) from the chromium compositor implementation bits

[Build Bot]

Comment on attachment 138609 [details]
Patch

Attachment 138609 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12523326

[Early Warning System Bot]

Comment on attachment 138609 [details]
Patch

Attachment 138609 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12526346

[Early Warning System Bot]

Comment on attachment 138609 [details]
Patch

Attachment 138609 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12528323

[Iain Merrick]

Experimenting with this patch, I found a bug: if you scroll the page, go back to the previous page, then forward again, the old scroll position is restored, but it is not taken into account in the positioning of the composited fixed-position layers. If you scroll further, the fixed-position layers pop into the correct position.

It seems like HistoryController::restoreScrollPositionAndViewState is not being propagated through to the compositor correctly. I'm looking for a fix. Not sure yet if it's specific to this patch; I'll file a separate bug if it's a existing latent problem.

[Iain Merrick]

OK, I think the problem is that the scroll position is restored while we're inside FrameView::layout(), and updateFixedElementsAfterScrolling() looks like this:

  void FrameView::updateFixedElementsAfterScrolling()
  {
  #if USE(ACCELERATED_COMPOSITING)
      if (!m_nestedLayoutCount && hasFixedObjects()) {
          if (RenderView* root = rootRenderer(this)) {
              root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll);
          }
      }
  #endif
  }

m_nestedLayoutCount is 1, so we don't call updateCompositingLayers().

[Iain Merrick]

This bug is independent of Sami's patch. Filed as https://bugs.webkit.org/show_bug.cgi?id=84965

[Sami Kyostila]

This patch now depends on the WebCore changes in bug 78864.

[Sami Kyostila]

Created attachment 139271 [details]
Patch

[Sami Kyostila]

Created attachment 142519 [details]
Patch

[Sami Kyostila]

N.B. the unit test in this version doesn't build since 73350 was reverted.

[James Robinson]

Comment on attachment 142519 [details]
Patch

I don't think this will even apply without 73350.  I would strongly prefer that we get 73350 landed and stabilized first so we can make sure we're on a good foundation and can test this patch as soon as landing it. Having untested and untestable code in the tree is a tax on everyone who wants to modify it.

[Sami Kyostila]

(In reply to comment #41)
> I don't think this will even apply without 73350.  I would strongly prefer that we get 73350 landed and stabilized first so we can make sure we're on a good foundation and can test this patch as soon as landing it. Having untested and untestable code in the tree is a tax on everyone who wants to modify it.

Definitely agreed. I'll get 73350 relanded before continuing with this.

[Sami Kyostila]

Created attachment 145072 [details]
Patch

Removed dependency on 73350 to avoid blocking this patch. Note that this still requires 78864 to be in place.

[Sami Kyostila]

Since it's taking a while to get 73350 in shape (because I need to verify it on ChromeOS), so I opted to break the dependency between these patches. Since they are mostly orthogonal and the only linkage was in the unit tests, this didn't result in any changes in the implementation.

This latest version together with 78864 gives us compositor thread positioned fixed position layers when --enable-fixed-position-compositing is used in Chromium. Note that since I didn't change the rules for layer promotion, you also need --fixed-position-creates-stacking-context or a transform on the fixed element for this to work.

[Dana Jansens]

(In reply to comment #44)
> Since it's taking a while to get 73350 in shape (because I need to verify it on ChromeOS), so I opted to break the dependency between these patches. Since they are mostly orthogonal and the only linkage was in the unit tests, this didn't result in any changes in the implementation.
> 
> This latest version together with 78864 gives us compositor thread positioned fixed position layers when --enable-fixed-position-compositing is used in Chromium. Note that since I didn't change the rules for layer promotion, you also need --fixed-position-creates-stacking-context or a transform on the fixed element for this to work.

Just curious, how do these layers behave when pageScaleFactor changes?

[Sami Kyostila]

(In reply to comment #45)
> Just curious, how do these layers behave when pageScaleFactor changes?

When page scale factor changes on the main thread, both the fixed layer and its container are scaled appropriately and the layer remains fixed relative the visible part of the container. For example, if the page scale factor increases, all layers get bigger and a fixed element starts covering a larger fraction of its container.

However, if the page scale changes as a response to a pinch gesture event on the compositor thread, it isn't applied to the fixed layers until the next commit from the main thread. Visually this means that the fixed layer drifts around while the user is pinch zooming. This isn't ideal, so I'll see if I can improve it.

[Vangelis Kokkevis]

Comment on attachment 145072 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:427
> +    // 2. A container layer's parent layer is a clip layer.

Do we care specifically that the parent of the container is a "clip" layer?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:462
> +                TransformationMatrix renderSurfaceMatrix = ancestor->transform().inverse();

I'm not convinced that this is correct.  It's not taking into account the anchor points of the ancestor layer which need to be used when applying the layer transform.  Should you be using the RS originTransform instead?

[Sami Kyostila]

Comment on attachment 145072 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:427
>> +    // 2. A container layer's parent layer is a clip layer.
> 
> Do we care specifically that the parent of the container is a "clip" layer?

Not really. It is just the layer that defines the coordinate space for container layer's scroll delta. However, since scroll layers are generally contained in clip layers, I think it makes sense to call it that here.

I've improved the comment to mention this.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:462
>> +                TransformationMatrix renderSurfaceMatrix = ancestor->transform().inverse();
> 
> I'm not convinced that this is correct.  It's not taking into account the anchor points of the ancestor layer which need to be used when applying the layer transform.  Should you be using the RS originTransform instead?

originTransform() is probably more appropriate, but it cannot be used as such because we end up applying the scroll offset to the fixed layer twice.

I tried to come up with a scheme to make this work, but it needs some more thought. Essentially we need to undo the orientation of the RS relative to the screen, but how the RS's sublayer transform should be applied to the fixed position is a bit unclear. Perhaps it shouldn't apply at all, but that would break the (normal) case where the RS is the fixed position container.

Meanwhile I'll attach a test page that shows the problem with originTransform(). Note that testing other types of RS/fixed interactions interactively is a bit tricky because WebCore needs to be changed to not consider transformed elements containers for fixed position.

[Sami Kyostila]

Created attachment 145350 [details]
Patch

Checkpoint, don't apply.

[Sami Kyostila]

Created attachment 145351 [details]
Transformed element within a fixed element in a translucent container.

[Vangelis Kokkevis]

Comment on attachment 145350 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:465
> +                // FIXME: renderSurfaceMatrix also includes the scroll offset.

I'm not sure I understand.  In what cases does the RSM include the scroll offset?

[Shawn Singh]

I think the math is accidentally a little bit wrong, even for the non-renderSurface case.  The problem is that we're using clipLayer->drawTransform.  However, this does not create the intended composite matrix for what combinedTransform (the local layer's draw transform) should be.  What we really wanted was clipLayer->parentMatrix() which is actually only a temporary matrix that is never stored.  It will be extremely messy and error-prone to walk the tree and recompute that matrix - we should probably try to find a different solution first.

Writing out the math explicitly (see the huge comment in CCLayerTreeHostCommon.cpp):
   A layer's draw transform is:
   M[draw] = M[parent] * Tr[parent2origin] * compositeLayerTransform * Tr[origin2center]

  A child layer's parentMatrix is:
   M[parent]_for_child = M[parent] * Tr[parent2origin] * compositeLayerTransform * compositeSubLayerTransform

The fixed position layer, for the sake of discussion here, is the child layer.  We're trying to decompose that so that we can insert an additional translation that cancels out the scroll.

So according to the comments in this patch, we decompose M[parent]_for_child into:
   M[clip] * Tr[container] * M[descendant]

However, M[clip] is unclear here, and that's where the mistake might be.  It's not supposed to be the clip layer's draw transform.  Instead, M[clip] should probably be the parentMatrix that is inherited by the scrolling layer.  So what we really need is clipLayer->parentMatrix() which we do not actually store.

Finally, its worth pointing out that I couldn't manually test any of this yet....  even though we removed 73350 as a dependency on this patch, actually it still is a dependency in my opinion because without that patch, scrollDelta is always 0,0.  That is the only way to manually test and develop this patch further, and not having scrollDelta plumbed through just makes it impossible to reason through this math.  Tomorrow I'll try to apply that patch, to iron out the math some more.

[Vangelis Kokkevis]

(In reply to comment #52)

> Finally, its worth pointing out that I couldn't manually test any of this yet....  even though we removed 73350 as a dependency on this patch, actually it still is a dependency in my opinion because without that patch, scrollDelta is always 0,0. 

You should be able to get a non-zero scrollDelta with the threaded compositor by scrolling the root layer.  It may be easier though to hack in some scroll delta values directly in the code to see how the math works out.

[Shawn Singh]

Created attachment 145886 [details]
Another example of fixed-position with a renderSurface in-between.

the red element should be fixed, and the blue element should scroll.

the reflection behavior seems broken on stable chromium and safari, but seems correct with the new scrolling patches (probably because something gets composited/stacked differently, seems like a bug in webcore to address separately.)

[Shawn Singh]

OK, I've gotten a decently complete picture of how this works now.

Summary:
I will submit a patch that refactors this to be more intuitive and clean.  Sami's math was mostly already correct, and I've also figured out the RenderSurface case.

Details:
In theory its slightly wrong  to use drawTransform instead of parentMatrix, but it turns out the differences are provably not going to affect the math, under current W3C specs.

There's also a much more intuitive interpretation of the math, which helps illustrate all cases in general.  Key observation: to undo the scroll, we have to apply the scroll in the space of the "clip" layer.  So we do the following steps (note carefully, interpreting the transforms from left-to-right):
    1. transform from fixed layer's target surface space to clip layer space.
    2. apply the "undoScrollTranslation"
    3. transform back to fixed layer's target surface space
    4. continue with using the normal parentMatrix, which transforms from target surface space to local layer space.

In math/code:

    scrollCompensation = clipLayerDrawTransform * undoScrollTranslation * clipLayerDrawTransform.inverse();

    if layer is fixedPosition
        combinedTransform = scrollCompensation * parentMatrix;
    else
        combinedTransform = parentMatrix;


For renderSurfaces, the same 4 steps above still apply.  But steps (1) and (3) are just a little bit more involved, because fixed layer's target surface space is not the same as clip layer's target surface space.

    localTargetSpaceToClipLayerSpace = renderSurface.originTransform.inverse() * clipLayerDrawTransform;
    scrollCompensation = localTargetSpaceToClipLayerSpace * underScrollTranslation * localTargetSpaceToClipLayerSpace.inverse();


I believe this should work correctly in all cases.

I attached my own version of the renderSurface case, too.  Both Sami's and my attachments seem to work correctly with my new patch; I'll submit the new patch after adding some more unit tests.

[Shawn Singh]

Created attachment 145907 [details]
In good condition, but still needs more unit tests

This patch passes all 7 tests attached on this bug when testing manually (using usleep() to make main thread really slow).  Changelogs and unit tests still need updating.  Compared to the previous patch, for now I only modified CCLayerTreeHostCommon.cpp.

[James Robinson]

(In reply to comment #56)
> Created an attachment (id=145907) [details]
> In good condition, but still needs more unit tests
> 
> This patch passes all 7 tests attached on this bug when testing manually (using usleep() to make main thread really slow).  Changelogs and unit tests still need updating.  Compared to the previous patch, for now I only modified CCLayerTreeHostCommon.cpp.

FYI, there are some test cases here that might be helpful: http://webstuff.nfshost.com/scrolling/  They are not exhaustive.

[Shawn Singh]

(In reply to comment #57)
> (In reply to comment #56)
> > Created an attachment (id=145907) [details] [details]
> > In good condition, but still needs more unit tests
> > 
> > This patch passes all 7 tests attached on this bug when testing manually (using usleep() to make main thread really slow).  Changelogs and unit tests still need updating.  Compared to the previous patch, for now I only modified CCLayerTreeHostCommon.cpp.
> 
> FYI, there are some test cases here that might be helpful: http://webstuff.nfshost.com/scrolling/  They are not exhaustive.

Thanks, those links seem to work, too.

[Shawn Singh]

Received Sami's go-ahead to own this bug to completion.  As I understand, he's actively working on testing/landing 78864 which this patch will depend on.

[Shawn Singh]

Created attachment 146216 [details]
Polished and buffed, and with unit tests

This patch is now pretty clean and robust, including unit tests. Please note that I removed the ScrollingCoordinator code from this patch; if you want, this patch could even land before 78864 (the included unit tests will cover the code in this patch even without 78864).

[Sami Kyostila]

Comment on attachment 146216 [details]
Polished and buffed, and with unit tests

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:352
> +        // Since we're going backwards up the hierarchy, the surface's inverse originTransform should pre-multiply the accumulating transform.

I'm trying to figure out whether we should also undo scroll offsets from all ancestor layers that are *not* the container. That could happen if the fixed element is embedded in a scrollable child layer but the root layer is the fixed position container.

I think one way to trigger this would be to use -webkit-overflow-scrolling: touch to cause a layer to be created for the scrollable child layer.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:425
> +    if ((scrollDelta.width() || scrollDelta.height())) {

Nit: could use !scrollDelta.isZero().

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:570
>      WebTransformationMatrix combinedTransform = parentMatrix;

Nit: could leave combinedTransform uninitialized at this point since we always assign to it below.

[Shawn Singh]

Thanks for the comments - I'll submit a new patch based on these.  I'll also fix a few variable names that can be clearer and clean up the unit tests a bit more.

One question below:

(In reply to comment #61)
> (From update of attachment 146216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146216&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:352
> > +        // Since we're going backwards up the hierarchy, the surface's inverse originTransform should pre-multiply the accumulating transform.
> 
> I'm trying to figure out whether we should also undo scroll offsets from all ancestor layers that are *not* the container. That could happen if the fixed element is embedded in a scrollable child layer but the root layer is the fixed position container.
> 
> I think one way to trigger this would be to use -webkit-overflow-scrolling: touch to cause a layer to be created for the scrollable child layer.

Are you referring to undoing the scroll offsets so that they affect the final transform of the surface?  or the final transform of the fixed-position layer?   I don't think we need to change any transforms on any other layers except the fixed-position one.  And for that layer, the scroll offsets that are embedded in intermediate renderSurfaces do not affect the actual scroll compensation we need to apply since we apply transformToContainerSpace * scroll translation * transformToContainerSpace.inverse().   Or am I misunderstanding what you're saying?

[Sami Kyostila]

(In reply to comment #62)
> Are you referring to undoing the scroll offsets so that they affect the final transform of the surface?  or the final transform of the fixed-position layer?   I don't think we need to change any transforms on any other layers except the fixed-position one.  And for that layer, the scroll offsets that are embedded in intermediate renderSurfaces do not affect the actual scroll compensation we need to apply since we apply transformToContainerSpace * scroll translation * transformToContainerSpace.inverse().   Or am I misunderstanding what you're saying?

Ah, I meant the undoing the scroll deltas for the fixed position layer itself. Scroll deltas coming from any ancestor layer and not just render surfaces to be exact. For example:

<div style="overflow: scroll; -webkit-overflow-scrolling: touch">
  <div style="position: fixed">
  </div>
</div>

I think in this case the fixed element should stay fixed to the view root.

[Shawn Singh]

(In reply to comment #63)
> (In reply to comment #62)
> > Are you referring to undoing the scroll offsets so that they affect the final transform of the surface?  or the final transform of the fixed-position layer?   I don't think we need to change any transforms on any other layers except the fixed-position one.  And for that layer, the scroll offsets that are embedded in intermediate renderSurfaces do not affect the actual scroll compensation we need to apply since we apply transformToContainerSpace * scroll translation * transformToContainerSpace.inverse().   Or am I misunderstanding what you're saying?
> 
> Ah, I meant the undoing the scroll deltas for the fixed position layer itself. Scroll deltas coming from any ancestor layer and not just render surfaces to be exact. For example:
> 
> <div style="overflow: scroll; -webkit-overflow-scrolling: touch">
>   <div style="position: fixed">
>   </div>
> </div>
> 
> I think in this case the fixed element should stay fixed to the view root.


Is it possible to have non-zero scroll deltas on a non-container layer?  If so, then yeah we have to completely rearrange the algorithm.   If not, then its only a concern about the root layer, and I'll test and check, I think it should work.  And I suppose we need a few more debug assertions if we're allowed to assume that non-containers will always have zero scroll.

[Sami Kyostila]

(In reply to comment #64)
> Is it possible to have non-zero scroll deltas on a non-container layer?  If so, then yeah we have to completely rearrange the algorithm.   If not, then its only a concern about the root layer, and I'll test and check, I think it should work.  And I suppose we need a few more debug assertions if we're allowed to assume that non-containers will always have zero scroll.

I think in my example the enclosing overflow div will not be marked as a container because it doesn't have a CSS transform, and since the layer is scrollable it may indeed have a non-zero scroll delta.

Note that this doesn't happen with current master because there's ever only one scrollable layer, but with the patch from 73350 I think it's possible.

Practically I guess this could be handled by undoing all scroll deltas from the fixed layer up to the first container.

[Shawn Singh]

Created attachment 146365 [details]
Patch for archival, still need to address comment 65

It looks like we definitely should wait for other patches to land so we can better test more possible things that can happen on web pages. I'm just uploading this for backup and will revisit this in just a few days. Thanks!

[Shawn Singh]

isContainerForFixedPositionLayers is a very sore misnomer.

in the DOM, yes technically it is an ancestor that contains the fixedPositionLayer.  But with the current approach we are taking, it is not the layer that the fixedPositionLayer is actually fixed onto.

We should either revise our approach to set this flag on the layer just above it (needs a bit more careful thought), or we should re-name this flag.

[Shawn Singh]

Created attachment 146492 [details]
Rewrote from scratch, added more tests, feels solid

[Shawn Singh]

Almost done!

Summary:

Vangelis and James, this is ready for review. I'm feeling good about this new algorithm and tests in this patch.  In addition to unit tests, I manually tested a good bit, and everything seems to work as desired.  Hopefully we can land the final version tomorrow (Friday)!!

Details:

(1) Because of comment #65, and thanks to fruitful offline discussion with Vangelis, it became clear that we had to rewrite this from scratch with a new approach, so that we can remove undesirable assumptions.  This patch uses the strictly correct geometric space where scrollDelta needs to be undone.  In addition to being more correct, this way we don't have to make assumptions about the relationships between layers to ensure we're in the correct space.  It also makes the concept of the "clipLayer" unnecessary, and therefore it is not as fragile to walk the hierarchy.

(2) Vangelis suggested we should go ahead and try to land it ASAP, and the new unit tests on this patch already cover the scrolling possibilities that we're worried about.  It will be far easier and less risky to back-merge smaller fix after m21 if needed, rather than delaying this and needing to merge this large patch after it branches.

(3) Added more unit tests, in particular to verify that the ordering of transforms is computed correctly when compensating for the scroll, and that multiple scrollDeltas can exist before finding the container for fixed position elements.

(4) Re-named transformation matrix variables, so that the name represents the right-to-left interpretation of what the transform actually does.  This is backwards from the previous patch, apologies if this causes drastic confusion.

[Shawn Singh]

Created attachment 146659 [details]
Again from scratch, tested and still feels solid

Vangelis and I had another discusison about this and decided to try seeing what it would look like if we computed scrollCompensation in the recursion itself. It works just as well as the previous patch, just a different algorithm. It does seem like less scary code, but I think it depends on who you ask whether its actually more complicated or less complicated =) At any rate, both this patch and the previous one should work equally well, whichever one reviewers like better is fine with me. Thanks!

[Vangelis Kokkevis]

Comment on attachment 146659 [details]
Again from scratch, tested and still feels solid

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

I like this approach better.  Thanks for writing it up!  One comment to avoid adding unnecessary operations in the common path.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:540
> +        // Special case: this layer is a composited fixed-position element; we need to

nit: I would say "fixed position _layer_" instead of "fixed position _element_" since the compositor doesn't know about HTML elements.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:703
> +    WebTransformationMatrix nextScrollCompensationMatrix = computeScrollCompensationMatrixForChildren(layer, parentMatrix, currentScrollCompensationMatrix);;

Since having a scroll delta or creating a RS are somewhat rare occurrences, it would be more efficient to eliminate the above function and move the code that computes the next matrix here.  In the most common case we can pass "currentScrollCompensationMatrix" down the recursion (no matrix copy necessary) and overwrite it only when needed.

[Adrienne Walker]

Comment on attachment 146659 [details]
Again from scratch, tested and still feels solid

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

I like this approach a lot.

Can you add one more test where a fixed position layer is separated from its container by at least two intermediate render surfaces?

> Source/WebCore/ChangeLog:8
> +        Unit tests added to CCLayerTreeHostCommonTest:

nit: put the testing block below the description, like layout tests are listed.

> Source/WebCore/ChangeLog:28
> +        its own. This allows fixed-position elements to be composited,

Not sure what you mean by composited here.  "This allows fixed position layers to be positioned correctly when scrolling in the compositor" maybe?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:239
> +    const WebKit::WebTransformationMatrix& scrollDeltaSpaceToSurfaceTransform() { return m_scrollDeltaSpaceToSurfaceTransform; }
> +    void setScrollDeltaSpaceToSurfaceTransform(const WebKit::WebTransformationMatrix& matrix) { m_scrollDeltaSpaceToSurfaceTransform = matrix; }

Unused?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:337
> +    // Set for the layer that has position fixed to the container layer's visible rect.

container => closest ancestor container?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:365
> +    //    it is fixed to an ancsestor, and is a container for any fixed-position descendants.

ancsestor => ancestor

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:541
> +        // explicitly compensate for all ancestors nonzero scrollDeltas to keep this layer

ancestors => ancestors'

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:707
> +    //
> +    // Recursion happens here.
> +    //

nit: needless comment.

[Shawn Singh]

Thanks for the comments.  New patch is coming in a moment, with the following differences:
- added an early exit that avoids stack allocation and matrix initialization/copy overheads in the computeScrollCompensationMatrixForChildren() helper function. This represents the compromise Vangelis and I reached discussing offline.
- added a new unit test for multiple intermediate surfaces
- addressed reviewers' nits

[Shawn Singh]

Created attachment 146909 [details]
Addressed reviewers comments

tested manually on all examples attached as well as some real pages, and passes all unit tests.

[Adrienne Walker]

Comment on attachment 146909 [details]
Addressed reviewers comments

R=me.  Thanks for the extra test.

[Shawn Singh]

Committed r120008: <http://trac.webkit.org/changeset/120008>

[Shawn Singh]

(In reply to comment #75)
> (From update of attachment 146909 [details])
> R=me.  Thanks for the extra test.

And thanks for the review =)