Bug 174362 - window.innerWidth/height shouldn't change under pinch zoom
Summary: window.innerWidth/height shouldn't change under pinch zoom
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified iOS 11
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on: 175235
Blocks: 170981
  Show dependency treegraph
 
Reported: 2017-07-11 07:50 PDT by Rick Byers
Modified: 2020-10-02 04:41 PDT (History)
14 users (show)

See Also:


Attachments
Patch (8.39 KB, patch)
2017-07-27 07:28 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2017-08-08 13:28 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Combined patch (175235+174362) for EWS (16.41 KB, patch)
2017-10-20 16:11 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (12.75 KB, patch)
2017-12-05 12:51 PST, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (16.91 KB, patch)
2017-12-06 11:12 PST, Ali Juma
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Byers 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)
Comment 1 Rick Byers 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.
Comment 2 Radar WebKit Bug Importer 2017-07-11 10:27:59 PDT
<rdar://problem/33239908>
Comment 3 Ali Juma 2017-07-27 07:28:42 PDT
Created attachment 316546 [details]
Patch
Comment 4 Simon Fraser (smfr) 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() ?
Comment 5 Ali Juma 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.
Comment 6 Ali Juma 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.
Comment 7 Build Bot 2017-08-08 14:42:14 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-08-08 14:42:15 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-08-08 14:45:47 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-08-08 14:45:48 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-08-08 15:00:51 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-08-08 15:00:53 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-08-08 17:18:40 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-08-08 17:18:42 PDT Comment hidden (obsolete)
Comment 15 Ali Juma 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.
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Frédéric Wang (:fredw) 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.
Comment 19 Ali Juma 2017-10-20 15:52:54 PDT
Created attachment 324456 [details]
Patch
Comment 20 Ali Juma 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.
Comment 21 Ali Juma 2017-10-20 16:11:37 PDT
Created attachment 324459 [details]
Combined patch (175235+174362) for EWS
Comment 22 Build Bot 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
Comment 23 Build Bot 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
Comment 24 Darin Adler 2017-11-22 12:39:10 PST
Simon, Hyatt, are you going to review this?
Comment 25 Frédéric Wang (:fredw) 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?
Comment 26 Ali Juma 2017-11-27 12:46:32 PST
Created attachment 327661 [details]
Patch
Comment 27 Ali Juma 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).
Comment 28 Simon Fraser (smfr) 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.
Comment 29 Ali Juma 2017-12-05 12:51:33 PST
Created attachment 328488 [details]
Patch
Comment 30 Ali Juma 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.
Comment 31 Simon Fraser (smfr) 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?
Comment 32 Ali Juma 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.)
Comment 33 Simon Fraser (smfr) 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.
Comment 34 Ali Juma 2017-12-06 11:12:30 PST
Created attachment 328597 [details]
Patch
Comment 35 Ali Juma 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.
Comment 36 Maciej Stachowiak 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.
Comment 37 Simon Fraser (smfr) 2020-06-01 13:12:18 PDT
Does this still reproduce?
Comment 38 Ali Juma 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.
Comment 39 Simon Fraser (smfr) 2020-06-01 16:45:29 PDT
We're still in a half-way state, sadly.