Bug 225730

Summary: Scrolling must be done after the layout when doing full page zoom
Product: WebKit Reporter: Tomoki Imai <tomoki.imai>
Component: ScrollingAssignee: Tomoki Imai <tomoki.imai>
Status: RESOLVED FIXED    
Severity: Normal CC: don.olmstead, simon.fraser, stephan.szabo, tomoki.imai, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Reproduction in Safari 13.1
none
Patch none

Tomoki Imai
Reported 2021-05-12 20:29:38 PDT
How to reproduce: - Open a long page (for example http://www.rakuten.co.jp/) - Scroll to the bottom - Zoom in Actual result: Compared to the other browsers (Chrome, Firefox), the scroll position after zoom is different from the original position. Expected result: The relative scroll position should be retained as much as possible. Environment: - WinCairo MiniBrowser (trunk r277317) - WebKitGTK MiniBrowser (trunk r277317) - Safari 13.1 (15609.1.20.111.8) Note: In the current code, the updating the scrolling position is done before the re-layout. The scrolling position is updated with the scale, but it is rounded by the original page height. https://github.com/WebKit/WebKit/blob/b37910cc0712363914a6d3d2f655db1c9c892f55/Source/WebCore/page/Frame.cpp#L964-L984
Attachments
Patch (6.43 KB, patch)
2021-05-12 20:33 PDT, Tomoki Imai 		no flags
DetailsFormatted DiffDiff
Patch (6.52 KB, patch)
2021-05-12 20:50 PDT, Tomoki Imai 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Reproduction in Safari 13.1 (14.37 MB, video/mp4)
2021-05-12 21:19 PDT, Tomoki Imai 		no flags
Details
Patch (6.50 KB, patch)
2021-05-13 00:11 PDT, Tomoki Imai 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Tomoki Imai
Comment 1 2021-05-12 20:33:37 PDT
Created attachment 428451 [details] Patch Patch to change the scrolling timing to the after layout.
Simon Fraser (smfr)
Comment 2 2021-05-12 20:39:33 PDT
When you say "Zoom in", how are you zooming?
Simon Fraser (smfr)
Comment 3 2021-05-12 20:43:14 PDT
When I scroll to the bottom on http://www.rakuten.co.jp/, then pinch-zoom to zoom in the scroll position stays in the correct place. Maybe there's a different issue on Windows?
Tomoki Imai
Comment 4 2021-05-12 20:50:26 PDT
Created attachment 428453 [details] Patch Rebased on the trunk
Tomoki Imai
Comment 5 2021-05-12 20:58:29 PDT
(In reply to Simon Fraser (smfr) from comment #2) > When you say "Zoom in", how are you zooming? In Safari, we tried "View" -> "Zoom In" in the menu bar which changes "ZoomFactor". (I'll post a video later) (In reply to Simon Fraser (smfr) from comment #3) > When I scroll to the bottom on http://www.rakuten.co.jp/, then pinch-zoom to > zoom in the scroll position stays in the correct place. > > Maybe there's a different issue on Windows? I didn't try the pinch-zoom, but I guess it changes the "ScaleFactor" not "ZoomFactor". The updating scrolling position code is shared among the ports, so I believe it's not only for Windows/GTK.
Simon Fraser (smfr)
Comment 6 2021-05-12 21:13:54 PDT
OK, I do see a flash of incorrect scroll position when zooming with Command-+ after scrolling down (it jumps up, then back down again).
Tomoki Imai
Comment 7 2021-05-12 21:19:27 PDT
Created attachment 428457 [details] Reproduction in Safari 13.1 This video shows what's happened in Safari 13.1. (Sorry for using the old Safari...) 00:00 At first, the zoom factor is the default value (probably 1). We don't see the cart icon. 00:09 Select "Zoom In", and we now see the cart icon (which we don't see in Chrome/Firefox). 01:12 Even after selecting "Zoom Out" to make the zoom factor the original value, it doesn't retain the original scroll position.
Tomoki Imai
Comment 8 2021-05-12 21:20:32 PDT
(In reply to Simon Fraser (smfr) from comment #6) > OK, I do see a flash of incorrect scroll position when zooming with > Command-+ after scrolling down (it jumps up, then back down again). Thanks for trying! Yes, Command-+ also should reproduce the issue because it's a shortcut for "View" -> "Zoom In".
Tomoki Imai
Comment 9 2021-05-13 00:11:14 PDT
Created attachment 428465 [details] Patch Fixed LayoutTest expected result (Added newlines).
Radar WebKit Bug Importer
Comment 10 2021-05-19 20:30:27 PDT
<rdar://problem/78239415>
EWS
Comment 11 2021-05-19 22:05:47 PDT
Committed r277775 (237936@main): <https://commits.webkit.org/237936@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428465 [details].
Tomoki Imai
Comment 12 2021-05-19 22:09:07 PDT
Thank you for your review!
Note You need to log in before you can comment on or make changes to this bug.