Bug 103521

Summary: Possible to resize out of bounds
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: WebKit2Assignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cmarcelo, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103875    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch jturcotte: review+

Allan Sandfeld Jensen
Reported 2012-11-28 06:32:58 PST
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.
Attachments
Patch (2.35 KB, patch)
2012-11-28 06:35 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (8.40 KB, patch)
2012-11-28 09:06 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (9.35 KB, patch)
2012-11-28 09:07 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (9.90 KB, patch)
2012-11-29 06:17 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (9.98 KB, patch)
2012-11-29 06:37 PST, Allan Sandfeld Jensen 		jturcotte: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1 2012-11-28 06:35:11 PST
Created attachment 176476 [details] Patch
Kenneth Rohde Christiansen
Comment 2 2012-11-28 06:36:38 PST
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
Comment 3 2012-11-28 09:06:47 PST
Created attachment 176494 [details] Patch
Allan Sandfeld Jensen
Comment 4 2012-11-28 09:07:30 PST
Created attachment 176495 [details] Patch Forgot to update the commit before uploading.
Andras Becsi
Comment 5 2012-11-28 09:11:05 PST
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 6 2012-11-28 09:14:02 PST
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
Comment 7 2012-11-28 09:16:57 PST
(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
Comment 8 2012-11-28 09:19:53 PST
(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 9 2012-11-28 09:31:04 PST
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 10 2012-11-29 02:43:08 PST
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
Comment 11 2012-11-29 06:17:14 PST
Created attachment 176714 [details] Patch Only enforce fitness on viewport resize, not on content size change
Jocelyn Turcotte
Comment 12 2012-11-29 06:27:22 PST
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
Comment 13 2012-11-29 06:37:10 PST
Created attachment 176716 [details] Patch Minor style changes
Jocelyn Turcotte
Comment 14 2012-11-29 06:48:57 PST
Comment on attachment 176716 [details] Patch Thanks, this is easier to read.
Allan Sandfeld Jensen
Comment 15 2012-11-29 07:01:57 PST
Committed r136129: <http://trac.webkit.org/changeset/136129>
Note You need to log in before you can comment on or make changes to this bug.