Changeset 252761 in webkit

Timestamp:

Nov 21, 2019 9:09:36 PM (4 years ago)

Author:

rniwa@webkit.org

Message:

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.

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/parser/adoption-agency-unload-iframe-4.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (2 diffs)
• Source/WebCore/loader/FrameLoader.cpp (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (3 diffs)
• Source/WebCore/page/FrameView.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/fast/parser/adoption-agency-unload-iframe-4.html

@@ -38 +38 @@
     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>`);

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -2002 +2002 @@
     if (updatedCompositingLayers && !frameView.needsLayout())
         frameView.viewportContentsChanged();
-
-    if (m_gotoAnchorNeededAfterStylesheetsLoad && !styleScope().hasPendingSheets())
-        frameView.scrollToFragment(m_url);
 }
 
@@ -3473 +3470 @@
     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);
+        });
+    }
 }
 

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -3251 +3251 @@
 void FrameLoader::scrollToFragmentWithParentBoundary(const URL& url, bool isNewNavigation)
 {
-    FrameView* view = m_frame.view();
-    if (!view)
-        return;
-
-    if (isSameDocumentReload(isNewNavigation, m_loadType) || itemAllowsScrollRestoration(history().currentItem()))
-        view->scrollToFragment(url);
+    auto view = makeRefPtr(m_frame.view());
+    auto document = makeRefPtr(m_frame.document());
+    if (!view || !document)
+        return;
+
+    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);
+    }
+
 }
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -2170 +2170 @@
 {
     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;
     }
@@ -2183 +2183 @@
 }
 
-bool FrameView::scrollToAnchor(const String& fragmentIdentifier)
+bool FrameView::scrollToFragmentInternal(const String& fragmentIdentifier)
 {
     LOG(Scrolling, "FrameView::scrollToAnchor %s", fragmentIdentifier.utf8().data());
@@ -2193 +2193 @@
     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);

trunk/Source/WebCore/page/FrameView.h

@@ -426 +426 @@
 
     bool scrollToFragment(const URL&);
-    bool scrollToAnchor(const String&);
     void maintainScrollPositionAtAnchor(ContainerNode*);
     WEBCORE_EXPORT void scrollElementToRect(const Element&, const IntRect&);
@@ -779 +778 @@
     void updateWidgetPositionsTimerFired();
 
+    bool scrollToFragmentInternal(const String&);
     void scrollToAnchor();
     void scrollPositionChanged(const ScrollPosition& oldPosition, const ScrollPosition& newPosition);