Bug 111670 – Support bottom-right anchored fixed-position elements during a pinch gesture

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

Implement pinch zoom for bottom-right fixed-position elements

[Tien-Ren Chen]

Created attachment 191899 [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.

[Build Bot]

Comment on attachment 191899 [details]
Patch

Attachment 191899 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17000343

[Tien-Ren Chen]

Created attachment 191901 [details]
Patch

[Sami Kyöstilä]

Comment on attachment 191901 [details]
Patch

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

> Source/Platform/chromium/public/WebLayer.h:199
> +    virtual void setFixedToRightEdge(bool) { }

I wonder if we should have something more like the bitfields in the ViewportConstraints instead of this? That'll also cover cases where a layer is fixed to both edges on the same axis, in which case we may want to do something special in the compositor.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1921
> +RenderObject* RenderLayerCompositor::requiresFixedToContainerByRenderObject(const RenderLayer* layer) const

This function name is kinda hard to parse. Maybe fixedToContainerByRenderObject? The description doesn't seem to match the implementation since we can also return the layer itself (so the RenderObject won't be between the layer and its nearest compositor ancestor).

[Tien-Ren Chen]

(In reply to comment #5)
> (From update of attachment 191901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191901&action=review
> 
> > Source/Platform/chromium/public/WebLayer.h:199
> > +    virtual void setFixedToRightEdge(bool) { }
> 
> I wonder if we should have something more like the bitfields in the ViewportConstraints instead of this? That'll also cover cases where a layer is fixed to both edges on the same axis, in which case we may want to do something special in the compositor.

I think you're right. I was too lazy to write a WebViewportConstraint. Will do.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1921
> > +RenderObject* RenderLayerCompositor::requiresFixedToContainerByRenderObject(const RenderLayer* layer) const
> 
> This function name is kinda hard to parse. Maybe fixedToContainerByRenderObject? The description doesn't seem to match the implementation since we can also return the layer itself (so the RenderObject won't be between the layer and its nearest compositor ancestor).

Yep... The range is actually [this, compositing ancestor)
The semantic is to return false if the layer doesn't need fixed position compensation, and true if it does, and if true also returns which RenderObject caused it.
I'll change the function name and refine the comments.

[James Robinson]

Comment on attachment 191901 [details]
Patch

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

Looks like you have a few refinements you plan to do with this patch.

>>> Source/Platform/chromium/public/WebLayer.h:199
>>> +    virtual void setFixedToRightEdge(bool) { }
>> 
>> I wonder if we should have something more like the bitfields in the ViewportConstraints instead of this? That'll also cover cases where a layer is fixed to both edges on the same axis, in which case we may want to do something special in the compositor.
> 
> I think you're right. I was too lazy to write a WebViewportConstraint. Will do.

Still waiting for this.

Also, do we need right-anchored right now?  I think the immediate need is just for bottom-anchored, isn't it?  We can add right-anchored later.

[Alexandre Elias]

Right-anchored is also solving a problem in that it will avoid jumping around on pinch zoom for those elements.  Given that the logic is identical, why not do both?

[James Robinson]

Ah, ok.  That's fine.

[Tien-Ren Chen]

Created attachment 193395 [details]
Patch

[Tien-Ren Chen]

Changes from last patch:
* Moved RenderLayerCompositor::fixedPositionedByAncestor to ScrollingCoordinatorChromium. It is a chromium-specific concept that only makes sense to our implementation.
* Added WebLayerPositionConstraint, deprecate fixedToContainerLayer flag. The structure can be extended to support sticky elements at a later time.

Why not reuse WebCore::ViewportConstraints? Because our approach to fixed position layer is significantly different so there are too many parameters that are useless to us.

Chromium patch coming soon.

[Tien-Ren Chen]

Created attachment 193420 [details]
Patch

[Sami Kyöstilä]

Comment on attachment 193420 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests. Can't test until chromium patch landed.

Presumably the basic fixed position layer functionality is covered by existing layout tests, right? We just don't have tests for bottom/right fixed things yet.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:239
> +static void clearPositionConstraintExcept(GraphicsLayer* layer, GraphicsLayer* except)

Bikeshed: clearPositionConstraintExceptForLayer()?

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:267
> +    // Avoid unnecessary commits

I'm not sure I get this. What does clearing the position constraint have to do with commits?

[Tien-Ren Chen]

(In reply to comment #13)
> (From update of attachment 193420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193420&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests. Can't test until chromium patch landed.
> 
> Presumably the basic fixed position layer functionality is covered by existing layout tests, right? We just don't have tests for bottom/right fixed things yet.

Yes exactly. And I have adjusted the stubs so we migrate to the new API without breaking existing tests.

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:239
> > +static void clearPositionConstraintExcept(GraphicsLayer* layer, GraphicsLayer* except)
> 
> Bikeshed: clearPositionConstraintExceptForLayer()?

SGTM. Will do.

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:267
> > +    // Avoid unnecessary commits
> 
> I'm not sure I get this. What does clearing the position constraint have to do with commits?

Modification to position constraints will need a commit, and the call might be made on layers all the time, even if the layer didn't change its positioning. 

Consider the situation that a layer is fixed-positioned, and a call to update position constraint has been made. If we just blindly clear all position constraint and set it again on the main layer, a commit will be triggered. That is why we need the ExceptForLayer thing, so we apply the updated constraint on the main layer in one shot, and an early exit can be made.

[Simon Fraser (smfr)]

I don't understand the bug title. You're not implementing pinch-zoom, you're altering the behavior when zooming, right?

[Tien-Ren Chen]

Created attachment 193683 [details]
Patch

[Tien-Ren Chen]

Changes:
* Renamed bug title
* Renamed the function skyostil@ mentioned

[Sami Kyöstilä]

Comment on attachment 193683 [details]
Patch

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

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:241
> +    if (layer && layer != except && scrollingWebLayerForGraphicsLayer(layer))

I think "scrollingWebLayerForGraphicsLayer" is a pretty misleading name (also true before your patch) because the returned layer may or may not have anything to do with scrolling -- it's just the platformLayer() we're interested in here and below. I don't think you should necessarily fix it in your patch, though.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:267
> +    // Avoid unnecessary commits

Thanks for explaining this. I'm on the fence about whether a simple if (backing->ancestorClippingLayer() != mainLayer) { ... } would be clearer but either way is fine by me.

[Tien-Ren Chen]

(In reply to comment #18)
> (From update of attachment 193683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193683&action=review
> 
> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:241
> > +    if (layer && layer != except && scrollingWebLayerForGraphicsLayer(layer))
> 
> I think "scrollingWebLayerForGraphicsLayer" is a pretty misleading name (also true before your patch) because the returned layer may or may not have anything to do with scrolling -- it's just the platformLayer() we're interested in here and below. I don't think you should necessarily fix it in your patch, though.

Can't agree with you more! I'm always wondering why we need a function for such simple access. I suggest we just replace it with ->platformLayer(), if everybody agree.

[Sami Kyöstilä]

(In reply to comment #19)
> Can't agree with you more! I'm always wondering why we need a function for such simple access. I suggest we just replace it with ->platformLayer(), if everybody agree.

Sounds good to me.

[Tien-Ren Chen]

Hello reviewers, do you want me to revise anything before starting to review this patch? Thanks!

[Tien-Ren Chen]

Reminder: this patch needs to be landed before the chromium counterpart.
Please review this patch first when you got the chance to review the chromium patch: https://codereview.chromium.org/12552004/
I will add the WebKit-side tests when the chromium patch gets landed. Thanks!

[Sami Kyöstilä]

I think this patch looks good to go.

[James Robinson]

Comment on attachment 193683 [details]
Patch

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

D'oh, I had comments I forgot to publish :(

> Source/Platform/chromium/public/WebLayer.h:34
> +// Remove after chromium patch landed
> +#include "WebLayerPositionConstraint.h"

Not sure I understand this constraint - what chromium patch needs to land to remove the need for this include?

> Source/Platform/chromium/public/WebLayer.h:199
> +    // Deprecated, remove after chromium patch landed
> +    virtual void setFixedToContainerLayer(bool) { }
> +    virtual bool fixedToContainerLayer() const { return positionConstraint().isFixedPosition; }

Our typical convention for this is to move the deprecated things to the bottom of the file under a big comment saying DEPRECATED.  Having it inline like this makes it harder to see

> Source/Platform/chromium/public/WebLayer.h:200
> +    // Use this instead, remove stub after chromium patch landed

Please document what these functions actually do first, then add another comment on its own line saying something like // FIXME: Make pure virtual after implementation lands.

> Source/Platform/chromium/public/WebLayer.h:202
> +    virtual WebLayerPositionConstraint positionConstraint() const { return WebLayerPositionConstraint(); }

Why do you need the getter?

[Tien-Ren Chen]

(In reply to comment #24)
> (From update of attachment 193683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193683&action=review
> 
> D'oh, I had comments I forgot to publish :(
> 
> > Source/Platform/chromium/public/WebLayer.h:34
> > +// Remove after chromium patch landed
> > +#include "WebLayerPositionConstraint.h"
> 
> Not sure I understand this constraint - what chromium patch needs to land to remove the need for this include?

The temporary stub below needs WebLayerPositionConstraint prototype. Let me change the comment to
"// Remove after making setPositionConstraint() pure virtual."

> > Source/Platform/chromium/public/WebLayer.h:199
> > +    // Deprecated, remove after chromium patch landed
> > +    virtual void setFixedToContainerLayer(bool) { }
> > +    virtual bool fixedToContainerLayer() const { return positionConstraint().isFixedPosition; }
> 
> Our typical convention for this is to move the deprecated things to the bottom of the file under a big comment saying DEPRECATED.  Having it inline like this makes it harder to see

Got it.

> > Source/Platform/chromium/public/WebLayer.h:200
> > +    // Use this instead, remove stub after chromium patch landed
> 
> Please document what these functions actually do first, then add another comment on its own line saying something like // FIXME: Make pure virtual after implementation lands.

Got it.

> > Source/Platform/chromium/public/WebLayer.h:202
> > +    virtual WebLayerPositionConstraint positionConstraint() const { return WebLayerPositionConstraint(); }
> 
> Why do you need the getter?

For the unit tests. The migration plan as following:
As we land the Chromium counterpart, WebLayerImpl::fixedToContainerLayer() will be removed, so it turns to fallback implementation WebLayerImpl::fixedToContainerLayer(), which will query positionConstraint() as now Chromium-side implements the new methods. At last, we'll land another patch in WebKit that completely removes fixedToContainerLayer() and update tests.

[Tien-Ren Chen]

Created attachment 195635 [details]
Patch

[James Robinson]

Comment on attachment 195635 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 195635 [details]
Patch

Clearing flags on attachment: 195635

Committed r147163: <http://trac.webkit.org/changeset/147163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.