From 87268531070c237d61fb6aef76d199eae975be04 Mon Sep 17 00:00:00 2001 From: Jan-Niklas Jaeschke Date: Fri, 17 May 2024 12:16:00 +0000 Subject: [PATCH] Bug 1895555 - Text Fragments: Implement same-document navigation. r=farre,dom-core Same-document navigation follows a different code path than normal navigation and was therefore not covered in the initial implementation for text fragments. Same-document navigation does not set a URI in the `Document`, which is the way cross-document navigation would parse text directives from the URL. Instead, `nsDocShell::ScrollToAnchor()` is called via `nsDocShell::InternalLoad()`-> `nsDocShell::HandleSameDocumentNavigation()`. This code path needs to parse and remove the fragment directive from the new fragment to be able to find text fragments and to allow for element-id fallback. `nsDocShell::ScrollToAnchor()` needs to start an attempt to scroll to the text fragment if it exists. It must not, however, clear the uninvoked text directives, because a same-document navigation could happen before the document is fully loaded, hence the target text might not be part of the DOM tree. As per spec, a second attempt to scroll to the text fragment is done after the load is completed. This is done by `Document::ScrollToRef()`, which is called by `nsDocumentViewer::LoadComplete()` after the load has finished. This call will clear the uninvoked directives. Differential Revision: https://phabricator.services.mozilla.com/D209726 --- docshell/base/nsDocShell.cpp | 35 ++++++++++++++++--- dom/base/Document.cpp | 2 ++ dom/base/FragmentDirective.cpp | 26 +++++++++----- dom/base/FragmentDirective.h | 14 ++++++++ .../iframes.sub.html.ini | 5 +-- .../percent-encoding.html.ini | 15 -------- .../same-document-tests.html.ini | 8 ----- .../scroll-to-text-fragment-same-doc.html.ini | 3 -- .../same-document-test-sync-load.html | 27 ++++++++++++++ 9 files changed, 91 insertions(+), 44 deletions(-) delete mode 100644 testing/web-platform/meta/scroll-to-text-fragment/same-document-tests.html.ini delete mode 100644 testing/web-platform/meta/scroll-to-text-fragment/scroll-to-text-fragment-same-doc.html.ini create mode 100644 testing/web-platform/tests/scroll-to-text-fragment/same-document-test-sync-load.html diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index eadb9eafa0b5..91ea123dfb48 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -64,6 +64,7 @@ #include "mozilla/dom/ContentFrameMessageManager.h" #include "mozilla/dom/DocGroup.h" #include "mozilla/dom/Element.h" +#include "mozilla/dom/FragmentDirective.h" #include "mozilla/dom/HTMLAnchorElement.h" #include "mozilla/dom/HTMLIFrameElement.h" #include "mozilla/dom/PerformanceNavigation.h" @@ -8476,6 +8477,17 @@ bool nsDocShell::IsSameDocumentNavigation(nsDocShellLoadState* aLoadState, rvURINew = aLoadState->URI()->GetHasRef(&aState.mNewURIHasRef); } + // A Fragment Directive must be removed from the new hash in order to allow + // fallback element id scroll. Additionally, the extracted parsed text + // directives need to be stored for further use. + nsTArray textDirectives; + if (FragmentDirective::ParseAndRemoveFragmentDirectiveFromFragmentString( + aState.mNewHash, &textDirectives)) { + if (Document* doc = GetDocument()) { + doc->FragmentDirective()->SetTextDirectives(std::move(textDirectives)); + } + } + if (currentURI && NS_SUCCEEDED(rvURINew)) { nsresult rvURIOld = currentURI->GetRef(aState.mCurrentHash); if (NS_SUCCEEDED(rvURIOld)) { @@ -10701,6 +10713,24 @@ nsresult nsDocShell::ScrollToAnchor(bool aCurHasRef, bool aNewHasRef, rootScroll->ClearDidHistoryRestore(); } + // If it's a load from history, we don't have any anchor jumping to do. + // Scrollbar position will be restored by the caller based on positions stored + // in session history. + bool scroll = aLoadType != LOAD_HISTORY && aLoadType != LOAD_RELOAD_NORMAL; + // If the load contains text directives, try to apply them. This may fail if + // the load is a same-document load that was initiated before the document was + // fully loaded and the target is not yet included in the DOM tree. + // For this case, the `uninvokedTextDirectives` are not cleared, so that + // `Document::ScrollToRef()` can re-apply the text directive. + // `Document::ScrollToRef()` is (presumably) the second "async" call mentioned + // in sec. 7.4.2.3.3 in the HTML spec, "Fragment navigations": + // https://html.spec.whatwg.org/#scroll-to-fragid:~:text=This%20algorithm%20will%20be%20called%20twice + const bool hasScrolledToTextFragment = + presShell->HighlightAndGoToTextFragment(scroll); + if (hasScrolledToTextFragment) { + return NS_OK; + } + // If we have no new anchor, we do not want to scroll, unless there is a // current anchor and we are doing a history load. So return if we have no // new anchor, and there is no current anchor or the load is not a history @@ -10712,11 +10742,6 @@ nsresult nsDocShell::ScrollToAnchor(bool aCurHasRef, bool aNewHasRef, // Both the new and current URIs refer to the same page. We can now // browse to the hash stored in the new URI. - // If it's a load from history, we don't have any anchor jumping to do. - // Scrollbar position will be restored by the caller based on positions stored - // in session history. - bool scroll = aLoadType != LOAD_HISTORY && aLoadType != LOAD_RELOAD_NORMAL; - if (aNewHash.IsEmpty()) { // 2. If fragment is the empty string, then return the special value top of // the document. diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp index 92d87a5e4a2a..c5ac37f04012 100644 --- a/dom/base/Document.cpp +++ b/dom/base/Document.cpp @@ -13121,6 +13121,8 @@ void Document::ScrollToRef() { const bool didScrollToTextFragment = presShell->HighlightAndGoToTextFragment(true); + FragmentDirective()->ClearUninvokedDirectives(); + // 2. If fragment is the empty string and no text directives have been // scrolled to, then return the special value top of the document. if (didScrollToTextFragment || mScrollToRef.IsEmpty()) { diff --git a/dom/base/FragmentDirective.cpp b/dom/base/FragmentDirective.cpp index 3300a85751b3..cb1b892f46df 100644 --- a/dom/base/FragmentDirective.cpp +++ b/dom/base/FragmentDirective.cpp @@ -51,6 +51,21 @@ JSObject* FragmentDirective::WrapObject(JSContext* aCx, return FragmentDirective_Binding::Wrap(aCx, this, aGivenProto); } +bool FragmentDirective::ParseAndRemoveFragmentDirectiveFromFragmentString( + nsCString& aFragment, nsTArray* aTextDirectives) { + ParsedFragmentDirectiveResult fragmentDirective; + const bool hasRemovedFragmentDirective = + StaticPrefs::dom_text_fragments_enabled() && + parse_fragment_directive(&aFragment, &fragmentDirective); + if (hasRemovedFragmentDirective) { + aFragment = fragmentDirective.url_without_fragment_directive; + if (aTextDirectives) { + aTextDirectives->SwapElements(fragmentDirective.text_directives); + } + } + return hasRemovedFragmentDirective; +} + void FragmentDirective::ParseAndRemoveFragmentDirectiveFromFragment( nsCOMPtr& aURI, nsTArray* aTextDirectives) { if (!aURI || !StaticPrefs::dom_text_fragments_enabled()) { @@ -65,18 +80,12 @@ void FragmentDirective::ParseAndRemoveFragmentDirectiveFromFragment( nsAutoCString hash; aURI->GetRef(hash); - ParsedFragmentDirectiveResult fragmentDirective; const bool hasRemovedFragmentDirective = - parse_fragment_directive(&hash, &fragmentDirective); + ParseAndRemoveFragmentDirectiveFromFragmentString(hash, aTextDirectives); if (!hasRemovedFragmentDirective) { return; } - Unused << NS_MutateURI(aURI) - .SetRef(fragmentDirective.url_without_fragment_directive) - .Finalize(aURI); - if (aTextDirectives) { - aTextDirectives->SwapElements(fragmentDirective.text_directives); - } + Unused << NS_MutateURI(aURI).SetRef(hash).Finalize(aURI); } nsTArray> FragmentDirective::FindTextFragmentsInDocument() { @@ -88,7 +97,6 @@ nsTArray> FragmentDirective::FindTextFragmentsInDocument() { textDirectiveRanges.AppendElement(range); } } - mUninvokedTextDirectives.Clear(); return textDirectiveRanges; } diff --git a/dom/base/FragmentDirective.h b/dom/base/FragmentDirective.h index 8972556d6ca1..881009531bb5 100644 --- a/dom/base/FragmentDirective.h +++ b/dom/base/FragmentDirective.h @@ -72,6 +72,9 @@ class FragmentDirective final : public nsISupports, public nsWrapperCache { return !mUninvokedTextDirectives.IsEmpty(); }; + /** Clears all uninvoked directives. */ + void ClearUninvokedDirectives() { mUninvokedTextDirectives.Clear(); } + /** Searches for the current uninvoked text directives and creates a range for * each one that is found. * @@ -94,6 +97,17 @@ class FragmentDirective final : public nsISupports, public nsWrapperCache { nsCOMPtr& aURI, nsTArray* aTextDirectives = nullptr); + /** Parses the fragment directive and removes it from the hash, given as + * string. This operation happens in-place. + * + * This function is called internally by + * `ParseAndRemoveFragmentDirectiveFromFragment()`. + * + * This function returns true if it modified `aFragment`. + */ + static bool ParseAndRemoveFragmentDirectiveFromFragmentString( + nsCString& aFragment, nsTArray* aTextDirectives = nullptr); + private: RefPtr FindRangeForTextDirective( const TextDirective& aTextDirective); diff --git a/testing/web-platform/meta/scroll-to-text-fragment/iframes.sub.html.ini b/testing/web-platform/meta/scroll-to-text-fragment/iframes.sub.html.ini index 4709d3dee85e..a4559dffa72b 100644 --- a/testing/web-platform/meta/scroll-to-text-fragment/iframes.sub.html.ini +++ b/testing/web-platform/meta/scroll-to-text-fragment/iframes.sub.html.ini @@ -2,11 +2,8 @@ [Text fragment specified in iframe.src] expected: FAIL - [Navigate same-origin iframe via window.location] - expected: FAIL - [Cross-origin with element-id fallback] expected: FAIL - [Non-matching text with element-id fallback] + [Navigate cross-origin iframe via window.location] expected: FAIL diff --git a/testing/web-platform/meta/scroll-to-text-fragment/percent-encoding.html.ini b/testing/web-platform/meta/scroll-to-text-fragment/percent-encoding.html.ini index 4b5642524607..9e951e30e786 100644 --- a/testing/web-platform/meta/scroll-to-text-fragment/percent-encoding.html.ini +++ b/testing/web-platform/meta/scroll-to-text-fragment/percent-encoding.html.ini @@ -1,18 +1,3 @@ [percent-encoding.html] - [Test navigation with fragment: Percent char without hex digits is invalid..] - expected: FAIL - - [Test navigation with fragment: Percent char followed by percent char is invalid..] - expected: FAIL - - [Test navigation with fragment: Single digit percent-encoding is invalid..] - expected: FAIL - - [Test navigation with fragment: Percent-encoding limited to two digits..] - expected: FAIL - [Test navigation with fragment: Percent-encoded "%%F".] expected: FAIL - - [Test navigation with fragment: Percent-encoding multibyte codepoint (CHECKMARK)..] - expected: FAIL diff --git a/testing/web-platform/meta/scroll-to-text-fragment/same-document-tests.html.ini b/testing/web-platform/meta/scroll-to-text-fragment/same-document-tests.html.ini deleted file mode 100644 index c277558b1b6c..000000000000 --- a/testing/web-platform/meta/scroll-to-text-fragment/same-document-tests.html.ini +++ /dev/null @@ -1,8 +0,0 @@ -[same-document-tests.html] - expected: - [OK, TIMEOUT] - [Basic element id fallback] - expected: FAIL - - [Malformed text directive element id fallback] - expected: [FAIL, TIMEOUT] diff --git a/testing/web-platform/meta/scroll-to-text-fragment/scroll-to-text-fragment-same-doc.html.ini b/testing/web-platform/meta/scroll-to-text-fragment/scroll-to-text-fragment-same-doc.html.ini deleted file mode 100644 index a2c6bcb3aaeb..000000000000 --- a/testing/web-platform/meta/scroll-to-text-fragment/scroll-to-text-fragment-same-doc.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[scroll-to-text-fragment-same-doc.html] - [Activated for same-document window.location.replace] - expected: FAIL diff --git a/testing/web-platform/tests/scroll-to-text-fragment/same-document-test-sync-load.html b/testing/web-platform/tests/scroll-to-text-fragment/same-document-test-sync-load.html new file mode 100644 index 000000000000..7eb1bf550591 --- /dev/null +++ b/testing/web-platform/tests/scroll-to-text-fragment/same-document-test-sync-load.html @@ -0,0 +1,27 @@ + +Same document navigation to text fragment directives before loading the document has finished + + + + + + + + +
+ This is a line of text. +