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)
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.
<rdar://problem/33239908>
Created attachment 316546 [details] Patch
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() ?
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.
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.
Comment on attachment 317605 [details] Patch Attachment 317605 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4279745 New failing tests: css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html fast/dom/client-width-height.html css3/viewport-percentage-lengths/viewport-percentage-lengths-calc.html fast/dom/HTMLImageElement/sizes/image-sizes-2x.html fast/dom/inner-width-height.html fast/scrolling/scrollbar-tickmarks-hittest.html fast/dom/HTMLImageElement/sizes/image-sizes-1x.html fast/events/prevent-default-prevents-interaction-with-scrollbars.html fast/overflow/horizontal-scroll-after-back.html fast/events/contextmenu-on-scrollbars.html fast/dom/client-width-height-quirks.html fast/overflow/scrollbar-restored.html css3/viewport-percentage-lengths/viewport-percentage-lengths-percent-size-child.html css3/viewport-percentage-lengths/viewport-percentage-lengths-anonymous-block.html fast/dom/window-inner-size-scaling.html fast/dom/iframe-inner-size-scaling.html
Created attachment 317618 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317605 [details] Patch Attachment 317605 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4279751 New failing tests: css3/viewport-percentage-lengths/viewport-percentage-lengths-percent-size-child.html css3/viewport-percentage-lengths/viewport-percentage-lengths-anonymous-block.html fast/dom/iframe-inner-size-scaling.html
Created attachment 317619 [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
Comment on attachment 317605 [details] Patch Attachment 317605 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4279765 New failing tests: css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html fast/dom/client-width-height.html fast/events/contextmenu-on-scrollbars.html css3/viewport-percentage-lengths/viewport-percentage-lengths-calc.html fast/dom/HTMLImageElement/sizes/image-sizes-2x.html fast/dom/inner-width-height.html fast/scrolling/scrollbar-tickmarks-hittest.html fast/dom/HTMLImageElement/sizes/image-sizes-1x.html fast/overflow/horizontal-scroll-after-back.html css3/viewport-percentage-lengths/viewport-percentage-lengths-anonymous-block.html fast/dom/client-width-height-quirks.html fast/overflow/scrollbar-restored.html css3/viewport-percentage-lengths/viewport-percentage-lengths-percent-size-child.html fast/events/prevent-default-prevents-interaction-with-scrollbars.html fast/dom/window-inner-size-scaling.html fast/dom/iframe-inner-size-scaling.html
Created attachment 317623 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317605 [details] Patch Attachment 317605 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4280626 New failing tests: fast/dom/window-inner-size-zooming.html
Created attachment 317653 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 318498 [details] Patch Updated patch to account for the size of platform scrollbars (used by WK1). This still depends on bug 175235.
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
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
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.
Created attachment 324456 [details] Patch
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.
Created attachment 324459 [details] Combined patch (175235+174362) for EWS
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
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
Simon, Hyatt, are you going to review this?
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?
Created attachment 327661 [details] Patch
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).
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.
Created attachment 328488 [details] Patch
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.
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?
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.)
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.
Created attachment 328597 [details] Patch
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.
Comment on attachment 328597 [details] Patch Seems correct to me, but unfortunately this patch no longer applies, so it needs to be updated.
Does this still reproduce?
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.
We're still in a half-way state, sadly.
*** Bug 245361 has been marked as a duplicate of this bug. ***
@smfr: Do you have an update on the half-way state you mentioned in 2020? If not, is interoperability from WebKit still being considered?
We should probably just use ScrollView::sizeForUnobscuredContent(). Need to think about scrollbars, and content insets. Currently the code uses unobscuredContentRectIncludingScrollbars() which is clearly wrong.