Skip to content

Commit 48b9449

Browse files
ilonatommyilonatommy
authored
Enhanced navigation should not scroll before content renders (#64054)
* Fix. * Add test that tracks scroll position in relation to navigation.
1 parent 687acb2 commit 48b9449

File tree

8 files changed

+397
-77
lines changed

8 files changed

+397
-77
lines changed

‎src/Components/Web.JS/src/Boot.Web.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { shouldAutoStart } from './BootCommon';
1515
import { Blazor } from './GlobalExports';
1616
import { WebStartOptions } from './Platform/WebStartOptions';
1717
import { attachStreamingRenderingListener } from './Rendering/StreamingRendering';
18+
import { resetScrollIfNeeded, ScrollResetSchedule } from './Rendering/Renderer';
1819
import { NavigationEnhancementCallbacks, attachProgressivelyEnhancedNavigationListener } from './Services/NavigationEnhancement';
1920
import { WebRootComponentManager } from './Services/WebRootComponentManager';
2021
import { hasProgrammaticEnhancedNavigationHandler, performProgrammaticEnhancedNavigation } from './Services/NavigationUtils';
@@ -57,6 +58,7 @@ function boot(options?: Partial<WebStartOptions>) : Promise<void> {
5758
},
5859
documentUpdated: () => {
5960
rootComponentManager.onDocumentUpdated();
61+
resetScrollIfNeeded(ScrollResetSchedule.AfterDocumentUpdate);
6062
jsEventRegistry.dispatchEvent('enhancedload', {});
6163
},
6264
enhancedNavigationCompleted() {

‎src/Components/Web.JS/src/Rendering/Renderer.ts‎

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,15 @@ import { getAndRemovePendingRootComponentContainer } from './JSRootComponents';
1111
interface BrowserRendererRegistry {
1212
[browserRendererId: number]: BrowserRenderer;
1313
}
14+
15+
export enum ScrollResetSchedule {
16+
None,
17+
AfterBatch, // Reset scroll after interactive components finish rendering (interactive navigation)
18+
AfterDocumentUpdate, // Reset scroll after enhanced navigation updates the DOM (enhanced navigation)
19+
}
20+
1421
const browserRenderers: BrowserRendererRegistry = {};
15-
let shouldResetScrollAfterNextBatch = false;
22+
let pendingScrollResetTiming: ScrollResetSchedule = ScrollResetSchedule.None;
1623

1724
export function attachRootComponentToLogicalElement(browserRendererId: number, logicalElement: LogicalElement, componentId: number, appendContent: boolean): void {
1825
let browserRenderer = browserRenderers[browserRendererId];
@@ -88,19 +95,28 @@ export function renderBatch(browserRendererId: number, batch: RenderBatch): void
8895
browserRenderer.disposeEventHandler(eventHandlerId);
8996
}
9097

91-
resetScrollIfNeeded();
98+
resetScrollIfNeeded(ScrollResetSchedule.AfterBatch);
9299
}
93100

94-
export function resetScrollAfterNextBatch(): void {
95-
shouldResetScrollAfterNextBatch = true;
96-
}
101+
export function scheduleScrollReset(timing: ScrollResetSchedule): void {
102+
if (timing !== ScrollResetSchedule.AfterBatch) {
103+
pendingScrollResetTiming = timing;
104+
return;
105+
}
97106

98-
export function resetScrollIfNeeded() {
99-
if (shouldResetScrollAfterNextBatch) {
100-
shouldResetScrollAfterNextBatch = false;
107+
if (pendingScrollResetTiming !== ScrollResetSchedule.AfterDocumentUpdate) {
108+
pendingScrollResetTiming = ScrollResetSchedule.AfterBatch;
109+
}
110+
}
101111

102-
// This assumes the scroller is on the window itself. There isn't a general way to know
103-
// if some other element is playing the role of the primary scroll region.
104-
window.scrollTo && window.scrollTo(0, 0);
112+
export function resetScrollIfNeeded(triggerTiming: ScrollResetSchedule) {
113+
if (pendingScrollResetTiming !== triggerTiming) {
114+
return;
105115
}
116+
117+
pendingScrollResetTiming = ScrollResetSchedule.None;
118+
119+
// This assumes the scroller is on the window itself. There isn't a general way to know
120+
// if some other element is playing the role of the primary scroll region.
121+
window.scrollTo && window.scrollTo(0, 0);
106122
}

‎src/Components/Web.JS/src/Services/NavigationEnhancement.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import { synchronizeDomContent } from '../Rendering/DomMerging/DomSync';
55
import { attachProgrammaticEnhancedNavigationHandler, handleClickForNavigationInterception, hasInteractiveRouter, isForSamePath, notifyEnhancedNavigationListeners, performScrollToElementOnTheSamePage, isSamePageWithHash } from './NavigationUtils';
6-
import { resetScrollAfterNextBatch, resetScrollIfNeeded } from '../Rendering/Renderer';
6+
import { scheduleScrollReset, ScrollResetSchedule } from '../Rendering/Renderer';
77

88
/*
99
In effect, we have two separate client-side navigation mechanisms:
@@ -81,7 +81,7 @@ function performProgrammaticEnhancedNavigation(absoluteInternalHref: string, rep
8181
}
8282

8383
if (!isForSamePath(absoluteInternalHref, originalLocation)) {
84-
resetScrollAfterNextBatch();
84+
scheduleScrollReset(ScrollResetSchedule.AfterDocumentUpdate);
8585
}
8686

8787
performEnhancedPageLoad(absoluteInternalHref, /* interceptedLink */ false);
@@ -108,8 +108,7 @@ function onDocumentClick(event: MouseEvent) {
108108
let isSelfNavigation = isForSamePath(absoluteInternalHref, originalLocation);
109109
performEnhancedPageLoad(absoluteInternalHref, /* interceptedLink */ true);
110110
if (!isSelfNavigation) {
111-
resetScrollAfterNextBatch();
112-
resetScrollIfNeeded();
111+
scheduleScrollReset(ScrollResetSchedule.AfterDocumentUpdate);
113112
}
114113
}
115114
});

‎src/Components/Web.JS/src/Services/NavigationManager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
import '@microsoft/dotnet-js-interop';
5-
import { resetScrollAfterNextBatch } from '../Rendering/Renderer';
5+
import { scheduleScrollReset, ScrollResetSchedule } from '../Rendering/Renderer';
66
import { EventDelegator } from '../Rendering/Events/EventDelegator';
77
import { attachEnhancedNavigationListener, getInteractiveRouterRendererId, handleClickForNavigationInterception, hasInteractiveRouter, hasProgrammaticEnhancedNavigationHandler, isForSamePath, isSamePageWithHash, isWithinBaseUriSpace, performProgrammaticEnhancedNavigation, performScrollToElementOnTheSamePage, scrollToElement, setHasInteractiveRouter, toAbsoluteUri } from './NavigationUtils';
88
import { WebRendererId } from '../Rendering/WebRendererId';
@@ -170,7 +170,7 @@ async function performInternalNavigation(absoluteInternalHref: string, intercept
170170
// To avoid ugly flickering effects, we don't want to change the scroll position until
171171
// we render the new page. As a best approximation, wait until the next batch.
172172
if (!isForSamePath(absoluteInternalHref, location.href)) {
173-
resetScrollAfterNextBatch();
173+
scheduleScrollReset(ScrollResetSchedule.AfterBatch);
174174
}
175175

176176
saveToBrowserHistory(absoluteInternalHref, replace, state);

‎src/Components/test/E2ETest/Infrastructure/ScrollOverrideScope.cs‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Globalization;
7+
using System.Linq;
8+
using OpenQA.Selenium;
9+
using Xunit.Sdk;
10+
11+
namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure;
12+
13+
internal sealed class ScrollOverrideScope : IDisposable
14+
{
15+
private readonly IJavaScriptExecutor _executor;
16+
private readonly bool _isActive;
17+
18+
public ScrollOverrideScope(IWebDriver browser, bool isActive)
19+
{
20+
_executor = (IJavaScriptExecutor)browser;
21+
_isActive = isActive;
22+
23+
if (!_isActive)
24+
{
25+
return;
26+
}
27+
28+
_executor.ExecuteScript(@"
29+
(function() {
30+
if (window.__enhancedNavScrollOverride) {
31+
if (window.__clearEnhancedNavScrollLog) {
32+
window.__clearEnhancedNavScrollLog();
33+
}
34+
return;
35+
}
36+
37+
const original = window.scrollTo.bind(window);
38+
const log = [];
39+
40+
function resolvePage() {
41+
const landing = document.getElementById('test-info-1');
42+
if (landing && landing.textContent === 'Scroll tests landing page') {
43+
return 'landing';
44+
}
45+
46+
const next = document.getElementById('test-info-2');
47+
if (next && next.textContent === 'Scroll tests next page') {
48+
return 'next';
49+
}
50+
51+
return 'other';
52+
}
53+
54+
window.__enhancedNavScrollOverride = true;
55+
window.__enhancedNavOriginalScrollTo = original;
56+
window.__enhancedNavScrollLog = log;
57+
window.__clearEnhancedNavScrollLog = () => { log.length = 0; };
58+
window.__drainEnhancedNavScrollLog = () => {
59+
const copy = log.slice();
60+
log.length = 0;
61+
return copy;
62+
};
63+
64+
window.scrollTo = function(...args) {
65+
log.push({
66+
page: resolvePage(),
67+
url: location.href,
68+
time: performance.now(),
69+
args
70+
});
71+
72+
return original(...args);
73+
};
74+
})();
75+
");
76+
77+
ClearLog();
78+
}
79+
80+
public void ClearLog()
81+
{
82+
if (!_isActive)
83+
{
84+
return;
85+
}
86+
87+
_executor.ExecuteScript("if (window.__clearEnhancedNavScrollLog) { window.__clearEnhancedNavScrollLog(); }");
88+
}
89+
90+
public void AssertNoPrematureScroll(string expectedPage, string navigationDescription)
91+
{
92+
if (!_isActive)
93+
{
94+
return;
95+
}
96+
97+
var entries = DrainLog();
98+
if (entries.Length == 0)
99+
{
100+
return;
101+
}
102+
103+
var unexpectedEntries = entries
104+
.Where(entry => !string.Equals(entry.Page, expectedPage, StringComparison.Ordinal))
105+
.ToArray();
106+
107+
if (unexpectedEntries.Length == 0)
108+
{
109+
return;
110+
}
111+
112+
var details = string.Join(
113+
", ",
114+
unexpectedEntries.Select(entry => $"page={entry.Page ?? "null"} url={entry.Url} time={entry.Time:F2}"));
115+
116+
throw new XunitException($"Detected a scroll reset while the DOM still displayed '{unexpectedEntries[0].Page ?? "unknown"}' during {navigationDescription}. Entries: {details}");
117+
}
118+
119+
private ScrollInvocation[] DrainLog()
120+
{
121+
if (!_isActive)
122+
{
123+
return Array.Empty<ScrollInvocation>();
124+
}
125+
126+
var result = _executor.ExecuteScript("return window.__drainEnhancedNavScrollLog ? window.__drainEnhancedNavScrollLog() : [];");
127+
if (result is not IReadOnlyList<object> entries || entries.Count == 0)
128+
{
129+
return Array.Empty<ScrollInvocation>();
130+
}
131+
132+
var resolved = new ScrollInvocation[entries.Count];
133+
for (var i = 0; i < entries.Count; i++)
134+
{
135+
if (entries[i] is IReadOnlyDictionary<string, object> dict)
136+
{
137+
dict.TryGetValue("page", out var pageValue);
138+
dict.TryGetValue("url", out var urlValue);
139+
dict.TryGetValue("time", out var timeValue);
140+
141+
resolved[i] = new ScrollInvocation(
142+
pageValue as string,
143+
urlValue as string,
144+
timeValue is null ? 0D : Convert.ToDouble(timeValue, CultureInfo.InvariantCulture));
145+
continue;
146+
}
147+
148+
resolved[i] = new ScrollInvocation(null, null, 0D);
149+
}
150+
151+
return resolved;
152+
}
153+
154+
public void Dispose()
155+
{
156+
if (!_isActive)
157+
{
158+
return;
159+
}
160+
161+
_executor.ExecuteScript(@"
162+
(function() {
163+
if (!window.__enhancedNavScrollOverride) {
164+
return;
165+
}
166+
167+
if (window.__enhancedNavOriginalScrollTo) {
168+
window.scrollTo = window.__enhancedNavOriginalScrollTo;
169+
delete window.__enhancedNavOriginalScrollTo;
170+
}
171+
172+
delete window.__enhancedNavScrollOverride;
173+
delete window.__enhancedNavScrollLog;
174+
delete window.__clearEnhancedNavScrollLog;
175+
delete window.__drainEnhancedNavScrollLog;
176+
})();
177+
");
178+
}
179+
180+
private readonly record struct ScrollInvocation(string Page, string Url, double Time);
181+
}

‎src/Components/test/E2ETest/Infrastructure/WebDriverExtensions/WebDriverExtensions.cs‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System;
5+
using System.Threading;
46
using OpenQA.Selenium;
57
using OpenQA.Selenium.Support.UI;
8+
using Microsoft.AspNetCore.Components.E2ETests.ServerRenderingTests;
69

710
namespace Microsoft.AspNetCore.Components.E2ETest;
811

@@ -64,4 +67,78 @@ public static long GetElementPositionWithRetry(this IWebDriver browser, string e
6467

6568
throw new Exception($"Failed to get position for element '{elementId}' after {retryCount} retries. Debug log: {log}");
6669
}
70+
71+
internal static ScrollObservation BeginScrollObservation(this IWebDriver browser, IWebElement element, Func<IWebDriver, bool> domMutationPredicate)
72+
{
73+
ArgumentNullException.ThrowIfNull(browser);
74+
ArgumentNullException.ThrowIfNull(element);
75+
ArgumentNullException.ThrowIfNull(domMutationPredicate);
76+
77+
var initialScrollPosition = browser.GetScrollY();
78+
return new ScrollObservation(element, initialScrollPosition, domMutationPredicate);
79+
}
80+
81+
internal static ScrollObservationResult WaitForStaleDomOrScrollChange(this IWebDriver browser, ScrollObservation observation, TimeSpan? timeout = null, TimeSpan? pollingInterval = null)
82+
{
83+
ArgumentNullException.ThrowIfNull(browser);
84+
85+
var wait = new DefaultWait<IWebDriver>(browser)
86+
{
87+
Timeout = timeout ?? TimeSpan.FromSeconds(10),
88+
PollingInterval = pollingInterval ?? TimeSpan.FromMilliseconds(50),
89+
};
90+
wait.IgnoreExceptionTypes(typeof(InvalidOperationException));
91+
92+
ScrollObservationOutcome? detectedOutcome = null;
93+
wait.Until(driver =>
94+
{
95+
if (observation.DomMutationPredicate(driver))
96+
{
97+
detectedOutcome = ScrollObservationOutcome.DomUpdated;
98+
return true;
99+
}
100+
101+
if (observation.Element.IsStale())
102+
{
103+
detectedOutcome = ScrollObservationOutcome.DomUpdated;
104+
return true;
105+
}
106+
107+
if (browser.GetScrollY() != observation.InitialScrollPosition)
108+
{
109+
detectedOutcome = ScrollObservationOutcome.ScrollChanged;
110+
return true;
111+
}
112+
113+
return false;
114+
});
115+
116+
var outcome = detectedOutcome ?? ScrollObservationOutcome.DomUpdated;
117+
118+
var finalScrollPosition = browser.GetScrollY();
119+
return new ScrollObservationResult(outcome, observation.InitialScrollPosition, finalScrollPosition);
120+
}
121+
122+
internal static bool IsStale(this IWebElement element)
123+
{
124+
try
125+
{
126+
_ = element.Enabled;
127+
return false;
128+
}
129+
catch (StaleElementReferenceException)
130+
{
131+
return true;
132+
}
133+
}
134+
}
135+
136+
internal readonly record struct ScrollObservation(IWebElement Element, long InitialScrollPosition, Func<IWebDriver, bool> DomMutationPredicate);
137+
138+
internal readonly record struct ScrollObservationResult(ScrollObservationOutcome Outcome, long InitialScrollPosition, long FinalScrollPosition);
139+
140+
internal enum ScrollObservationOutcome
141+
{
142+
ScrollChanged,
143+
DomUpdated,
67144
}

0 commit comments

Comments
 (0)