Bug 70609

For reference please see the file Source/WebCore/dom/ViewportArguments.cpp: http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/ViewportArguments.cpp&exact_package=chromium&q=ViewportArguments.cpp&type=cs We would like to delete these lines or at least put them behind a (default) parameter: // Update minimum scale factor, to never allow zooming out more than viewport result.minimumScale = max<float>(result.minimumScale, max(availableWidth / width, availableHeight / height));

Created attachment 112373 [details] Patch

So basically what you want is the support for fit

(In reply to comment #3) > So basically what you want is the support for fit Yup. We want to let the embedder decide on the minimum scale factor.

So this is tricky area. The content is larger than the viewport and you cannot zoom fully out. That might very well be how the web page was designed so as a general rule we try to not work around bad content. What is really important here is whether the scale restriction was explicitly set or implicitly calculated. Zalan (which I just cc'ed) has some patches for dealing with that. Zalan, maybe you can chime in?

Comment on attachment 112373 [details] Patch Attachment 112373 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10219088

(In reply to comment #5) > So this is tricky area. The content is larger than the viewport and you cannot zoom fully out. > > That might very well be how the web page was designed so as a general rule we try to not work around bad content. > > What is really important here is whether the scale restriction was explicitly set or implicitly calculated. > > Zalan (which I just cc'ed) has some patches for dealing with that. Zalan, maybe you can chime in? It depends on what you consider the purpose of overview mode to be? If I'm in overview mode I'd like to be able to look at the width of the page at a glance, without scrolling. The change I'm proposing leaves this decision in the hands of the embedder. Existing ports that make use of this function will not need to make any changes, because, by default, nothing has changed.

(In reply to comment #7) > (In reply to comment #5) > > So this is tricky area. The content is larger than the viewport and you cannot zoom fully out. > > > > That might very well be how the web page was designed so as a general rule we try to not work around bad content. > > > > What is really important here is whether the scale restriction was explicitly set or implicitly calculated. > > > > Zalan (which I just cc'ed) has some patches for dealing with that. Zalan, maybe you can chime in? > > It depends on what you consider the purpose of overview mode to be? If I'm in overview mode I'd like to be able to look at the width of the page at a glance, without scrolling. The change I'm proposing leaves this decision in the hands of the embedder. Existing ports that make use of this function will not need to make any changes, because, by default, nothing has changed. You don't need this change for that. Your embedder can just ignore the proposed scale values and scale such that the full width is visible. You might have to update your layout height then, by multiplying the width with the ratio you want for your overview thumbnails.

we, with n9 browser, do pretty much the same by ignoring implicit minimum values for certain cases. The logic is at the app level and because we decided not to force such parameters through the viewport API.

(In reply to comment #9) > we, with n9 browser, do pretty much the same by ignoring implicit minimum values for certain cases. The logic is at the app level and because we decided not to force such parameters through the viewport API. We are in agreement! It seems like the best solution is to get this code into WebCore if multiple ports are interested in the same behavior.

(In reply to comment #10) > (In reply to comment #9) > > we, with n9 browser, do pretty much the same by ignoring implicit minimum values for certain cases. The logic is at the app level and because we decided not to force such parameters through the viewport API. > > We are in agreement! It seems like the best solution is to get this code into WebCore if multiple ports are interested in the same behavior. I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm.

Comment on attachment 112373 [details] Patch Let's not open for a can of worms. The code in WebCore is for the basic algorithm and should stay as such. If different behavior is wished for, it can easily be done on top.

(In reply to comment #12) > (From update of attachment 112373 [details]) > Let's not open for a can of worms. The code in WebCore is for the basic algorithm and should stay as such. If different behavior is wished for, it can easily be done on top. Agree. That's why we decided to do it at the app level. We also have some specific adjustment for initial and maximum values. Pushing all of them through the API would make it end up looking really broken.

(In reply to comment #11) > I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm. We all agree that WebCore should just be a simple implementation of the spec. However if you compare the spec (http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values) to the algorithm implemented in computeViewportAttributes, you will see that they roughly match up, until the end of "Extend width and height to fill the window/viewing area for the specified zoom". Then computeViewportAttributes does two things which are _not_ in the spec: 1. It modifies the minScale to avoid "zooming out more than viewport". 2. It overwrites min/maxScale if userScalable is false. Both of these additions are counter to the spec. While the 2nd one is slighty less of a problem, the 1st is an implementation-specific heuristic, making the -- often wrong -- assumption that embedders will never want to zoom out more than the viewport. And if the embedder does want to zoom out more than the viewport (as at least we and N9 do), it is no longer possible! Since that line has overwritten the true value of minScale, the embedder is unable to distinguish whether the webpage had explictly forbidden zooming out more than the viewport (in which case that should be respected) or if minScale was small or auto (in which case we can zoom out to show the whole page). We should remove this line from computeViewportAttributes, and instead any embedders which chose to have this implementation-specific behavior should do it themselves.

(In reply to comment #14) > (In reply to comment #11) > > I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm. > > We all agree that WebCore should just be a simple implementation of the spec. However if you compare the spec (http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values) to the algorithm implemented in computeViewportAttributes, you will see that they roughly match up, until the end of "Extend width and height to fill the window/viewing area for the specified zoom". Then computeViewportAttributes does two things which are _not_ in the spec: > > 1. It modifies the minScale to avoid "zooming out more than viewport". > 2. It overwrites min/maxScale if userScalable is false. > > Both of these additions are counter to the spec. While the 2nd one is slighty less of a problem, the 1st is an implementation-specific heuristic, making the -- often wrong -- assumption that embedders will never want to zoom out more than the viewport. > > And if the embedder does want to zoom out more than the viewport (as at least we and N9 do), it is no longer possible! Since that line has overwritten the true value of minScale, the embedder is unable to distinguish whether the webpage had explictly forbidden zooming out more than the viewport (in which case that should be respected) or if minScale was small or auto (in which case we can zoom out to show the whole page). > > We should remove this line from computeViewportAttributes, and instead any embedders which chose to have this implementation-specific behavior should do it themselves. I see. I guess we should just remove the line then and add this to the client. That is fine with me.

Created attachment 113176 [details] Patch

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #11) > > > I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm. > > > > We all agree that WebCore should just be a simple implementation of the spec. However if you compare the spec (http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values) to the algorithm implemented in computeViewportAttributes, you will see that they roughly match up, until the end of "Extend width and height to fill the window/viewing area for the specified zoom". Then computeViewportAttributes does two things which are _not_ in the spec: > > > > 1. It modifies the minScale to avoid "zooming out more than viewport". > > 2. It overwrites min/maxScale if userScalable is false. > > > > Both of these additions are counter to the spec. While the 2nd one is slighty less of a problem, the 1st is an implementation-specific heuristic, making the -- often wrong -- assumption that embedders will never want to zoom out more than the viewport. > > > > And if the embedder does want to zoom out more than the viewport (as at least we and N9 do), it is no longer possible! Since that line has overwritten the true value of minScale, the embedder is unable to distinguish whether the webpage had explictly forbidden zooming out more than the viewport (in which case that should be respected) or if minScale was small or auto (in which case we can zoom out to show the whole page). > > > > We should remove this line from computeViewportAttributes, and instead any embedders which chose to have this implementation-specific behavior should do it themselves. > > I see. I guess we should just remove the line then and add this to the client. That is fine with me. I've uploaded a new version of the patch, deleting that line from computeViewportAttributes and I've updated all the clients that use it. It'll probably take me a couple of attempts to get all the bots to turn green.

You're only updating minimum scale, but what if the viewport arguments specified an initial scale that is smaller than your clamped minimum scale? ie: initial-scale:0.9 and your minimum-scale is restricted to 1.0 by the visibleViewport?

(In reply to comment #18) > You're only updating minimum scale, but what if the viewport arguments specified an initial scale that is smaller than your clamped minimum scale? > ie: initial-scale:0.9 and your minimum-scale is restricted to 1.0 by the visibleViewport? Good catch! This would happen if !args.userScalable in addition to the above. Maybe I should move out these lines out to a different method too: if (!args.userScalable) result.maximumScale = result.minimumScale = result.initialScale;

Comment on attachment 113176 [details] Patch Attachment 113176 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10244977

Comment on attachment 113176 [details] Patch Attachment 113176 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10249315

(In reply to comment #19) > (In reply to comment #18) > > You're only updating minimum scale, but what if the viewport arguments specified an initial scale that is smaller than your clamped minimum scale? > > ie: initial-scale:0.9 and your minimum-scale is restricted to 1.0 by the visibleViewport? > > > > Good catch! This would happen if !args.userScalable in addition to the above. Maybe I should move out these lines out to a different method too: > > > if (!args.userScalable) > result.maximumScale = result.minimumScale = result.initialScale; Adding method: restrictScaleFactorToInitialScaleIfNotUserScalable

Created attachment 113186 [details] Patch

Comment on attachment 113186 [details] Patch Attachment 113186 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10259052

Comment on attachment 113186 [details] Patch Attachment 113186 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10256096

Comment on attachment 113186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113186&action=review Look pretty good. Zalan what do you think? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2616 > ViewportAttributes attrs = WebCore::computeViewportAttributes(arguments, /* default layout width for non-mobile pages */ 980, deviceWidth, deviceHeight, deviceDPI, IntSize(availableWidth, availableHeight)); > + WebCore::restrictMinimumScaleFactorToViewportSize(attrs, IntSize(availableWidth, availableHeight)); > + WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable(attrs); > return String::format("viewport size %dx%d scale %f with limits [%f, %f] and userScalable %f\n", attrs.layoutSize.width(), attrs.layoutSize.height(), attrs.initialScale, attrs.minimumScale, attrs.maximumScale, attrs.userScalable); I think it might be better to update the test results. The layout tests should test the implementation of the spec and the compliance.

(In reply to comment #26) > (From update of attachment 113186 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113186&action=review > > Look pretty good. Zalan what do you think? Looks pretty good to me too.

Comment on attachment 113186 [details] Patch Please make it work on mac become committing

Created attachment 113333 [details] Patch

(In reply to comment #28) > (From update of attachment 113186 [details]) > Please make it work on mac become committing Waiting on mac bot to turn green before committing.

Created attachment 113487 [details] Patch for landing

(In reply to comment #31) > Created an attachment (id=113487) [details] > Patch for landing Oops, accidentally lost the ChangeLog, I just removed the c+ flag. Fixing this then uploading a new patch and landing it.

Created attachment 113494 [details] Patch

Created attachment 113495 [details] Patch

Comment on attachment 113495 [details] Patch Clearing flags on attachment: 113495 Committed r99195: <http://trac.webkit.org/changeset/99195>

All reviewed patches have been landed. Closing bug.

(In reply to comment #36) > All reviewed patches have been landed. Closing bug. It broke the Qt-WK2 build: /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp: In member function ‘void QTouchWebViewPrivate::updateViewportConstraints()’: /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:95: error: ‘restrictMinimumScaleFactorToViewport’ is not a member of ‘WebCore’

(In reply to comment #37) > (In reply to comment #36) > > All reviewed patches have been landed. Closing bug. > > It broke the Qt-WK2 build: > > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp: In member function ‘void QTouchWebViewPrivate::updateViewportConstraints()’: > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:95: error: ‘restrictMinimumScaleFactorToViewport’ is not a member of ‘WebCore’ Ohh, that's a typo That should say restrictMinimumScaleFactorToViewportSize. I'm surprised the QT bot was green.

(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > All reviewed patches have been landed. Closing bug. > > > > It broke the Qt-WK2 build: > > > > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp: In member function ‘void QTouchWebViewPrivate::updateViewportConstraints()’: > > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:95: error: ‘restrictMinimumScaleFactorToViewport’ is not a member of ‘WebCore’ > Ohh, that's a typo That should say restrictMinimumScaleFactorToViewportSize. I'm surprised the QT bot was green. Because Qt bot on build.webkit.org doesn't build WK2.

and unfortunately it broke 21 tests: http://build.webkit.org/results/Qt%20Linux%20Release/r99196%20%2839289%29/results.html

Qt-WK2 buildfix landed in http://trac.webkit.org/changeset/99199

I added failing tests to the Skipped list until fix to make buildbot green to be able catch new regressions. Please unskip them with a fix. http://trac.webkit.org/changeset/99213/trunk/LayoutTests/platform/qt/Skipped

(In reply to comment #42) > I added failing tests to the Skipped list until fix to make buildbot green to be able catch new regressions. Please unskip them with a fix. > > http://trac.webkit.org/changeset/99213/trunk/LayoutTests/platform/qt/Skipped I looked at the difference in output briefly, and it seems very very small. It's possible this is a small rounding issue but I haven't verified. I will be doing Chromium-side testing of all this shortly.

These tests fail on GTK too.

(In reply to comment #44) > These tests fail on GTK too. Skipped in GTK too: http://trac.webkit.org/changeset/99269 Please unskip when fixed!

This regression is still valid. Are you guys going to fix it?

(In reply to comment #46) > This regression is still valid. Are you guys going to fix it? I will look into this.

(In reply to comment #47) > (In reply to comment #46) > > This regression is still valid. Are you guys going to fix it? > > I will look into this. Any news on this issue?

It is a 4.5 month old regression ... It isn't a fairplay to leave it unresolved.

Created attachment 138600 [details] Patch

(In reply to comment #50) > Created an attachment (id=138600) [details] > Patch (In reply to comment #49) > It is a 4.5 month old regression ... It isn't a fairplay to leave it unresolved. Sorry, I was a little inactive on WebKit the last few months.

Comment on attachment 138600 [details] Patch Attachment 138600 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12522279

Comment on attachment 138600 [details] Patch Attachment 138600 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12525309

Comment on attachment 138600 [details] Patch Attachment 138600 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12522301

Created attachment 139817 [details] Patch

Created attachment 139824 [details] Patch

Comment on attachment 139824 [details] Patch Attachment 139824 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12593894

Comment on attachment 139824 [details] Patch Attachment 139824 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12593897

Created attachment 139829 [details] Patch

Comment on attachment 139829 [details] Patch Attachment 139829 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12595843

Comment on attachment 139829 [details] Patch Attachment 139829 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12598731

Created attachment 139835 [details] Patch

Comment on attachment 139835 [details] Patch Attachment 139835 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12604017

Comment on attachment 139835 [details] Patch Attachment 139835 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12603016

Created attachment 139842 [details] Patch

Comment on attachment 139842 [details] Patch Attachment 139842 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12609006

Comment on attachment 139842 [details] Patch Attachment 139842 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12487038

Created attachment 139855 [details] Patch

Comment on attachment 139855 [details] Patch Attachment 139855 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12611028

Comment on attachment 139855 [details] Patch Attachment 139855 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12600042

Created attachment 139873 [details] Patch

Comment on attachment 139873 [details] Patch Clearing flags on attachment: 139873 Committed r115907: <http://trac.webkit.org/changeset/115907>

All reviewed patches have been landed. Closing bug.

(In reply to comment #73) > All reviewed patches have been landed. Closing bug. If anyone still sees a problem here, please yell at me :-)

Reopen, because it broke all viewport tests on Qt and on GTK. Qt results seems to be correct, but they needs to be updated: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r115937%20%2836886%29/results.html --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-actual.txt @@ -1,2 +1,2 @@ -viewport size 320x352 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000 +viewport size 320.000000x352.000000 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000 But GTK results are absolutely false: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r115937%20%2823177%29/results.html --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-actual.txt @@ -1,2 +1,2 @@ -viewport size 320x352 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000 +viewport size -1082130432x480 scale 320.000000 with limits [352.000000, 1.000000] and userScalable 1.000000

Rollout landed in http://trac.webkit.org/changeset/115939 Please update the expected files too with the proper fix.

Created attachment 140019 [details] Patch

Created attachment 140021 [details] Patch

(In reply to comment #78) > Created an attachment (id=140021) [details] > Patch With this change one test result should be updated (at least on Qt): --- /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/viewport/viewport-86-expected.txt +++ /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/viewport/viewport-86-actual.txt @@ -1,2 +1,2 @@ -viewport size 457x503 scale 0.700000 with limits [0.700000, 5.000000] and userScalable -1.000000 +viewport size 457x502 scale 0.700000 with limits [0.700000, 5.000000] and userScalable -1.000000 Additionally please remove the tests from qt/Skipped lists you fixed with this patch: -# REGRESSION(r99195) -# https://bugs.webkit.org/show_bug.cgi?id=70609 -fast/viewport/viewport-83.html ... -fast/viewport/viewport-warnings-3.html -

Created attachment 140043 [details] Patch

Please unskip these tests on GTK+ as well. :)

Created attachment 140047 [details] Patch

(In reply to comment #81) > Please unskip these tests on GTK+ as well. :) Done. Landing...

Comment on attachment 140047 [details] Patch Clearing flags on attachment: 140047 Committed r115998: <http://trac.webkit.org/changeset/115998>

All reviewed patches have been landed. Closing bug.