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

Fix highlighter behavior on mobile #1694

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

omnibs
Copy link
Member

@omnibs omnibs commented Aug 7, 2024

🔧 Modifying a component

Context

Part of https://linear.app/noredink/issue/PHX-1796/investigate-double-click-to-highlight-individual-words#comment-3a9f6403

We noticed users were commonly creating highlights and deleting them within 5s, and deemed those "accidental highlights".

We suspected the accidental highlights were coming from mobile devices, and investiating it opened a bit of a pandora's box for mobile issues with highlighting. The ones relevant to noredink-ui are:

  • When you tap over a highlightable in the middle of a scrolling motion, it trigger a highlight
scroll.highlight.mov
  • Tap+move to highlight sentences was being processed for all highlights in the page
spooky.action.at.a.distance.mov
  • Tap+move to highlight sentences wasn't working at all for viewFold, because:
    • There is no highlight container enforced
    • Elements are not necessarily direct descendants of the container
viewFold.mov

This fixes the 2nd and 3rd issues. The first one will be harder, so I'll do it in a follow-up PR.

I chose not to make viewFold create a container, which might be a mistake, because it'll be easy for folks to forget that's necessary for mobile, like we forgot to set up highlighter ports at all in project Text Response. I'm super open to forcing a generic div container inside viewFold.

Component completion checklist

  • I've gone through the relevant sections of the Development Accessibility guide with the changes I made to this component in mind
  • Changes are clearly documented
    • Component docs include a changelog
    • Any new exposed functions or properties have docs
  • Changes extend to the Component Catalog
    • The Component Catalog is updated to use the newest version, if appropriate
    • The Component Catalog example version number is updated, if appropriate
    • Any new customizations are available from the Component Catalog
    • The component example still has:
      • an accurate preview
      • valid sample code
      • correct keyboard behavior
      • correct and comprehensive guidance around how to use the component
  • Changes to the component are tested/the new version of the component is tested
  • Component API follows standard patterns in noredink-ui
    • e.g., as a dev, I can conveniently add an nriDescription
    • and adding a new feature to the component will not require major API changes to the component
  • If this is a new major version of the component, our team has stories created to upgrade all instances of the old component. Here are links to the stories:
    • add your story links here OR just write this is not a new major version
  • Please assign the following reviewers:
    • Someone from your team who can review your PR in full and review requirements from your team's perspective.
    • Component library owner - Someone from this group will review your PR for accessibility and adherence to component library foundations.
    • If there are user-facing changes, a designer. (You may want to direct your designer to the deploy preview for easy review):
      • For writing-related component changes, add Stacey (staceyadams)
      • For quiz engine-related components, add Ravi (ravi-morbia)
      • For a11y-related changes to general components, add Ben (bendansby)
      • For general component-related changes or if you’re not sure about something, add the Design group (NoRedInk/design)

All highlighters would process these touch events, regardless of whether
the events were meant for them or not
Helps learn when we misconfigured nri-ui examples
Also, we're guaranteed to have events only for the current highlighter,
since this event listener is assigned to document.getElementById(id) up
top.
Copy link

linear bot commented Aug 7, 2024

@omnibs omnibs marked this pull request as ready for review August 7, 2024 01:26
Copy link
Contributor

@brian-carroll brian-carroll left a comment

Choose a reason for hiding this comment

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

Good stuff.
Sounds like we have some holes in our testing process for mobile. 😬
I tested this on my Google Pixel 6 using Chrome, and confirmed that the bugs shown in spooky.action.at.a.distance.mov and viewFold.mov are fixed.
scroll.highlight.mov is broken, as expected.

@omnibs omnibs changed the title Phx 1796 fix mobile highlighting Fix highlighter behavior on mobile Aug 7, 2024
@omnibs
Copy link
Member Author

omnibs commented Aug 12, 2024

ps: i'm holding this one bc fixing persisting highlights on mobile while mobile is still so prone to accidental highlights is not super desirable, so I'll merge this one when #1695 is ready (ty for the speedy review, regardless!)

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

Successfully merging this pull request may close these issues.

2 participants