[Blazor] Allow `<FocusOnNavigate>` to work when rendered statically #57131

MackinnonBuck · 2024-08-01T23:43:44Z

[Blazor] Allow `<FocusOnNavigate>` to work when rendered statically

Updates the implementation of <FocusOnNavigate> to render a custom element when rendered statically.

Description

The behavior can be summarized as follows:

After each enhanced navigation with a GET method, focus the first element matching the specified selector.
- The element may appear in a streaming update, but we're only allowed to move focus once per enhanced navigation
  - i.e., even if another element matching the selector becomes visible, don't move focus again
- If the focus changes between the start of the navigation and the matching element getting added to the page (e.g., the user focused an input and started typing in it), don't programmatically change the focus again
If the programmatically focused element gets removed from the page, apply focus to another element matching the current selector, if it exists.

Fixes #52412
Addresses #56287 (but does not close it)
Possibly addresses #53887

MackinnonBuck · 2024-08-05T23:17:21Z

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

+    enhancedNavigationStarted: (resource, options) => {
+      jsEventRegistry.dispatchEvent('enhancednavigationstart', { resource, options });
+    },


This new event isn't strictly necessary - we could create internal hooks to enable FocusOnNavigate functionality.

However, it's pretty cheap to add, it helps with the implementation of FocusOnNavigate, and we've gotten some 👍s on #53887 from the community, indicating that this would be useful.

That said, I don't want to start a whole separate design discussion around this, especially since it's late in the release so we need to be wary about new features. So, if there's any objection to adding this here, I'm happy to remove it 🙂

MackinnonBuck · 2024-08-05T23:27:03Z

src/Components/Web.JS/src/Rendering/FocusOnNavigate.ts

+class FocusOnNavigateElement extends HTMLElement {
+  connectedCallback() {
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
+    currentFocusOnNavigateElement = this;
+  }
+
+  disconnectedCallback() {
+    if (currentFocusOnNavigateElement === this) {
+      currentFocusOnNavigateElement = null;
+    }
+  }
+}


In a previous iteration, I had a slightly more sophisticated implementation that gracefully handled having multiple <FocusOnNavigate> components at the same time, which can be later removed from the page in an arbitrary order without breaking things. However, I realized that's unlikely to be a real scenario, because <FocusOnNavigate> is almost always going to exist in one location (in the router), especially considering it requires a RouteData parameter that you can only obtain from the <Router> (unless you construct it yourself, which I don't think is common).

This simplified implementation always prioritizes the last-rendered <blazor-focus-on-navigate> custom element and assumes that if one already exists, it's going to be disconnected soon because we're in the process of applying updates to the DOM.

If others disagree, I'm happy to add the old logic back. It's really not hugely complicated and I'm confident that it works, but it's just more code that apps need to download (and that we need to maintain).

We could choose to throw if we find more than one instance of focus on navigate on the document on the server even if we wanted.

It's not even clear what the behavior is when many of them are available on the page interactively.

TBH I'd be happy with avoiding the need for extra checks/errors and just go with a "first one in the document wins" rule. It would be the same as the HTML autofocus attribute:

That's fine for me, given that there's defined behavior.

We should also consider interactions between FocusOnNavigate and autofocus now that you mention it.

I've updated the implementation to defer to autofocus on the initial page load. That is, autofocus takes precedence over FocusOnNavigate.

That said, autofocus is not a replacement for FocusOnNavigate, because after the first element gets autofocused, no other elements can get autofocused until a full page reload. In other words, autofocus does not work with enhanced nav.

Sounds reasonable.

javiercn · 2024-08-06T17:41:08Z

src/Components/Web.JS/dist/Release/blazor.webassembly.js

Why is this being checked-in

Probably due to the changes in DomWrapper.ts and JSEventRegistry.ts, which are used in WebAssembly

blazor.webassembly.js shouldn't be checked-in, hence why I was asking. We only do server/web/webview.

src/Components/Web.JS/src/DomWrapper.ts

MackinnonBuck · 2024-08-06T23:34:37Z

src/Components/Web.JS/src/Rendering/FocusOnNavigate.ts

+  if (target instanceof Element && target === lastFocusedElement) {
+    // We want to apply focus after all synchronous changes to the DOM have completed,
+    // including the potential removal of this element.
+    setTimeout(() => {
+      if (!document.contains(target)) {
+        tryApplyFocus(/* forceMoveFocus */ false);
+      }
+    }, 0);


I've done some more thorough testing and found that this isn't totally sufficient:

It works on Chrome and Edge, but not Firefox (the element doesn't get refocused)

Streaming updates may remove the tabindex attribute from an element that isn't otherwise focusable (like a header), and that removal unfocuses the element. We could re-apply the attribute and focus it again here, but that has additional problems:

This causes screen readers to re-announce the element that got focused, which can be jarring

That also doesn't work on Firefox 🙂

Ideally, we would prevent the element from losing focus in the first place, when possible. I'm considering an alternative approach where we don't attempt to move focus until streaming has completed and all components have become interactive (and produced their first render). This allows us to avoid attempting to reason about what should happen if the focused element gets removed from the page. It also aligns with the behavior of an interactive <FocusOnNavigate> component a bit more.

Could you clarify what scenario this addresses? Is it for the edge case where, in a sequence of streaming updates, one of them adds the desired element (and we focus it), but then a subsequent streaming update removes that element and replaces it with a different one matching the selector?

If that's the only case where this matters, is it possible we could just not do anything? As in, we focus the first one that matches during a given navigation and that's it. Developers typically want to use this feature to announce page titles (e.g., <h1> elems), and they won't typically be removing and re-adding page titles except in the edge case where the title text changes, and in that case there are other better ways to do it for example with aria-live which will announce the header text update in a way that makes sense.

Perhaps there is some other critical reason we have to handle this, but it's not obvious from the code comment. Apologies if I'm missing something specified elsewhere.

BTW great job on the testing/verification here. From your remarks I see you must have tried this with a range of browsers and at least some screen reader.

Oh wait I think I now understand the problem:

First streaming update adds a <h1> (say), the FocusOnNavigate code has to add a tabindex to make it focusable, and then reader begins to announce it.

Second streaming update removes the tabindex because it's not present in the original output, and the reader's announcement is truncated.

Is that right?

If so I think your idea to simply wait until enhanced nav is finished sound like the best option. That also deals with the case where the header text changes during the streaming page load. Example:

@attribute [StreamRendering] @if (product is null) { <h1>Loading...</h1> } else { <h1>@product.Name</h1> }

In this case we would want to announce @product.Name and not Loading.... Getting sensible announcements without relying on the developer knowing about aria-live would be a benefit.

I agree, it'll be best if we wait for the entire enhanced nav to finish before we try to focus the element. The contract in that scenario is simple, your element must be on the page after the navigation has completed. And it saves us from having to deal with all the complexity of elements being added or removed during the streaming rendering process.

I updated the implementation to wait until an enhanced navigation completes before applying focus. However, I decided not to handle the case where a focused element gets removed upon the transition to interactivity. I started to work on this, but realized it adds quite a bit of complexity (we need to keep track of all SSR'd components currently on the page that have not transitioned to interactivity, then the interactive renderer needs to indicate somehow when each component completes its first render...).

The ultimate fix would be to not replace the DOM in the transition to interactivity (and we do have an issue tracking that). Then we wouldn't have to worry about this problem at all.

SteveSandersonMS · 2024-08-07T09:45:27Z

src/Components/Web.JS/src/Rendering/FocusOnNavigate.ts

+
+function onEnhancedNavigationStart(ev: EnhancedNavigationStartEvent) {
+  // Only focus on enhanced load if the enhanced navigation is not a form post.
+  allowFocusOnEnhancedLoad = ev.options.method !== 'post';


This rule could use some clarification in the comment. My guess is that you're trying to differentiate "navigating to a new place" (should change focus) vs "updating the existing page" (should not).

There's a slightly related rule in NavigationEnhancement.ts where we check isForSamePath to determine whether we think the new URL is conceptually equivalent to the old one - it compares the URL up to, but excluding, the querystring. Arguably there would be some benefit to following the same rule here since that would give us fewer new rules to explain/justify, and is arguably more general than doing it on HTTP method alone. There are certainly cases with an enhanced GET request where the UI is conceptually just updating and not going to a new page, e.g., pagination links on a grid, just as there are cases where a POST conceptually goes to a new page.

But altogether I guess this is a subjective call to make. I'm fine with whatever subjective choice you prefer here. If you could slightly expand the code comment to specify the rationale for whatever rule you prefer, I think that's totally sufficient.

I think is better if we are consistent with what we do in NavigationEnhancement.ts

This has been updated to reuse the same logic in NavigationEnhancement.ts

javiercn · 2024-08-07T13:39:27Z

src/Components/Web.JS/src/Rendering/FocusOnNavigate.ts

+    // There may be streaming updates on the initial page load, so we want to allow
+    // those to add elements matching the active selector.
+    allowFocusOnEnhancedLoad = true;
+    document.addEventListener('DOMContentLoaded', afterInitialPageLoad);


Not sure that it matters, but might be worth passing { once: true } (see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#options)

src/Components/Web.JS/src/Rendering/FocusOnNavigate.ts

javiercn

Looks great, and I appreciate all the work you have done to thoroughly test the implementation.

My biggest question is whether it is worth it to handle streaming rendering scenarios or just wait until the enhanced navigation has finished to change the focus.

I believe the latter approach will simplify the implementation and will align it with what we do in other areas of the framework where we require the element to be present after streaming rendering has completed (for example for dispatching forms).

I can't think of a scenario where we want to focus on the middle of streaming rendering, given how jarring the experience can be, and I'm also a bit concerned about streaming rendering and accessibility in general, as it has the potential to make the screen reader experience problematic.

That said, I think this is a separate concern that we should analyze and handle separately.

For what respects to FocusOnNavigate I think just waiting would save us some trouble.

We should also consider how this interacts with autofocus

javiercn

Looks great!

ghost added the area-blazor Includes: Blazor, Razor Components label Aug 1, 2024

MackinnonBuck commented Aug 5, 2024

View reviewed changes

MackinnonBuck marked this pull request as ready for review August 5, 2024 23:27

MackinnonBuck requested a review from a team as a code owner August 5, 2024 23:27

MackinnonBuck requested review from javiercn and halter73 August 5, 2024 23:28

MackinnonBuck added 5 commits August 5, 2024 16:39

Support <FocusOnNavigate> in SSR, new events

e628615

Clean up, add tests

427f2a4

Simplify handling of multiple selectors

cd3bcf7

Update enhancednavigationstart event

fe9f08a

Update blazor.*.js

8759ede

MackinnonBuck force-pushed the mbuck/fix-focus-on-navigate branch from 92acc12 to 8759ede Compare August 5, 2024 23:41

MackinnonBuck requested a review from SteveSandersonMS August 6, 2024 16:37

javiercn reviewed Aug 6, 2024

View reviewed changes

src/Components/Web.JS/src/DomWrapper.ts Outdated Show resolved Hide resolved

MackinnonBuck commented Aug 6, 2024

View reviewed changes

SteveSandersonMS reviewed Aug 7, 2024

View reviewed changes

javiercn reviewed Aug 7, 2024

View reviewed changes

src/Components/Web.JS/src/Rendering/FocusOnNavigate.ts Outdated Show resolved Hide resolved

javiercn reviewed Aug 7, 2024

View reviewed changes

MackinnonBuck added 2 commits August 7, 2024 15:01

Simlified to wait until streaming completes

699b9d3

Respect autofocus on the initial page load

8075b2e

MackinnonBuck requested review from SteveSandersonMS and javiercn August 7, 2024 22:38

javiercn approved these changes Aug 8, 2024

View reviewed changes

MackinnonBuck merged commit 43d0754 into main Aug 8, 2024
26 checks passed

MackinnonBuck deleted the mbuck/fix-focus-on-navigate branch August 8, 2024 16:29

dotnet-policy-service bot added this to the 9.0-rc1 milestone Aug 8, 2024

MackinnonBuck mentioned this pull request Aug 8, 2024

Blazor enhanced navigation "before" javascript event #53887

Closed

1 task

[Blazor] Allow <FocusOnNavigate> to work when rendered statically #57131

[Blazor] Allow <FocusOnNavigate> to work when rendered statically #57131

Uh oh!

Conversation

MackinnonBuck commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[Blazor] Allow <FocusOnNavigate> to work when rendered statically

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

javiercn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[Blazor] Allow `<FocusOnNavigate>` to work when rendered statically #57131

[Blazor] Allow `<FocusOnNavigate>` to work when rendered statically #57131

MackinnonBuck commented Aug 1, 2024 •

edited

Loading

[Blazor] Allow `<FocusOnNavigate>` to work when rendered statically

SteveSandersonMS Aug 7, 2024 •

edited

Loading

SteveSandersonMS Aug 7, 2024 •

edited

Loading

javiercn left a comment •

edited

Loading