Bug 132978 - [iOS WK2] When zoomed, fixed elements jump at the start of a scroll, and jump back at the end.
Summary: [iOS WK2] When zoomed, fixed elements jump at the start of a scroll, and jump...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
Keywords: InRadar
Depends on:
Reported: 2014-05-15 18:05 PDT by Simon Fraser (smfr)
Modified: 2014-05-15 20:57 PDT (History)
7 users (show)

See Also:

Patch (16.63 KB, patch)
2014-05-15 18:10 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2014-05-15 19:09 PDT, Simon Fraser (smfr)
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-05-15 18:05:32 PDT
[iOS WK2] When zoomed, fixed elements jump at the start of a scroll, and jump back at the end.
Comment 1 Simon Fraser (smfr) 2014-05-15 18:10:43 PDT
Created attachment 231545 [details]
Comment 2 Simon Fraser (smfr) 2014-05-15 18:11:17 PDT
Comment 3 Benjamin Poulain 2014-05-15 18:38:57 PDT
Comment on attachment 231545 [details]

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

Looks good to me but we'll have to look into the regression.

> Source/WebKit2/UIProcess/PageClient.h:260
> +    virtual float minimumZoomScale() const = 0;

Maybe displayedContentMinimumZoomScale()?

It would be nice to differentiate that scale from the real zoom scale we have in the content.

displayedContentMinimumZoomScale() would make it more obvious when comparing that value to displayedContentScale() in WebPageProxy.

> Source/WebKit2/UIProcess/PageClient.h:261
> +    virtual WebCore::FloatSize contentsSize() const = 0;

That make sense for our current architecture...but it is sooo weird the page proxy has to ask the client for the content size.

Maybe we should store a copy of the contentSize on WebPageProxy and update it on WebPageProxy::didCommitLayerTree()?

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:190
> +float PageClientImpl::minimumZoomScale() const

I would make this a double.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:223
> +    float scale = displayedContentScale();

This should be a double.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:224
> +    float minimumScale = m_pageClient.minimumZoomScale();


> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:228
> +        const CGFloat slope = 12;
> +        CGFloat factor = std::max<CGFloat>(1 - slope * (minimumScale - scale), 0);

Let's use double here instead of CGFloat now that this code is in WebPageProxy.
Comment 4 Simon Fraser (smfr) 2014-05-15 19:09:32 PDT
Created attachment 231550 [details]
Comment 5 Benjamin Poulain 2014-05-15 20:07:24 PDT
Comment on attachment 231550 [details]

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

> Source/WebKit2/UIProcess/PageClient.h:260
> +    virtual float minimumZoomScale() const = 0;


> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:192
> +    if (UIScrollView *scroller = [m_contentView _scroller])

I am trying to get rid of all the _scroller.

You can use [m_webView scrollView] here instead.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:214
> +WebCore::FloatRect WebPageProxy::computeCustomFixedPositionRect(const FloatRect& unobscuredContentRect, float scale, UnobscuredRectConstraint constraint) const

float -> double.
scale -> displayedContentScale for clarity.
Comment 6 Simon Fraser (smfr) 2014-05-15 20:57:23 PDT