Bug 174362 – window.innerWidth/height shouldn't change under pinch zoom

• Component: Layout and Rendering
• Status: ASSIGNED

[Rick Byers]

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)

[Rick Byers]

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]

<rdar://problem/33239908>

[Ali Juma]

Created attachment 316546 [details]
Patch

[Simon Fraser (smfr)]

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]

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]

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 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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Ali Juma]

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 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]

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 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]

Created attachment 324456 [details]
Patch

[Ali Juma]

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]

Created attachment 324459 [details]
Combined patch (175235+174362) for EWS

[Build Bot]

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]

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]

Simon, Hyatt, are you going to review this?

[Frédéric Wang (:fredw)]

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]

Created attachment 327661 [details]
Patch

[Ali Juma]

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 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]

Created attachment 328488 [details]
Patch

[Ali Juma]

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 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 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 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]

Created attachment 328597 [details]
Patch

[Ali Juma]

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 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)]

Does this still reproduce?

[Ali Juma]

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)]

We're still in a half-way state, sadly.

[Alexey Proskuryakov]

*** Bug 245361 has been marked as a duplicate of this bug. ***

[Bramus]

@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)]

We should probably just use ScrollView::sizeForUnobscuredContent(). Need to think about scrollbars, and content insets.

Currently the code uses unobscuredContentRectIncludingScrollbars() which is clearly wrong.