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 drag shadow dimensions crash #924

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Aug 27, 2020

Fixes wordpress-mobile/WordPress-Android#10492

Note: only one reviewer required

Description

The issue described turned out to be a problem with the text selection having a zero-width representation on the end of a paragraph or line and as such, when the drag and drop mechanism wanted to create the shadow image to be dragged around, it would then fail because you can't create an image with zero dimensions.

Narrowing down

In Aztec, we're using EndOfParagraphMarker span to mark the end of a paragraph, same happens with a newline.

For example, this is an actual "\n" without it being end of paragraph:

newlinehere

And here we have an EndOfParagraphMarker:

endofparagraphmarker

What the solution does

This solution makes use of the precedence in which events are handled. The Drag listener is only called when a drag operation has been determined to have started. For that, we first need to detect a long press gesture. Once we detect it, we figure out whether the long press happened within the boundaries of selected text (*). Then, if the selected is exactly 1 character and such spanned has either an EndOfParagraphMarker or a AztecVisualLinebreak span, then we just mark the event as handled to prevent the regular View's handler from choking with it.

I decided to apply this patchonly for the platforms where this behavior has been observed (Android 9 for 99% of users, and 10 as I was able to observe all of the times I tested on an API 29 emulator).

Other

Also, when I dragged newlines from a textarea (using split screen with Chrome open), the newlines appeared as escaped characters, instead of actual newlines, or
tags.

I also observed this, and also (as can be seen up in the comments) the placement of the cursor is far left/top away from the actual touch points (where user's finger is touching).

(*) A note on the selection: it's not perfect, because we calculate a box starting on the top-left corner of the first line that has selected text, and the box ends on the right-bottom end of the last line containing selected text (this means you could long tap outside the selected text but still on one of the lines on the extremes and still trigger the listener code checks) but it should be enough. In practice, if you have code selected and you long tap around the edges it should be OK to assume you wanted to tap on the selection. And if that was not the case, then a single tap should unselect and get you ready to go. I don't expect troubles with this, but wanted to mention so the reviewer knows how I came about this.

Alternative solution

I first explored the solution of implementing our own drag & drop mechanism as can be seen here by mimicking what TextView does, and actually providing a non-zero dimensioned shadow view when it's the case that the selection has a zero-width representation.

This works well. However, I thought this was going too far, as we can just use the same long press detection mechanism and then just omit the action altogether (after all, who would want to drag a newline only? Seems like a gesture glitch more than anything).

In the case the current solution in this PR does not cover all the cases, we can always go ahead with the other branch solution (moving it here to AztecText instead of using AztecEditorFragment of course) and just make it work for every case. 👍

To test

Aztec demo app:

  1. open aztec demo
  2. scroll down, tap on the right of the W logo
  3. write a short word, observe it gets added as a new line (because there's no space)
  4. now select the word you just written
  5. move the start caret up, right to the right of the W image
  6. move the end carett left, right to the beginning of the word you entered
  7. now long tap on the image (it should work given the image is technically on the same line as per the linebreak)
  8. observe nothing happens. Internally, the long click listener is triggered, the AztecVisualLinebreak is identified as having been selected and as such we signal the listener as handled, to avoid having a crash.

aztecdemosele

WPAndroid:

  1. set your site settings to not use the Block editor, so we use Aztec only
  2. create a new Post
  3. add a few lines of text, breaking every now and then
  4. select the first word of an intermediate line
  5. then, move the start selector caret up and to the end of the line right above of it
  6. then, move the end selector caret to the left to the very beginning of the line
  7. now, long press on the selection and try to move around
  8. observe nothing happens 👍
    Should look like this:
    selectin

(side note: on WPAndroid I tried this branch which implements pretty much the same things on AztecEditorFragment, to make sure whether there was a difference with where we'd set the long click listener, but found none, so it's better to have it here in AztecText directly and also save the problem for Gutenberg as well).

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mzorz mzorz requested review from ashiagr and mkevins August 27, 2020 17:06
Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

Nice work @mzorz!
Works as expected!

  • Demo Aztec app ✅
  • WPAndroid app ✅

@mzorz
Copy link
Contributor Author

mzorz commented Aug 28, 2020

Thank you @marecar3 ! 🙇

Also adding a note here just in case - I also tested mobile Gutenberg for long press detection by changing the aztecVersion in gradle files to point to the commit hash in this PR, and couldn't find any issues 👍

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.

IllegalStateException: Drag shadow dimensions must be positive
2 participants