getBoundingClientRect returns wrong value for combination of page zoom and scroll

[Ali Juma]

Created attachment 313852 [details]
Test page

See the attached test case. Increase page zoom (using Cmd-+) and then scroll down. While the blue div is still entirely visible, it gets a bounding client rect whose origin has a negative y coordinate.

[Simon Fraser (smfr)]

What build are you reporting this against? Test Safari Tech Preview 33.

[Ali Juma]

This is happening with Safari Tech Preview 33 (the behavior in Safari 10.1.1 is correct).

[Radar WebKit Bug Importer]

<rdar://problem/32983841>

[Simon Fraser (smfr)]

The bug here is that FrameView::layoutViewport() returns a rect affected by page zoom, but we need to account for that when using its origin to compute layout viewport-relative client rects.

[Simon Fraser (smfr)]

Need to decide if FrameView::layoutViewportRect() and visualViewportRect() are really in Document coordinates (unaffected by page zoom) or not.

[Simon Fraser (smfr)]

Created attachment 314105 [details]
Patch

[Build Bot]

Comment on attachment 314105 [details]
Patch

Attachment 314105 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4017960

New failing tests:
fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed.html

[Build Bot]

Created attachment 314111 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Comment on attachment 314105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314105&action=review

> Source/WebCore/platform/graphics/FloatSize.h:97
> +    FloatSize scaled(float s) const

Can you call the parameter scale for consistency?

> Source/WebCore/platform/graphics/LayoutPoint.h:75
> +    LayoutPoint scaled(float s) const
> +    {
> +        return { m_x * s, m_y * s };
> +    }
> +
> +    LayoutPoint scaled(float sx, float sy) const

In the LayoutPoint and LayoutSize (for the most part) you spelled out scale rather than abbreviating. Would be nice to be consistent.

[Simon Fraser (smfr)]

I will skip the test on iOS since it relies on window.scrollTo().

[Dean Jackson]

Comment on attachment 314105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314105&action=review

> Source/WebCore/dom/SimulatedClick.cpp:77
> +            auto rect = target.getBoundingClientRect();
> +            LayoutRect clientRect = { rect->x(), rect->y(), rect->width(), rect->height() };

Maybe LayoutRect should take DOMRect in a constructor.

> Source/WebCore/dom/SimulatedClick.cpp:78
> +            initCoordinates(clientRect.center());

Although it seems pointless to create a LayoutRect, just to get the center point. Why not initCoordinates(LayoutPoint((rect->x() + rect->width) / 2, .... ?

[Simon Fraser (smfr)]

https://trac.webkit.org/r218982

[Darin Adler]

Comment on attachment 314105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314105&action=review

> Tools/MiniBrowser/mac/WK1BrowserWindowController.m:238
> +    if (_zoomTextOnly)
> +        [_webView makeTextStandardSize:sender];
> +    else
> +        [_webView resetPageZoom:sender];

Should this perhaps always reset both?

[Vik]

This isn't resolved. Is anyone still look at this? It's really causing issues.

[Vik]

(Doesn't work if you add CSS Zoom property to the page body)

[Simon Fraser (smfr)]

This bug wasn't about CSS zoom. Please file a new issue, with a test, for the problem you're seeing.