Bug 111670 - Support bottom-right anchored fixed-position elements during a pinch gesture
Summary: Support bottom-right anchored fixed-position elements during a pinch gesture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-06 19:31 PST by Tien-Ren Chen
Modified: 2013-03-28 14:49 PDT (History)
19 users (show)

See Also:


Attachments
Patch (11.31 KB, patch)
2013-03-06 19:36 PST, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (11.33 KB, patch)
2013-03-06 20:01 PST, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (16.97 KB, patch)
2013-03-15 16:17 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (16.95 KB, patch)
2013-03-15 21:10 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2013-03-18 16:21 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (17.36 KB, patch)
2013-03-28 13:48 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tien-Ren Chen 2013-03-06 19:31:25 PST
Implement pinch zoom for bottom-right fixed-position elements
Comment 1 Tien-Ren Chen 2013-03-06 19:36:23 PST
Created attachment 191899 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-06 19:43:27 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Build Bot 2013-03-06 19:51:21 PST
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
Comment 4 Tien-Ren Chen 2013-03-06 20:01:45 PST
Created attachment 191901 [details]
Patch
Comment 5 Sami Kyöstilä 2013-03-08 04:21:22 PST
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).
Comment 6 Tien-Ren Chen 2013-03-08 14:00:11 PST
(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.
Comment 7 James Robinson 2013-03-14 11:33:05 PDT
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.
Comment 8 Alexandre Elias 2013-03-14 11:36:24 PDT
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?
Comment 9 James Robinson 2013-03-14 12:36:36 PDT
Ah, ok.  That's fine.
Comment 10 Tien-Ren Chen 2013-03-15 16:17:19 PDT
Created attachment 193395 [details]
Patch
Comment 11 Tien-Ren Chen 2013-03-15 16:23:22 PDT
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.
Comment 12 Tien-Ren Chen 2013-03-15 21:10:14 PDT
Created attachment 193420 [details]
Patch
Comment 13 Sami Kyöstilä 2013-03-18 04:32:10 PDT
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?
Comment 14 Tien-Ren Chen 2013-03-18 12:26:36 PDT
(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.
Comment 15 Simon Fraser (smfr) 2013-03-18 13:08:18 PDT
I don't understand the bug title. You're not implementing pinch-zoom, you're altering the behavior when zooming, right?
Comment 16 Tien-Ren Chen 2013-03-18 16:21:00 PDT
Created attachment 193683 [details]
Patch
Comment 17 Tien-Ren Chen 2013-03-18 16:21:42 PDT
Changes:
* Renamed bug title
* Renamed the function skyostil@ mentioned
Comment 18 Sami Kyöstilä 2013-03-19 10:47:12 PDT
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.
Comment 19 Tien-Ren Chen 2013-03-19 13:31:31 PDT
(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.
Comment 20 Sami Kyöstilä 2013-03-20 03:23:07 PDT
(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.
Comment 21 Tien-Ren Chen 2013-03-21 16:08:31 PDT
Hello reviewers, do you want me to revise anything before starting to review this patch? Thanks!
Comment 22 Tien-Ren Chen 2013-03-27 20:06:27 PDT
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!
Comment 23 Sami Kyöstilä 2013-03-28 07:36:28 PDT
I think this patch looks good to go.
Comment 24 James Robinson 2013-03-28 12:58:57 PDT
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?
Comment 25 Tien-Ren Chen 2013-03-28 13:31:48 PDT
(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.
Comment 26 Tien-Ren Chen 2013-03-28 13:48:49 PDT
Created attachment 195635 [details]
Patch
Comment 27 James Robinson 2013-03-28 13:54:57 PDT
Comment on attachment 195635 [details]
Patch

R=me
Comment 28 WebKit Review Bot 2013-03-28 14:48:55 PDT
Comment on attachment 195635 [details]
Patch

Clearing flags on attachment: 195635

Committed r147163: <http://trac.webkit.org/changeset/147163>
Comment 29 WebKit Review Bot 2013-03-28 14:49:01 PDT
All reviewed patches have been landed.  Closing bug.