Scrolling to fragment shouldn't happen as a part of updating style
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Nov 2019 05:09:36 +0000 (05:09 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Nov 2019 05:09:36 +0000 (05:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203982

Reviewed by Simon Fraser.

Source/WebCore:

Don't scroll to the current URL's fragment at the end of resolveStyle. Instead, schedule a task
to scroll to the current URL's fragment when all pending stylesheets have finished loading.

This patch also moves the code which sets a Document's m_gotoAnchorNeededAfterStylesheetsLoadflag
from FrameView to FrameLoader as FrameView shouldn't be relying on the states of pending stylesheets.

* dom/Document.cpp:
(WebCore::Document::resolveStyle): Removed the code to scroll to the current URL's fragment.
(WebCore::Document::didRemoveAllPendingStylesheet): Added a code to schedule a task to scoll.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::scrollToFragmentWithParentBoundary): Moved the code to trigger the code
in didRemoveAllPendingStylesheet from FrameView.
* page/FrameView.cpp:
(WebCore::FrameView::scrollToFragment):
(WebCore::FrameView::scrollToFragmentInternal): Renamed from scrollToAnchor since this function sets
the current anchor. Also removed the code which defers the scrolling based on pending stylesheets'
states since such a code doesn't belong in FrameView.
* page/FrameView.h:

LayoutTests:

Made an existing test more robust.

* fast/parser/adoption-agency-unload-iframe-4.html: Made the iframe's data URL not cachable.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252761 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/parser/adoption-agency-unload-iframe-4.html
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h

index ae7395f..f2d8e3f 100644 (file)
@@ -1,3 +1,14 @@
+2019-11-21  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Scrolling to fragment shouldn't happen as a part of updating style
+        https://bugs.webkit.org/show_bug.cgi?id=203982
+
+        Reviewed by Simon Fraser.
+
+        Made an existing test more robust.
+
+        * fast/parser/adoption-agency-unload-iframe-4.html: Made the iframe's data URL not cachable.
+
 2019-11-21  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Remove font-variant @font-face descriptor
index dae623b..a884c8c 100644 (file)
@@ -37,7 +37,7 @@ test.step(() => {
 
     doc.write(`<div><b><p><script>
         parent.p = document.querySelector('p');
-        document.write('<link rel="stylesheet" href="data:,a">');
+        document.write('<link rel="stylesheet" href="data:,a${(new Date).toISOString() + Math.random()}">');
         location.hash = 'target';
     <\/script></b></p></div></body>`);
 });
index 90992f0..ca012f3 100644 (file)
@@ -1,3 +1,29 @@
+2019-11-21  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Scrolling to fragment shouldn't happen as a part of updating style
+        https://bugs.webkit.org/show_bug.cgi?id=203982
+
+        Reviewed by Simon Fraser.
+
+        Don't scroll to the current URL's fragment at the end of resolveStyle. Instead, schedule a task
+        to scroll to the current URL's fragment when all pending stylesheets have finished loading.
+
+        This patch also moves the code which sets a Document's m_gotoAnchorNeededAfterStylesheetsLoadflag
+        from FrameView to FrameLoader as FrameView shouldn't be relying on the states of pending stylesheets.
+
+        * dom/Document.cpp:
+        (WebCore::Document::resolveStyle): Removed the code to scroll to the current URL's fragment.
+        (WebCore::Document::didRemoveAllPendingStylesheet): Added a code to schedule a task to scoll.
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::scrollToFragmentWithParentBoundary): Moved the code to trigger the code
+        in didRemoveAllPendingStylesheet from FrameView.
+        * page/FrameView.cpp:
+        (WebCore::FrameView::scrollToFragment):
+        (WebCore::FrameView::scrollToFragmentInternal): Renamed from scrollToAnchor since this function sets
+        the current anchor. Also removed the code which defers the scrolling based on pending stylesheets'
+        states since such a code doesn't belong in FrameView.
+        * page/FrameView.h:
+
 2019-11-21  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Remove font-variant @font-face descriptor
index c968e5d..e6bb792 100644 (file)
@@ -2001,9 +2001,6 @@ void Document::resolveStyle(ResolveStyleType type)
     // check if they need to be resumed after layout.
     if (updatedCompositingLayers && !frameView.needsLayout())
         frameView.viewportContentsChanged();
-
-    if (m_gotoAnchorNeededAfterStylesheetsLoad && !styleScope().hasPendingSheets())
-        frameView.scrollToFragment(m_url);
 }
 
 void Document::updateTextRenderer(Text& text, unsigned offsetOfReplacedText, unsigned lengthOfReplacedText)
@@ -3472,6 +3469,20 @@ void Document::didRemoveAllPendingStylesheet()
 {
     if (auto* parser = scriptableDocumentParser())
         parser->executeScriptsWaitingForStylesheetsSoon();
+
+    if (m_gotoAnchorNeededAfterStylesheetsLoad) {
+        // https://html.spec.whatwg.org/multipage/browsing-the-web.html#try-to-scroll-to-the-fragment
+        eventLoop().queueTask(TaskSource::Networking, [protectedThis = makeRef(*this), this] {
+            auto frameView = makeRefPtr(view());
+            if (!frameView)
+                return;
+            if (!haveStylesheetsLoaded()) {
+                m_gotoAnchorNeededAfterStylesheetsLoad = true;
+                return;
+            }
+            frameView->scrollToFragment(m_url);
+        });
+    }
 }
 
 bool Document::usesStyleBasedEditability() const
index 7b10cb9..9b4211b 100644 (file)
@@ -3250,12 +3250,19 @@ static bool isSameDocumentReload(bool isNewNavigation, FrameLoadType loadType)
 
 void FrameLoader::scrollToFragmentWithParentBoundary(const URL& url, bool isNewNavigation)
 {
-    FrameView* view = m_frame.view();
-    if (!view)
+    auto view = makeRefPtr(m_frame.view());
+    auto document = makeRefPtr(m_frame.document());
+    if (!view || !document)
         return;
 
-    if (isSameDocumentReload(isNewNavigation, m_loadType) || itemAllowsScrollRestoration(history().currentItem()))
-        view->scrollToFragment(url);
+    if (isSameDocumentReload(isNewNavigation, m_loadType) || itemAllowsScrollRestoration(history().currentItem())) {
+        // https://html.spec.whatwg.org/multipage/browsing-the-web.html#try-to-scroll-to-the-fragment
+        if (!document->haveStylesheetsLoaded())
+            document->setGotoAnchorNeededAfterStylesheetsLoad(true);
+        else
+            view->scrollToFragment(url);
+    }
+
 }
 
 bool FrameLoader::shouldClose()
index a524e43..0f70ac7 100644 (file)
@@ -2169,12 +2169,12 @@ void FrameView::restoreScrollbar()
 bool FrameView::scrollToFragment(const URL& url)
 {
     String fragmentIdentifier = url.fragmentIdentifier();
-    if (scrollToAnchor(fragmentIdentifier))
+    if (scrollToFragmentInternal(fragmentIdentifier))
         return true;
 
     // Try again after decoding the ref, based on the document's encoding.
     if (TextResourceDecoder* decoder = frame().document()->decoder()) {
-        if (scrollToAnchor(decodeURLEscapeSequences(fragmentIdentifier, decoder->encoding())))
+        if (scrollToFragmentInternal(decodeURLEscapeSequences(fragmentIdentifier, decoder->encoding())))
             return true;
     }
 
@@ -2182,7 +2182,7 @@ bool FrameView::scrollToFragment(const URL& url)
     return false;
 }
 
-bool FrameView::scrollToAnchor(const String& fragmentIdentifier)
+bool FrameView::scrollToFragmentInternal(const String& fragmentIdentifier)
 {
     LOG(Scrolling, "FrameView::scrollToAnchor %s", fragmentIdentifier.utf8().data());
 
@@ -2192,13 +2192,7 @@ bool FrameView::scrollToAnchor(const String& fragmentIdentifier)
 
     ASSERT(frame().document());
     auto& document = *frame().document();
-
-    if (!document.haveStylesheetsLoaded()) {
-        document.setGotoAnchorNeededAfterStylesheetsLoad(true);
-        return false;
-    }
-
-    document.setGotoAnchorNeededAfterStylesheetsLoad(false);
+    RELEASE_ASSERT(document.haveStylesheetsLoaded());
 
     Element* anchorElement = document.findAnchor(fragmentIdentifier);
 
index fd4f9d6..ded7a5b 100644 (file)
@@ -425,7 +425,6 @@ public:
     WEBCORE_EXPORT void adjustPageHeightDeprecated(float* newBottom, float oldTop, float oldBottom, float bottomLimit);
 
     bool scrollToFragment(const URL&);
-    bool scrollToAnchor(const String&);
     void maintainScrollPositionAtAnchor(ContainerNode*);
     WEBCORE_EXPORT void scrollElementToRect(const Element&, const IntRect&);
 
@@ -778,6 +777,7 @@ private:
 
     void updateWidgetPositionsTimerFired();
 
+    bool scrollToFragmentInternal(const String&);
     void scrollToAnchor();
     void scrollPositionChanged(const ScrollPosition& oldPosition, const ScrollPosition& newPosition);
     void scrollableAreaSetChanged();