Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scrollTop and scrollLeft always 0 in DOMContentLoaded handler on a scrolled page #8809

Closed
wants to merge 1 commit into from

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Jan 19, 2023

scrollTop and scrollLeft always 0 in DOMContentLoaded handler on a scrolled page
https://bugs.webkit.org/show_bug.cgi?id=250814

Reviewed by NOBODY (OOPS!).

Merge - https://chromium.googlesource.com/chromium/blink/+/3ad81cf0b724a2a25ee5cc79a09541d9a971ae15

Currently, when performing a "back" navigation, the scroll position
is restored at the time the first layout happens (see
FrameLoader::didFirstLayout). If that does not go through, another
attempt to restore the scroll position is made when the frame
completes loading (see FrameLoader::checkLoadCompleteForThisFrame).

In a closer look, one might notice that in the later, not only the
"back" navigation case is covered, but also "reload".

Patch makes these two attempts of restore the scroll position to
operate under the same circumstances: back and reload.

Apart from bringing them in pair, it also makes it possible to
query the scroll position at the time DOMContentLoaded fires, as
Gecko and Blink (Presto in the past as well) engines do.

* Source/WebCore/loader/FrameLoader.cpp:
(FrameLoader::didFirstLayout):
(1) Add early return, if not presnet "m_frame.page()"
(2) Accounted for Reload types during back navigation
* LayoutTests/fast/loader/scroll-position-restored-on-reload-at-load-event.html: Add Test Case
* LayoutTests/fast/loader/scroll-position-restored-on-reload-at-load-event-expected.txt: Add Test Case Expectation
* LayoutTests/fast/loader/fast/loader/scroll-position-restored-on-back-at-load-event.html: Add Test Case
* LayoutTests/fast/loader/fast/loader/scroll-position-restored-on-back-at-load-event-expected.txt: Add Test Case Expectation

a592af3

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk
βœ… πŸ§ͺ webkitperl ❌ πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

@Ahmad-S792 Ahmad-S792 added the Page Loading For bugs in page loading, including handling of network callbacks. label Jan 19, 2023
@Ahmad-S792 Ahmad-S792 self-assigned this Jan 19, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 19, 2023
…rolled page

scrollTop and scrollLeft always 0 in DOMContentLoaded handler on a scrolled page
https://bugs.webkit.org/show_bug.cgi?id=250814

Reviewed by NOBODY (OOPS!).

Merge - https://chromium.googlesource.com/chromium/blink/+/3ad81cf0b724a2a25ee5cc79a09541d9a971ae15

Currently, when performing a "back" navigation, the scroll position
is restored at the time the first layout happens (see FrameLoader::didFirstLayout).
If that does not go through, another attempt to restore the scroll position
is made when the frame completes loading (see FrameLoader::checkLoadCompleteForThisFrame).

In a closer look, one might notice that in the later, not only the "back" navigation
case is covered, but also "reload".

Patch makes these two attempts of restore the scroll position to operate under the
same circumstances: back and reload.

Apart from bringing them in pair, it also makes it possible to query the scroll
position at the time DOMContentLoaded fires, as Gecko and Blink (Presto in the past as well) engines do.

* Source/WebCore/loader/FrameLoader.cpp:
(FrameLoader::didFirstLayout):
(1) Add early return, if not presnet "m_frame.page()"
(2) Accounted for Reload types during back navigation
* LayoutTests/fast/loader/scroll-position-restored-on-reload-at-load-event.html: Add Test Case
* LayoutTests/fast/loader/scroll-position-restored-on-reload-at-load-event-expected.txt: Add Test Case Expectation
* LayoutTests/fast/loader/scroll-position-restored-on-back-at-load-event.html: Add Test Case
* LayoutTests/fast/loader/scroll-position-restored-on-back-at-load-event-expected.txt: Add Test Case Expectation
* LayoutTests/resources/back.html: Add Test Case Dependency
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Feb 8, 2023
@Ahmad-S792 Ahmad-S792 marked this pull request as ready for review February 19, 2023 23:49
@rniwa rniwa requested a review from smfr February 20, 2023 19:29
@Ahmad-S792 Ahmad-S792 closed this Apr 13, 2023
@Ahmad-S792 Ahmad-S792 deleted the fix250814 branch November 30, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Page Loading For bugs in page loading, including handling of network callbacks.
Projects
None yet
3 participants