Bug 173841

Summary: getBoundingClientRect returns wrong value for combination of page zoom and scroll
Product: WebKit Reporter: Ali Juma <ajuma>
Component: DOMAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, darin, dbates, dino, esprehn+autocc, kangil.han, sam, simon.fraser, vik, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test page
none
Patch
dino: review+, buildbot: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2 none

Ali Juma
Reported 2017-06-26 10:50:12 PDT
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.
Attachments
Test page (931 bytes, text/html)
2017-06-26 10:50 PDT, Ali Juma 		no flags
Details
Patch (26.61 KB, patch)
2017-06-28 20:43 PDT, Simon Fraser (smfr) 		dino: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (924.54 KB, application/zip)
2017-06-28 22:23 PDT, Build Bot 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2017-06-26 10:53:25 PDT
What build are you reporting this against? Test Safari Tech Preview 33.
Ali Juma
Comment 2 2017-06-26 10:56:13 PDT
This is happening with Safari Tech Preview 33 (the behavior in Safari 10.1.1 is correct).
Radar WebKit Bug Importer
Comment 3 2017-06-26 10:57:56 PDT
<rdar://problem/32983841>
Simon Fraser (smfr)
Comment 4 2017-06-26 20:56:29 PDT
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)
Comment 5 2017-06-26 21:30:45 PDT
Need to decide if FrameView::layoutViewportRect() and visualViewportRect() are really in Document coordinates (unaffected by page zoom) or not.
Simon Fraser (smfr)
Comment 6 2017-06-28 20:43:45 PDT
Created attachment 314105 [details] Patch
Build Bot
Comment 7 2017-06-28 22:23:45 PDT
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
Comment 8 2017-06-28 22:23:46 PDT
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 9 2017-06-28 22:30:35 PDT
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)
Comment 10 2017-06-29 16:41:23 PDT
I will skip the test on iOS since it relies on window.scrollTo().
Dean Jackson
Comment 11 2017-06-29 17:00:52 PDT
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)
Comment 12 2017-06-29 19:09:38 PDT
https://trac.webkit.org/r218982
Darin Adler
Comment 13 2017-06-30 16:52:10 PDT
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
Comment 14 2021-03-02 12:44:35 PST
This isn't resolved. Is anyone still look at this? It's really causing issues.
Vik
Comment 15 2021-03-02 12:45:16 PST
(Doesn't work if you add CSS Zoom property to the page body)
Simon Fraser (smfr)
Comment 16 2021-03-02 13:18:45 PST
This bug wasn't about CSS zoom. Please file a new issue, with a test, for the problem you're seeing.
Note You need to log in before you can comment on or make changes to this bug.