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 mobile cursor from jumping to bottom #2625

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Conversation

LMS007
Copy link
Contributor

@LMS007 LMS007 commented Oct 10, 2020

When selecting text on the android, it was possible to also highlight the text inside the buttons in the adder. When this happened, the range would grab everything from where you started to highlight to the very end of the dom because the adder is attached as the last node inside body.

The simple fix is to prohibit text selection inside the adder which prevents the selection from getting the adder itself.


This was a bit tricky to figure out.

1 You need an android
2 Set up remote debugging (see screen shot) https://developers.google.com/web/tools/chrome-devtools/remote-debugging

Screen Shot 2020-10-09 at 8 59 44 AM

I tracked this down to an issue where in selection.js, it was grabbing a much larger range than it should have. I think what was happening it is the adder itself could act as part of the thing you can select/highlight. On mobile, events for selecting text, touching text, etc fire differently than on the desktop. If you moved you finger too fast or move over a transition from one common ancestor to another, then it grabbed the adder text too.

I was able to totally prevent this by adding 'pointer-events: none; on the adder. While this fully disabled the adder, it fixed this issue. Next I thought that perhaps we should dynamically set that value and we should be good! I was able to pinpoint the problem by adding a simple condition inside selectedRange

if (range.endContainer.nodeName === 'HYPOTHESIS-ADDER') {
   return null;
}

And then in guest.js, we can pass down a flag when range is null to add a css class to the adder. This actually didn't work very well perhaps due to something asynchronous that I didn't have a good understanding of.

The next idea was to just prevent css selection entirely and it didn't need a flag because I don't think we really care about allowing the text to be selected in the first place the adder.

Things to consider.

  • Test this on an iphone (Done. I tested this on my iPhone X - @robertknight)
  • What does this mean for accessibility / voice over? (This should be fine. user-select affects what a user can select. It doesn't affect how a screen reader reads controls - @robertknight)

@LMS007 LMS007 requested a review from robertknight October 10, 2020 00:01
@robertknight robertknight force-pushed the android-selection-fix branch from dc2fd09 to 3111f65 Compare October 12, 2020 11:35
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #2625 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2625   +/-   ##
=======================================
  Coverage   97.38%   97.38%           
=======================================
  Files         200      200           
  Lines        7466     7466           
  Branches     1647     1647           
=======================================
  Hits         7271     7271           
  Misses        195      195           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a28edd...796c74b. Read the comment docs.

The issue is not specific to Chrome-based browsers on Android, although
it does cause much more of a problem in that browser than Safari on iOS.
This makes it easier to use the dev server for testing Hypothesis on
mobile devices with smaller screens (ie. phones).
@robertknight robertknight force-pushed the android-selection-fix branch from a69b397 to 796c74b Compare October 12, 2020 11:46
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

The change here makes complete sense. user-select is the right tool for this. I tested on iOS and it is possible to reproduce the problem but it is much less of an issue in practice due to slight differences in selection behavior in Safari vs. Chrome.

It looks like an earlier version of the change was committed directly to master by mistake in 3a28edd.

I have made a couple of further changes in this PR to reword the comment to clarify what user-select does and remove the mention of Android, since the issue can occur in other browsers as well. I also made a change to use a mobile-optimized viewport for the dev server homepage for ease of testing.

@robertknight robertknight merged commit 24bf671 into master Oct 12, 2020
@robertknight robertknight deleted the android-selection-fix branch October 12, 2020 11:53
@robertknight
Copy link
Member

robertknight commented Oct 12, 2020

The solution here resolves the immediate problem of text inside the adder being selected. However with user-select the adder still impedes text selection to some extent because it appears where the user is dragging their finger. This can make it fiddly when selecting text over multiple lines. As the user drags their finger down to select the next line, it may move over the location where the adder is. When this happens, instead of expanding the selection to include the next line, it will remain unchanged because the element under the finger is the adder, which has user-select: none. The user instead needs to move their finger outside of the adder to get the selection to expand.

I can see several ways we might be able to resolve this:

  1. Make the adder "transparent" to events while the user is dragging, eg. by temporarily assigning pointer-events: none
  2. Don't show the adder while the user is creating a selection but only when the user releases their finger. This is how the native selection UI solves the problem
  3. Move the adder to a fixed location on the screen where it is well away from the selection. On small screens it might be fine to show the adder as a toolbar at the bottom of the window for example. On an a larger display (eg. an iPad) this could be confusing since the toolbar will appear a long way away from the selection and thus the user may not notice it.

My recommendation would be to look into option (2) first of all.

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