Bug 132978

Summary: [iOS WK2] When zoomed, fixed elements jump at the start of a scroll, and jump back at the end.
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cmarcelo, commit-queue, jamesr, luiz, simon.fraser, tonikitoo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch benjamin: review+

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]
Patch
Comment 2 Simon Fraser (smfr) 2014-05-15 18:11:17 PDT
<rdar://problem/16894428>
Comment 3 Benjamin Poulain 2014-05-15 18:38:57 PDT
Comment on attachment 231545 [details]
Patch

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();

ditto

> 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]
Patch
Comment 5 Benjamin Poulain 2014-05-15 20:07:24 PDT
Comment on attachment 231550 [details]
Patch

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

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

float->double
minimumZoomScale->displayedContentMinimumZoomScale().

> 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
http://trac.webkit.org/changeset/168920