search

vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.44k stars 26.79k forks source link

Text inside `next-route-announcer` can be copied to clipboard #37915

Open zgreen opened 2 years ago

zgreen commented 2 years ago

Verify canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:29 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T8101
Binaries:
  Node: 16.15.1
  npm: 8.11.0
  Yarn: 1.22.17
  pnpm: 7.3.0
Relevant packages:
  next: 12.1.7-canary.44
  react: 18.2.0
  react-dom: 18.2.0

What browser are you using? (if relevant)

Version 102.0.5005.115 (Official Build) (arm64)

How are you deploying your application? (if relevant)

next start and/or next export

Describe the Bug

Text inside next-route-announcer can be copied to clipboard via CMD+A and then CMD+C (Mac).

While the text inside next-route-announcer is hidden from view, it can still be copied to clipboard. This can occur if a user selects all text on the page, and also occasionally if the Next.js application manipulates text selection near the bottom of the page.

Expected Behavior

Text inside next-route-announcer should not be available to the clipboard.

(I would have filed a PR myself, but thus far I'm unable to build latest canary with pnpm, owing to what looks like a node-sass error)

Link to reproduction

https://github.com/zgreen/nextjs-user-select-bug

To Reproduce

You must trigger a route change for this issue to occur.

Using the reproduction link provied:

  1. Start the application with npm run dev
  2. Click on the "test" text at the very top of the page (this will trigger a route change)
  3. Select all text via CMD+A and then CMD+C (Mac)
  4. Observe that the clipboard text is now hello!hello!, despite the fact that the text hello! only appears once on the page. This is because the next inside next-route-announcer has also been copied.

This can be easily remedied using user-select, added as an inline-style here (I've used a similar approach with a global stylesheet to fix this issue on my own sites.

Again, happy to submit a PR if I can figure out a way around the node-sass build failure.

Thanks!

SukkaW commented 2 years ago

It is kinda annoying, but I wonder if it is working as intended.

Gatsby has <div id="gatsby-announcer"> as well, and if you use Cmd + A on https://www.gatsbyjs.com, the content of route announcer will also end up in the clipboard.

I have also checked some community implementations as well:

None of them use user-select: none at all. Maybe it is not a bug after all? IMHO we should consult an a11y expert on this.

shuding commented 2 years ago

@SukkaW Yeah this is a tricky problem. If I recall correctly, when setting the content to be not visible/selectable via hidden, display: none, visibility: hidden, etc. it can’t be detected by VoiceOver anymore. But I did not test the user-select: none case.

junyper commented 2 years ago

I think because the content in this live region should also be present in the page (e.g. in an h1 element), it should be safe to disable the selection on it. Also note that we are trying to replicate the native browser behavior when navigating to a new page here, and in that case the <title> content isn't included in the text selection either. Adding this style will definitely improve the UX for sighted users as it's very unexpected to include content that is not visible in their selection in the clipboard contents.

junyper commented 2 years ago

Another option: we could remove the live region content after a delay (5-7 seconds to be safe). I think that is what react-spectrum's LiveAnnouncer does.

shuding commented 1 year ago

That's a great call!

sentience commented 1 year ago

The new app-router-announcer seems to avoid this issue by hiding the announcer inside a Shadow DOM element:

https://github.com/vercel/next.js/blob/cd70065bad00cb9d3b0e7c9a94ee9241e3c5f5d8/packages/next/src/client/components/app-router-announcer.tsx#L22-L25

…where it will be announced by a screen reader but not read as part of the page content. A fix for next-route-announcer, I think, could do the same thing.

sentience commented 1 year ago

Related: #35831

B-Salinas commented 10 months ago

I'm ngl, found this github issue totally at random.

TL;DR: Updating my portfolio and looking at nice websites' frameworks to figure out what to use for seemless-ness. Couldn't find the framework/s using google chrome inspect tools, so found a tag I don't recognize -- next-route-announcer --, googled and here I am.

What I'm about to type seems like a really ~ dumb ~ question to me, but why is /this/ a bad thing - being able to copy to the clipboard what's in next-route-announcer?

(and thank you to the existence of this github issue)

sentience commented 10 months ago

@B-Salinas It's a bad thing because every attempt has been made to have the next-route-announcer be imperceptible to users. We don't actually want the user to perceive a duplication of the page title sitting at the end of the page content – we just want screen readers to announce changes to the page title when they are made. The only way to do that is to reproduce the current page title inside an aria-live region on the page, and update it when the page title updates.

In other words, semantically, next-route-announcer should not be a part of the page content, but an alternative way for users to access the page title. Therefore, when copying page content, we would prefer next-router-announcer not to be included.

I hope that makes sense!

B-Salinas commented 10 months ago

@B-Salinas It's a bad thing because every attempt has been made to have the next-route-announcer be imperceptible to users. We don't actually want the user to perceive a duplication of the page title sitting at the end of the page content – we just want screen readers to announce changes to the page title when they are made. The only way to do that is to reproduce the current page title inside an aria-live region on the page, and update it when the page title updates.

In other words, semantically, next-route-announcer should not be a part of the page content, but an alternative way for users to access the page title. Therefore, when copying page content, we would prefer next-router-announcer not to be included.

I hope that makes sense!

OHH, that's interesting honestly. You think it'd be as simple as toggling something that hides selected tags from screen readers -- not originally what I thought this was about. Thank you @sentience for breaking it down! I hope this bug is squashed soon :)