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

Contain [aria-busy] toggling within Frames #442

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

seanpdoyle
Copy link
Contributor

Setting [aria-busy="true"] on the <html> element during a
<turbo-frame> navigation might be too aggressive given the frame's
value proposition of a compartmentalized request-response cycle. If
applications style the page based on html[aria-busy="true"], having
that toggled during (eager or lazy) Frame loading could lead to a
disorienting experience.

If that isn't the case for applications, they can synchronize the
attribute themselves with a MutationObserver:

const observer = new MutationObserver(([ { target } ]) => {
  document.documentElement.setAttribute("aria-busy", target.busy)
}, { subtree: true, attributeFilter: ["busy"] })

observer.observe(document.body)

Setting `[aria-busy="true"]` on the `<html>` element during a
`<turbo-frame>` navigation might be too aggressive given the frame's
value proposition of a compartmentalized request-response cycle. If
applications style the page based on `html[aria-busy="true"]`, having
that toggled during (eager or lazy) Frame loading could lead to a
disorienting experience.

If that isn't the case for applications, they can synchronize the
attribute themselves with a `MutationObserver`:

```js
const observer = new MutationObserver(([ { target } ]) => {
  document.documentElement.setAttribute("aria-busy", target.busy)
}, { subtree: true, attributeFilter: ["busy"] })

observer.observe(document.body)
```
@dhh
Copy link
Member

dhh commented Nov 12, 2021

I think if we can't use aria-busy in a uniform way, we should use data-turbo-busy, and then leave it as an exercise for the app to choose when aria-busy might make sense for their flow.

@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Nov 12, 2021

I think Turbo's usage of [aria-busy] on the <html> element for page-wide navigations and the <turbo-frame> (and submitted <form>) is appropriate.

The issue this change is addressing would be the same with [data-turbo-busy].

Imagine an application that renders a page-wide spinner instead of a progress bar. If a <turbo-frame> navigation toggled html[data-turbo-busy] (or html[aria-busy="true"], the attribute itself is unimportant), and CSS was hooked off [data-turbo-busy], the spinner would be rendered whenever a lazy- or eagerly- loaded frame was navigated.

This change aims to contain <turbo-frame>-initiated toggling to the frame itself and the <form> that initiated the navigation. Does that break the uniformity of our usage?

seanpdoyle added a commit to seanpdoyle/turbo-site that referenced this pull request Nov 12, 2021
@dhh
Copy link
Member

dhh commented Nov 12, 2021

Ahhh, I misunderstood the original issue. Yes, that this is about turbo-frame being self-contained. Not whether aria-busy or not makes sense. I thought you meant it wouldn't work for the turbo-frame. Yup, all good with this then.

@dhh dhh merged commit c4e62f8 into hotwired:main Nov 12, 2021
@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Nov 12, 2021

Switching to [data-turbo-busy] leaves the door open for the same style of mutation observer, where the client applications could own the logic for setting [aria-busy]:

const observer = new MutationObserver((mutations) => {
  for (const { target } of mutations) {
    if (target.localName == "html") {
      // somehow determine if the change was initiated by a `<turbo-frame>` element or a page visit
    } else if (target.localName == "form" || target.localName == "turbo-frame") {
      target.setAttribute("aria-busy", target.hasAttribute("data-turbo-busy"))
    }
  }
}, { subtree: true, attributeFilter: ["data-turbo-busy"] })

observer.observe(document.body)

That's possible, but the issue feels the same for html-scoped attribute changes.

@seanpdoyle seanpdoyle deleted the frame-do-not-toggle-busy-on-html branch November 12, 2021 16:07
@dhh
Copy link
Member

dhh commented Nov 12, 2021

You could actually make a fair case that aria-busy should not be set on html, but that data-turbo-busy should. Since you might have a global style spinner. But... I think it's fine to see how this is going to be used first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants