Possible to resize out of bounds

[Allan Sandfeld Jensen]

If you double-tap to zoom, and double-tap to zoom out, it is possible to resize the viewport while breaking the viewport zoom/resize bounds. 

While not problem for a full-screen application it could cause trouble for a QML application that resize the viewport to make room for keyboard or notifications. As soon as the user has performed a manual zoom, all the resize logic would act differently, even if the user has zoomed back to the initial state.

[Allan Sandfeld Jensen]

Created attachment 176476 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 176476 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:285
> +    bool wasMinimumScale = (m_effectiveScale == toViewportScale(m_minimumScaleToFit));

wasAt ?

> Source/WebKit2/UIProcess/PageViewportController.cpp:303
> -            applyScaleAfterRenderingContents(toViewportScale(minimumScale));
> +        if (!hasSuspendedContent()) {
> +            if (wasMinimumScale)
> +                applyScaleAfterRenderingContents(toViewportScale(m_minimumScaleToFit));
> +            else {
> +                float boundedScale = innerBoundedViewportScale(m_effectiveScale);
> +                if (boundedScale != m_effectiveScale)
> +                    applyScaleAfterRenderingContents(boundedScale);
> +            }
> +        }

Any QML test for this?

[Allan Sandfeld Jensen]

Created attachment 176494 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 176495 [details]
Patch

Forgot to update the commit before uploading.

[Andras Becsi]

Comment on attachment 176495 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:286
> +    bool currentlyScaledToFit = (m_effectiveScale == toViewportScale(m_minimumScaleToFit));
> +

It might be better to use fuzzyCompare for these float values.

> Source/WebKit2/UIProcess/PageViewportController.cpp:302
> +                if (boundedScale != m_effectiveScale)

fuzzyCompare here as well?

[Andras Becsi]

Comment on attachment 176495 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:-94
> -
> -            // We had user interaction, size should change but not scale.
> -            setDisplay("target", "block")
> -            compare(documentSize(), "960x1440")
> -            compare(test.contentsScale, 1.0)

Why remove this?

[Allan Sandfeld Jensen]

(In reply to comment #6)
> (From update of attachment 176495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176495&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:-94
> > -
> > -            // We had user interaction, size should change but not scale.
> > -            setDisplay("target", "block")
> > -            compare(documentSize(), "960x1440")
> > -            compare(test.contentsScale, 1.0)
> 
> Why remove this?

Because it is no longer true. The since the viewport is scaled to fit, it will still resize to stay fit.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (From update of attachment 176495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176495&action=review
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:286
> > +    bool currentlyScaledToFit = (m_effectiveScale == toViewportScale(m_minimumScaleToFit));
> > +
> 
> It might be better to use fuzzyCompare for these float values.
> 

Here it doesn't matter because the values are not calculated. It is testing if the effectiveScale has been specifically set to the this value.

> > Source/WebKit2/UIProcess/PageViewportController.cpp:302
> > +                if (boundedScale != m_effectiveScale)
> 
> fuzzyCompare here as well?

I guess that could make sense, but again. It is more about testing if the effective scale has been set to this value, not if the values are close.

[Andras Becsi]

Comment on attachment 176495 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:-94
>>> -            compare(test.contentsScale, 1.0)
>> 
>> Why remove this?
> 
> Because it is no longer true. The since the viewport is scaled to fit, it will still resize to stay fit.

Hmm, but we shouldn't change the scale if there was user interaction and the viewport size did not change, we should leave the scale to the value set by the user, no?

[Andras Becsi]

Comment on attachment 176495 [details]
Patch

I'm sorry, but this patch breaks expected behavior.
The above mentioned test should not be removed since it is still valid.
After a user interaction the scale should only be reset if the viewport size changes _and_ the current scale would result in the page being out of bounds eg. smaller than the allowed minimum-scale-to-fit.

[Allan Sandfeld Jensen]

Created attachment 176714 [details]
Patch

Only enforce fitness on viewport resize, not on content size change

[Jocelyn Turcotte]

Comment on attachment 176714 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.cpp:296
> -        if (!m_hadUserInteraction && !hasSuspendedContent())
> -            applyScaleAfterRenderingContents(toViewportScale(minimumScale));
> +        if (!hasSuspendedContent()) {
> +            if (scaleToFit)

For the readability sake, could you try to put (!m_hadUserInteraction || userInitiatedUpdate) in the same statement?
I also liked the context that "currentlyFitToViewport" added in the previous patch, if that name can be preserved.

[Allan Sandfeld Jensen]

Created attachment 176716 [details]
Patch

Minor style changes

[Jocelyn Turcotte]

Comment on attachment 176716 [details]
Patch

Thanks, this is easier to read.

[Allan Sandfeld Jensen]

Committed r136129: <http://trac.webkit.org/changeset/136129>