Hold the Scroll to the Text Fragment until a user scroll happens. #4371

megangardner · 2022-09-15T00:36:25Z

`4dc689b`

Hold the Scroll to the Text Fragment until a user scroll happens.
https://bugs.webkit.org/show_bug.cgi?id=245207
rdar://99668200

Reviewed by Tim Horton.

For each layout that comes in until the user scrolls we need to reset
the scroll to keep the fragment in the middle of the page.
Otherwise, more content can come in the scroll the fragment off the
page, which can be very confusing to the user.

* Source/WebCore/page/FrameView.cpp:
(WebCore::FrameView::reset):
(WebCore::FrameView::scrollToFragment):
(WebCore::FrameView::maintainScrollPositionAtRange):
(WebCore::FrameView::textFragmentIndicatorTimerFired):
(WebCore::FrameView::cancelScheduledTextFragmentIndicatorTimer):
(WebCore::FrameView::scrollToRange):
(WebCore::FrameView::performPostLayoutTasks):
* Source/WebCore/page/FrameView.h:

Canonical link: https://commits.webkit.org/254507@main

7efddec

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🛠 mac-AS-debug~~	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	~~🧪 api-mac~~	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
✅ 🛠 🧪 merge	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	~~🧪 mac-wk2-stress~~

webkit-early-warning-system · 2022-09-15T00:36:35Z

EWS run on previous version of this PR (hash 8325577)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	⏳ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-15T00:38:00Z

EWS run on previous version of this PR (hash 62c50f1)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	⏳ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

hortont424 · 2022-09-15T00:40:36Z

Source/WebCore/page/FrameView.cpp

@@ -2321,6 +2322,19 @@ void FrameView::maintainScrollPositionAtAnchor(ContainerNode* anchorNode)
        scrollToAnchor();
 }

+void FrameView::maintainScrollPositionAtRange(SimpleRange* range)
+{
+    UNUSED_PARAM(range);


Get rid of this, since you clearly use it.

hortont424 · 2022-09-15T00:40:53Z

Source/WebCore/page/FrameView.cpp

    m_pendingTextFragmentIndicatorRange.reset();
    m_pendingTextFragmentIndicatorText = String();
    m_delayedTextFragmentIndicatorTimer.stop();
+


hortont424 · 2022-09-15T00:42:54Z

Source/WebCore/page/FrameView.cpp

@@ -3531,6 +3547,30 @@ void FrameView::scrollToAnchor()
    cancelScheduledScrolls();
 }

+void FrameView::scrollToRange()


This is a super vague name. Can we call it "scrollToTextFragment"?

(or scrollToTextFragmentRange, or something)

hortont424 · 2022-09-15T00:43:19Z

Source/WebCore/page/FrameView.h

@@ -472,6 +472,7 @@ class FrameView final : public ScrollView {

    bool scrollToFragment(const URL&);
    void maintainScrollPositionAtAnchor(ContainerNode*);
+    void maintainScrollPositionAtRange(SimpleRange*);


ditto here, not just "range" but more importantly "text fragment"

hortont424 · 2022-09-15T00:44:21Z

Source/WebCore/page/FrameView.h

@@ -911,6 +913,7 @@ class FrameView final : public ScrollView {
    RefPtr<Node> m_nodeToDraw;
    std::optional<SimpleRange> m_pendingTextFragmentIndicatorRange;
    String m_pendingTextFragmentIndicatorText;
+    bool m_doNotResetFragmentTimer { false };


I think this isn't just about the timer. It's "should incoming scrolling cancel maintaining scroll-to-text-fragment position". Try to make a better name. It's hard to compress down.

webkit-early-warning-system · 2022-09-15T04:46:54Z

EWS run on previous version of this PR (hash a33c32b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 api-mac	~~🧪 api-gtk~~
	✅ 🛠 tv	⏳ 🧪 mac-wk1
	✅ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

hortont424 · 2022-09-15T04:52:39Z

Source/WebCore/page/FrameView.cpp

@@ -2321,6 +2322,18 @@ void FrameView::maintainScrollPositionAtAnchor(ContainerNode* anchorNode)
        scrollToAnchor();
 }

+void FrameView::maintainScrollPositionAtScrollToTextFragmentRange(SimpleRange* range)


Why doesn't this take a reference?

hortont424 · 2022-09-15T04:53:10Z

Source/WebCore/page/FrameView.cpp

+{
+    LOG(Scrolling, "FrameView::maintainScrollPositionAtScrollToTextFragmentRange at %p", range);
+
+    m_pendingTextFragmentIndicatorRange = *range;


Since clearly it must be nonnull

hortont424 · 2022-09-15T04:53:53Z

Source/WebCore/page/FrameView.cpp

@@ -2509,6 +2521,8 @@ void FrameView::textFragmentIndicatorTimerFired()

 void FrameView::cancelScheduledTextFragmentIndicatorTimer()
 {
+    if (m_skipScrollResetofScrollToTextFragementRange)


Your camel casing fell apart in the middle

Also your spelling of fragment

hortont424 · 2022-09-15T04:54:33Z

Source/WebCore/page/FrameView.cpp

+    ASSERT(frame().document());
+    Ref document = *frame().document();
+
+    SetForScope doNotResetFragmentTimer(m_skipScrollResetofScrollToTextFragementRange, true);


The local name should be fixed up too

webkit-early-warning-system · 2022-09-15T06:20:36Z

7efddec

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🛠 mac-AS-debug~~	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	~~🧪 api-mac~~	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
✅ 🛠 🧪 merge	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	~~🧪 mac-wk2-stress~~

https://bugs.webkit.org/show_bug.cgi?id=245207 rdar://99668200 Reviewed by Tim Horton. For each layout that comes in until the user scrolls we need to reset the scroll to keep the fragment in the middle of the page. Otherwise, more content can come in the scroll the fragment off the page, which can be very confusing to the user. * Source/WebCore/page/FrameView.cpp: (WebCore::FrameView::reset): (WebCore::FrameView::scrollToFragment): (WebCore::FrameView::maintainScrollPositionAtRange): (WebCore::FrameView::textFragmentIndicatorTimerFired): (WebCore::FrameView::cancelScheduledTextFragmentIndicatorTimer): (WebCore::FrameView::scrollToRange): (WebCore::FrameView::performPostLayoutTasks): * Source/WebCore/page/FrameView.h: Canonical link: https://commits.webkit.org/254507@main

webkit-commit-queue · 2022-09-15T08:05:39Z

Committed 254507@main (4dc689b): https://commits.webkit.org/254507@main

Reviewed commits have been landed. Closing PR #4371 and removing active labels.

megangardner requested a review from cdumez as a code owner September 15, 2022 00:36

megangardner self-assigned this Sep 15, 2022

megangardner added DOM For bugs specific to XML/HTML DOM elements (including parsing). WebKit Nightly Build labels Sep 15, 2022

megangardner force-pushed the eng/Hold-the-Scroll-to-the-Text-Fragment-until-a-user-scroll-happens- branch from 8325577 to 62c50f1 Compare September 15, 2022 00:37

megangardner requested review from whsieh, hortont424 and dcrousso September 15, 2022 00:38

hortont424 reviewed Sep 15, 2022

View reviewed changes

megangardner force-pushed the eng/Hold-the-Scroll-to-the-Text-Fragment-until-a-user-scroll-happens- branch from 62c50f1 to a33c32b Compare September 15, 2022 04:46

hortont424 approved these changes Sep 15, 2022

View reviewed changes

megangardner force-pushed the eng/Hold-the-Scroll-to-the-Text-Fragment-until-a-user-scroll-happens- branch from a33c32b to 7efddec Compare September 15, 2022 06:20

megangardner added the merge-queue Applied to send a pull request to merge-queue label Sep 15, 2022

webkit-early-warning-system force-pushed the eng/Hold-the-Scroll-to-the-Text-Fragment-until-a-user-scroll-happens- branch from 7efddec to 4dc689b Compare September 15, 2022 08:05

webkit-early-warning-system merged commit 4dc689b into WebKit:main Sep 15, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hold the Scroll to the Text Fragment until a user scroll happens. #4371

Hold the Scroll to the Text Fragment until a user scroll happens. #4371

megangardner commented Sep 15, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 15, 2022 •

edited

webkit-early-warning-system commented Sep 15, 2022 •

edited

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

webkit-early-warning-system commented Sep 15, 2022 •

edited

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

hortont424 Sep 15, 2022

webkit-early-warning-system commented Sep 15, 2022 •

edited

webkit-commit-queue commented Sep 15, 2022

Hold the Scroll to the Text Fragment until a user scroll happens. #4371

Hold the Scroll to the Text Fragment until a user scroll happens. #4371

Conversation

megangardner commented Sep 15, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 15, 2022 • edited

webkit-early-warning-system commented Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Sep 15, 2022 • edited

webkit-commit-queue commented Sep 15, 2022

megangardner commented Sep 15, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 15, 2022 •

edited

webkit-early-warning-system commented Sep 15, 2022 •

edited

webkit-early-warning-system commented Sep 15, 2022 •

edited

webkit-early-warning-system commented Sep 15, 2022 •

edited