Bug 174362

Summary: window.innerWidth/height shouldn't change under pinch zoom
Product: WebKit Reporter: Rick Byers <rbyers>
Component: Layout and RenderingAssignee: Ali Juma <ajuma>
Status: ASSIGNED    
Severity: Normal CC: 7raivis, ahmad.saleem792, bfulgham, bramus, buildbot, darin, dbates, decademoon.bugzilla, fred.wang, hyatt, mjs, muodov, rbuis, rniwa, simon.fraser, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: BrowserCompat, InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: iOS 11   
Bug Depends on: 175235    
Bug Blocks: 170981    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Patch
none
Combined patch (175235+174362) for EWS
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Patch mjs: review-

Rick Byers
Reported 2017-07-11 07:50:29 PDT
A specific example of bug 170981. Today window.innerWidth/height describe the visual viewport when (under the "inert" model) they should represent be the layout viewport. Repro: https://rbyers.github.io/sb/innerWidth.html The Google Search team has reported this to us as an interop problem that has caused them pain in practice (when trying to position things correctly). I tried on iOS 11 beta (15A5304i)
Attachments
Patch (8.39 KB, patch)
2017-07-27 07:28 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch (9.05 KB, patch)
2017-08-08 13:28 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews103 for mac-elcapitan (1.10 MB, application/zip)
2017-08-08 14:42 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.15 MB, application/zip)
2017-08-08 14:45 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.88 MB, application/zip)
2017-08-08 15:00 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (7.48 MB, application/zip)
2017-08-08 17:18 PDT, Build Bot 		no flags
Details
Patch (12.39 KB, patch)
2017-08-18 08:13 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.20 MB, application/zip)
2017-08-18 09:08 PDT, Build Bot 		no flags
Details
Patch (12.49 KB, patch)
2017-10-20 15:52 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Combined patch (175235+174362) for EWS (16.41 KB, patch)
2017-10-20 16:11 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.16 MB, application/zip)
2017-10-21 02:12 PDT, Build Bot 		no flags
Details
Patch (12.76 KB, patch)
2017-11-27 12:46 PST, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch (12.75 KB, patch)
2017-12-05 12:51 PST, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch (16.91 KB, patch)
2017-12-06 11:12 PST, Ali Juma 		mjs: review-
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Rick Byers
Comment 1 2017-07-11 07:56:43 PDT
Also verified on MacOS with WebKit Nightly r219324 with touchpad pinch-zoom. Apparently we have the same problem on Chrome Mac at the moment: https://bugs.chromium.org/p/chromium/issues/detail?id=740956. Our primary case (touchscreen pinch-zoom) does correctly leave innerWidth as the layout viewport.
Radar WebKit Bug Importer
Comment 2 2017-07-11 10:27:59 PDT
<rdar://problem/33239908>
Ali Juma
Comment 3 2017-07-27 07:28:42 PDT
Created attachment 316546 [details] Patch
Simon Fraser (smfr)
Comment 4 2017-08-02 12:00:02 PDT
Comment on attachment 316546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316546&action=review > Source/WebCore/page/DOMWindow.cpp:1256 > - return view->mapFromLayoutToCSSUnits(static_cast<int>(view->unobscuredContentRectIncludingScrollbars().height())); > + return static_cast<int>(view->unobscuredContentRectIncludingScrollbars().height() * view->layoutViewportToClientScaleFactor()); Why isn't this just the height of FrameView::layoutViewportRect() ?
Ali Juma
Comment 5 2017-08-04 18:40:09 PDT
In trying to directly use FrameView::layoutViewportRect to compute innerWidth/innerHeight, I've run into another bug where the layout viewport has the wrong size after window resize, since ScrollView::updateScrollbars reaches its cMaxUpdateScrollbarsPass recursion limit. I'll fix that separately and then come back to this.
Ali Juma
Comment 6 2017-08-08 13:28:53 PDT
Created attachment 317605 [details] Patch This uses the layout viewport rect's height/width to compute innerHeight/innerWidth. But there are tests that fail because of bug 175235, when the layout viewport rect has the wrong size.
Build Bot
Comment 7 2017-08-08 14:42:14 PDT Comment hidden (obsolete)
Build Bot
Comment 8 2017-08-08 14:42:15 PDT Comment hidden (obsolete)
Build Bot
Comment 9 2017-08-08 14:45:47 PDT Comment hidden (obsolete)
Build Bot
Comment 10 2017-08-08 14:45:48 PDT Comment hidden (obsolete)
Build Bot
Comment 11 2017-08-08 15:00:51 PDT Comment hidden (obsolete)
Build Bot
Comment 12 2017-08-08 15:00:53 PDT Comment hidden (obsolete)
Build Bot
Comment 13 2017-08-08 17:18:40 PDT Comment hidden (obsolete)
Build Bot
Comment 14 2017-08-08 17:18:42 PDT Comment hidden (obsolete)
Ali Juma
Comment 15 2017-08-18 08:13:50 PDT
Created attachment 318498 [details] Patch Updated patch to account for the size of platform scrollbars (used by WK1). This still depends on bug 175235.
Build Bot
Comment 16 2017-08-18 09:08:48 PDT
Comment on attachment 318498 [details] Patch Attachment 318498 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4337380 New failing tests: css3/viewport-percentage-lengths/viewport-percentage-lengths-percent-size-child.html css3/viewport-percentage-lengths/viewport-percentage-lengths-anonymous-block.html
Build Bot
Comment 17 2017-08-18 09:08:50 PDT
Created attachment 318502 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Frédéric Wang (:fredw)
Comment 18 2017-10-18 07:57:18 PDT
Comment on attachment 318498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318498&action=review > Source/WebCore/page/DOMWindow.cpp:1267 > + float widthIncludingScrollbar = view->baseLayoutViewportSize().width().toFloat() + view->horizontalScrollbarIntrusion() + view->platformWidgetHorizontalScrollbarIntrusion(); Are you directly using baseLayoutViewportSize on purpose, or did you mean layoutViewportRect().width()? > Source/WebCore/page/FrameView.h:486 > + float clientToLayoutViewportScaleFactor() const; It looks like this function can be private, as it is not used outside the class.
Ali Juma
Comment 19 2017-10-20 15:52:54 PDT
Created attachment 324456 [details] Patch
Ali Juma
Comment 20 2017-10-20 15:54:39 PDT
Comment on attachment 318498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318498&action=review >> Source/WebCore/page/DOMWindow.cpp:1267 >> + float widthIncludingScrollbar = view->baseLayoutViewportSize().width().toFloat() + view->horizontalScrollbarIntrusion() + view->platformWidgetHorizontalScrollbarIntrusion(); > > Are you directly using baseLayoutViewportSize on purpose, or did you mean layoutViewportRect().width()? Thanks, good catch, I meant to use layoutViewportRect() here. >> Source/WebCore/page/FrameView.h:486 >> + float clientToLayoutViewportScaleFactor() const; > > It looks like this function can be private, as it is not used outside the class. Since layoutViewportToClientScaleFactor is used outside and needs to be public, I left this public too for the sake of symmetry and to keep all the space conversion function declarations together.
Ali Juma
Comment 21 2017-10-20 16:11:37 PDT
Created attachment 324459 [details] Combined patch (175235+174362) for EWS
Build Bot
Comment 22 2017-10-21 02:12:35 PDT
Comment on attachment 324456 [details] Patch Attachment 324456 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4942423 New failing tests: css3/viewport-percentage-lengths/viewport-percentage-lengths-percent-size-child.html css3/viewport-percentage-lengths/viewport-percentage-lengths-anonymous-block.html
Build Bot
Comment 23 2017-10-21 02:12:36 PDT
Created attachment 324499 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Darin Adler
Comment 24 2017-11-22 12:39:10 PST
Simon, Hyatt, are you going to review this?
Frédéric Wang (:fredw)
Comment 25 2017-11-24 03:15:42 PST
Comment on attachment 324456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324456&action=review @Ali: Can you rebase your patch? It seems that the last one had conflicts in Windows... > Source/WebCore/platform/ScrollView.h:183 > + // The space occupied by a platform widget's scrollbars. I think it WebKit style is to use sentences in comments like "This is the space occupied...". > LayoutTests/ChangeLog:10 > + * fast/dom/window-inner-size-scaling.html: I think it would be nice to add short explanations in the ChangeLog. For example that window-inner-size-scaling is modified to test that innerWidth/Height are independent of zoom scale. I'm not sure I understand the rationale for the changes in iframe-inner-size-scaling.html... shoudn't it be unaffected by this patch if visualViewportEnabled is disabled in the test?
Ali Juma
Comment 26 2017-11-27 12:46:32 PST
Created attachment 327661 [details] Patch
Ali Juma
Comment 27 2017-11-27 12:49:12 PST
Comment on attachment 324456 [details] Patch It might be more web-compatible to put the changes to innerWidth/innerHeight behind the visualViewportAPI flag rather than just the visualViewport flag, since that make it easier to detect the behavior change (by checking for the presence of window.visualViewport). Thoughts? View in context: https://bugs.webkit.org/attachment.cgi?id=324456&action=review >> Source/WebCore/platform/ScrollView.h:183 >> + // The space occupied by a platform widget's scrollbars. > > I think it WebKit style is to use sentences in comments like "This is the space occupied...". Thanks, done. >> LayoutTests/ChangeLog:10 >> + * fast/dom/window-inner-size-scaling.html: > > I think it would be nice to add short explanations in the ChangeLog. For example that window-inner-size-scaling is modified to test that innerWidth/Height are independent of zoom scale. I'm not sure I understand the rationale for the changes in iframe-inner-size-scaling.html... shoudn't it be unaffected by this patch if visualViewportEnabled is disabled in the test? Done. The issue in iframe-inner-size-scaling.html is that the size of the iframe depends on the size of <body>, and previously this changed when scrollbars appeared (as a result of zooming in). The test was passing before only because the call to innerWIdth didn't force a layout update. This became a problem now that innerWidth/innerHeight do force a layout update. visualViewportEnabled is true by default on many platforms (mac-WK1 and all of WK2).
Simon Fraser (smfr)
Comment 28 2017-11-30 16:31:29 PST
Comment on attachment 327661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327661&action=review > Source/WebCore/page/DOMWindow.cpp:1269 > + float heightIncludingScrollbar = view->layoutViewportRect().height() + view->verticalScrollbarIntrusion() + view->platformWidgetVerticalScrollbarIntrusion(); It's really weird to add both verticalScrollbarIntrusion and platformWidgetVerticalScrollbarIntrusion.
Ali Juma
Comment 29 2017-12-05 12:51:33 PST
Created attachment 328488 [details] Patch
Ali Juma
Comment 30 2017-12-05 12:52:05 PST
Comment on attachment 327661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327661&action=review >> Source/WebCore/page/DOMWindow.cpp:1269 >> + float heightIncludingScrollbar = view->layoutViewportRect().height() + view->verticalScrollbarIntrusion() + view->platformWidgetVerticalScrollbarIntrusion(); > > It's really weird to add both verticalScrollbarIntrusion and platformWidgetVerticalScrollbarIntrusion. Changed to only add one or the other, depending on whether there's a platform widget.
Simon Fraser (smfr)
Comment 31 2017-12-05 13:15:40 PST
Comment on attachment 328488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328488&action=review > Source/WebCore/page/DOMWindow.cpp:1270 > + float scrollbarHeight = view->platformWidget() ? view->platformWidgetVerticalScrollbarIntrusion() : view->verticalScrollbarIntrusion(); > + return static_cast<int>((view->layoutViewportRect().height() + scrollbarHeight) * view->layoutViewportToClientScaleFactor()); I'm still a bit surprised that we need code here to deal with platform vs. non-platform scrollbars. What does scrollbar->occupiedWidth() return if there are platform widget scrollbars?
Ali Juma
Comment 32 2017-12-05 13:36:07 PST
Comment on attachment 328488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328488&action=review >> Source/WebCore/page/DOMWindow.cpp:1270 >> + return static_cast<int>((view->layoutViewportRect().height() + scrollbarHeight) * view->layoutViewportToClientScaleFactor()); > > I'm still a bit surprised that we need code here to deal with platform vs. non-platform scrollbars. What does scrollbar->occupiedWidth() return if there are platform widget scrollbars? When there's a platform widget, ScrollView's own scrollbars are null so there's no scrollbar to call occupiedWidth() on. (ScrollView's scrollbars get created in ScrollView::updateScrollbars, but when there's a platform widget that method returns immediately without doing any work.)
Simon Fraser (smfr)
Comment 33 2017-12-05 14:09:16 PST
Comment on attachment 328488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328488&action=review >>> Source/WebCore/page/DOMWindow.cpp:1270 >>> + return static_cast<int>((view->layoutViewportRect().height() + scrollbarHeight) * view->layoutViewportToClientScaleFactor()); >> >> I'm still a bit surprised that we need code here to deal with platform vs. non-platform scrollbars. What does scrollbar->occupiedWidth() return if there are platform widget scrollbars? > > When there's a platform widget, ScrollView's own scrollbars are null so there's no scrollbar to call occupiedWidth() on. (ScrollView's scrollbars get created in ScrollView::updateScrollbars, but when there's a platform widget that method returns immediately without doing any work.) OK. Can you push this code down into ScrollVIew? DOMWindow should not be in the position of having to think about platform scrollbars. Maybe an enum param to horizontalScrollbarIntrusion(), like IncludePlatformWidgetScrollbars or something.
Ali Juma
Comment 34 2017-12-06 11:12:30 PST
Created attachment 328597 [details] Patch
Ali Juma
Comment 35 2017-12-06 11:13:04 PST
Comment on attachment 328488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328488&action=review >>>> Source/WebCore/page/DOMWindow.cpp:1270 >>>> + return static_cast<int>((view->layoutViewportRect().height() + scrollbarHeight) * view->layoutViewportToClientScaleFactor()); >>> >>> I'm still a bit surprised that we need code here to deal with platform vs. non-platform scrollbars. What does scrollbar->occupiedWidth() return if there are platform widget scrollbars? >> >> When there's a platform widget, ScrollView's own scrollbars are null so there's no scrollbar to call occupiedWidth() on. (ScrollView's scrollbars get created in ScrollView::updateScrollbars, but when there's a platform widget that method returns immediately without doing any work.) > > OK. Can you push this code down into ScrollVIew? DOMWindow should not be in the position of having to think about platform scrollbars. > > Maybe an enum param to horizontalScrollbarIntrusion(), like IncludePlatformWidgetScrollbars or something. Done.
Maciej Stachowiak
Comment 36 2020-05-30 20:11:18 PDT
Comment on attachment 328597 [details] Patch Seems correct to me, but unfortunately this patch no longer applies, so it needs to be updated.
Simon Fraser (smfr)
Comment 37 2020-06-01 13:12:18 PDT
Does this still reproduce?
Ali Juma
Comment 38 2020-06-01 13:53:36 PDT
It still reproduces. Where did things wind up with making other APIs use layout viewport coordinates? I recall there were some regressions and reverts, but don't remember where we ended up.
Simon Fraser (smfr)
Comment 39 2020-06-01 16:45:29 PDT
We're still in a half-way state, sadly.
Alexey Proskuryakov
Comment 40 2022-09-20 19:03:07 PDT
*** Bug 245361 has been marked as a duplicate of this bug. ***
Bramus
Comment 41 2023-04-20 05:53:41 PDT
@smfr: Do you have an update on the half-way state you mentioned in 2020? If not, is interoperability from WebKit still being considered?
Simon Fraser (smfr)
Comment 42 2023-08-21 20:11:33 PDT
We should probably just use ScrollView::sizeForUnobscuredContent(). Need to think about scrollbars, and content insets. Currently the code uses unobscuredContentRectIncludingScrollbars() which is clearly wrong.
Note You need to log in before you can comment on or make changes to this bug.