Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explainer for focus management and scroll restoration #197

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Jan 7, 2022

This also rearranges some of the content around accessibility technology announcements and loading spinners/stop buttons, grouping them all under a new section "Customizations and consequences of navigation interception".

Still a few TODOs for next week, thus leaving this in a draft state. But the plan is for this PR to address #187, #190, and #25.


Preview | Diff

@domenic domenic marked this pull request as ready for review January 10, 2022 20:20
@domenic
Copy link
Collaborator Author

domenic commented Jan 10, 2022

Hmm, transitionWhile() being the place to set these options runs into unclear semantics if you set it twice:

appHistory.addEventListener("navigate", e => {
  e.transitionWhile(Promise.resolve(), { focusReset: "manual" });
  e.transitionWhile(Promise.resolve(), { focusReset: "after-transition" });
});

// A more realistic example might involve multiple navigate handlers...

Options I see...

  • Require the options to always match, throwing an exception on callers after the first if they mismatch. (Not great for apps trying to use multiple transitionWhile() calls or navigate event handlers in a non-coordinated fashion.)

  • Try to come up with a conflict-resolution procedure. E.g.:

    • First wins (with later values ignored)
    • Last wins (with previous values ignored)
    • if "manual" is ever specified (i.e., not the default), it wins over "after-transition". (Gets complicated if we ever expand the value space like the explainer indicates we might.)
    • Any of these could include console warnings, I guess.
  • Abandon the idea of tying these options to transitionWhile() calls? E.g. navigateEvent.setOptions(...)?

    • It's not clear this really solves the problem, since you could still call that method twice. But it might give us flexibility to pick one of the above strategies in a way that's less confusing.
    • This is a bit less appealing because these options generally only make sense in the transitionWhile() case anyway... we'd basically duplicate all of the precondition logic.
    • Maybe something global, like appHistory.setTransitionOptions(...) or appHistory.focusReset = ..., appHistory.scrollRestoration = ...?

@domenic
Copy link
Collaborator Author

domenic commented Jan 12, 2022

I think the primary question is: do we expect people to want the same values for focusReset / scrollRestoration for all app history-mediated navigations in their app? Or might it vary depending on the navigation in question?

If we expect it to be the same, then a global API makes sense. Otherwise, it'd need to be configured per-navigate event.

README.md Outdated Show resolved Hide resolved
README.md Outdated

- Per the above-linked [research by Fable Tech Labs](https://www.gatsbyjs.com/blog/2019-07-11-user-testing-accessible-client-routing/), screen reader users generally prefer focus to be reset to a heading or wrapper element, instead of the `<body>` element. So to get the optimal experience with app history interception, developers should use `autofocus=""` appropriately on such elements.

- For traversals (i.e. cases where `navigateEvent.navigationType === "traverse"`), getting parity with the [back/forward cache experience](https://web.dev/bfcache/) requires restoring focus to the same element that was previously focused when that history entry was active. Unfortunately, this isn't something the browser can do automatically for client-side rendered applications; the notion of "the same element" [is not generally stable](https://github.com/WICG/app-history/issues/190#:~:text=On%20the%20other%20hand%2C%20in%20the%20general%20case%20we%20won%27t%20be%20able%20to%20identify%20%22the%20same%20element%22!) in such cases. So for such cases, using `focusReset: "manual"`, storing some identifier for the currently-focused element in the app history state, and calling `element.focus()` appropriately upon transition could give a better experience.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment here when it would be appropriate to call element.focus() after transition? Would you recommend using the transition property, or in a currentchange listener, after the promise resolves, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any of appHistory.transition.finished.then(updateFocus), appHistory.back().then(updateFocus) or appHistory.addEventListener("navigatesuccess", updateFocus) would work; they have the same timing.

If you wanted to update focus even on failure then you'd do appHistory.transition.finished.finally(updateFocus) or appHistory.back().finally(updateFocus). For the event version you'd add handlers for both navigatesuccess and navigateerror.

You could also integrate it into your specific application logic, e.g.

appHistory.addEventListener("navigate", e => {
  e.transitionWhile((async () => {
    await updateDOMWithSkeleton();
    findElement().focus();
    await updateTheRestOfTheDOM();
  })());
});

I'll mention the basics here in the PR.

README.md Outdated

Currently the browser provides two options: performing scroll restoration automatically, or disabling it entirely with `history.scrollRestoration = "manual"`. App history gives us an opportunity to provide some intermediate options to developers, at least for the case of same-document transitions. We do this via another option to `transitionWhile()`:

- `e.transitionWhile(promise, { scrollRestoration: "after-transition" })`: the default behavior. The browser delays its scroll restoration logic until `promise` fulfills; it will perform no scroll restoration if the promise rejects. If the user has scrolled during the transition then no scroll restoration will be performed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to cancel scroll behavior if the user scrolls during the transition? I feel like the current behavior in a multi-page application is to disregard user scrolls while the page is navigating?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd tend to agree: user scroll should be ignored. But this rule could apply to manual calls to window.scrollTo and similar APIs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Chromium engineers pointed me to code which abandons scroll restoration (for MPA navs) when the user scrolls. I will do some black-box testing to confirm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example:

  1. Open https://neat-equal-cent.glitch.me/, preferably in a vertically-small-ish window
  2. Wait for, e.g., paragraph 50 to load, and scroll down to it
  3. Click the example.com link
  4. Click back. Do not touch anything.
  5. After paragraph 50 loads, scroll position will be restored.

Then try it this way:

  1. Click back. Once a scrollbar appears, scroll slightly.
  2. Even after paragraph 50 loads, your scroll position will not be disturbed. You having scrolled will have changed how it works.

README.md Show resolved Hide resolved

We could also add the following APIs in the future, but we are currently not planning on including them until we hear developer feedback that they'd be helpful:

- `e.transitionWhile(promise, { scrollRestoration: "immediate" })`: the browser performs its usual scroll restoration logic, but does so immediately instead of waiting for `promise`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equivalent to calling restoreScroll immediately though, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

We could also add the following APIs in the future, but we are currently not planning on including them until we hear developer feedback that they'd be helpful:

- `e.transitionWhile(promise, { scrollRestoration: "immediate" })`: the browser performs its usual scroll restoration logic, but does so immediately instead of waiting for `promise`.
- `e.transitionWhile(promise, { scrollRestoration: "auto" })`: the browser performs its usual scroll restoration logic, at its usual indeterminate time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would anyone want this? Sounds dubious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's why it's in the unclear-utility section :).

- `e.transitionWhile(promise, { focusReset: "immediate" })`: immediately resets the focus to the `<body>` element, without waiting for the promise to settle.
- `e.transitionWhile(promise, { focusReset: "two-stage" })`: immediately resets the focus to the `<body>` element, and then has the same behavior as `"after-transition"`.

#### Scroll position restoration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering about how there's no ability to influence WHERE the browser scrolls. This just provides an API for WHEN the browser restores scroll position. is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. You can use window.scrollTo() + scrollRestoration: "manual" for manual scrolling. I can mention that...

README.md Outdated

- Per the above-linked [research by Fable Tech Labs](https://www.gatsbyjs.com/blog/2019-07-11-user-testing-accessible-client-routing/), screen reader users generally prefer focus to be reset to a heading or wrapper element, instead of the `<body>` element. So to get the optimal experience with app history interception, developers should use `autofocus=""` appropriately on such elements.

- For traversals (i.e. cases where `navigateEvent.navigationType === "traverse"`), getting parity with the [back/forward cache experience](https://web.dev/bfcache/) requires restoring focus to the same element that was previously focused when that history entry was active. Unfortunately, this isn't something the browser can do automatically for client-side rendered applications; the notion of "the same element" [is not generally stable](https://github.com/WICG/app-history/issues/190#:~:text=On%20the%20other%20hand%2C%20in%20the%20general%20case%20we%20won%27t%20be%20able%20to%20identify%20%22the%20same%20element%22!) in such cases. So for such cases, using `focusReset: "manual"`, storing some identifier for the currently-focused element in the app history state, and calling `element.focus()` appropriately upon transition could give a better experience.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO heuristics with (1) the input object instance (if it wasn't removed from DOM), and (2) ID should be a good start. Are there any other similar restoration rules, e.g. from the current BFCache heuristics? Maybe also include an input's name? ID are often autogenerated, but names are usually logical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For bfcache there aren't any heuristics; the entire DOM is still around in the same state it used to be.

I'm not aware of any real precedence for trying to capture something interesting about the current state of the page in a way that survives arbitrary author DOM mutations... We could try to invent it, but I'd punt it to the future-additions section for now.

- `e.transitionWhile(promise, { scrollRestoration: "after-transition" })`: the default behavior. The browser delays its scroll restoration logic until `promise` fulfills; it will perform no scroll restoration if the promise rejects. If the user has scrolled during the transition then no scroll restoration will be performed.
- `e.transitionWhile(promise, { scrollRestoration: "manual" })`: The browser will perform no automatic scroll restoration. However, the developer can use the below API to get semi-automatic scroll restoration.

When using `scrollRestoration: "manual"`, the `e.restoreScroll()` API is available. This will perform the browser's scroll restoration logic at the specified time. This allows cases that require precise control over scroll restoration timing, such as a non-broken version of the [demo referenced above](https://nifty-blossom-meadow.glitch.me/legacy-history/transition.html), to be written like so:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we're going down the manual route, should we just leave it to the app to restore scroll manually? e.restoreScroll() is otherwise useful because an app doesn't have to save an old scroll position manually. But would this make API excessively complicated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hopeful e.restoreScroll() is doable without much work, so I kept it in the explainer as an MVP feature. It's true we could omit it though without any compat issues, so it might get deprioritized if other things come up or if we find out that it's not as easy to implement as I'd hoped.


- `e.transitionWhile(promise, { scrollRestoration: "immediate" })`: the browser performs its usual scroll restoration logic, but does so immediately instead of waiting for `promise`.
- `e.transitionWhile(promise, { scrollRestoration: "auto" })`: the browser performs its usual scroll restoration logic, at its usual indeterminate time.
- `const { x, y } = e.scrollDestination()` giving the current position the browser would restore to, if `e.restoreScroll()` was called.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have this API, I think we can drop e.restoreScroll() for manual mode.

@domenic
Copy link
Collaborator Author

domenic commented Jan 19, 2022

Offline feedback is that it might vary depending on navigation. So we still have lots of options to choose from in how to resolve conflicts...

I think I'd prefer to tie this to transitionWhile(). So the alternatives are:

  • Throw on mismatches
  • Last wins
  • First wins
  • Something more complicated

I think throwing is too painful for people who want to have multiple uncoordinated transitionWhile() handlers; see #94 for the background there. Although it should be rare and might not work perfectly, it shouldn't be almost-always-broken.

So I'm going to propose two alternatives:

  1. Last wins

  2. More-complicated version

    In the more-complicated version, transitionWhile(..., { focusReset: "after-transition" }) is not equivalent to transitionWhile(..., { focusReset: undefined }) or transitionWhile(...). So we have three possibilities: "after-transition", "manual", and undefined. The rule is then the last non-undefined value wins.

    The intent here is that if we have multiple people calling transitionWhile(), ones that don't explicitly set focusReset don't impact the outcome, even if they were called after cases that did set it.

I'll spec last-wins for now but am very open to feedback as to whether the more-complicated version, or first wins, would be better.

@domenic domenic changed the title Explainer for focus management, scroll restoration, and :target Explainer for focus management and scroll restoration Jan 27, 2022
@domenic
Copy link
Collaborator Author

domenic commented Feb 1, 2022

Going to split the spec work out into its own PR so I can keep noodling, and then we can point people to the actual README for the current plans.

Plus, expand and organize the stuff about accessibility technology announcements and loading spinners.

Closes #25. Closes #187. Closes #190.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants