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

Only report selection changes if text view is still first responder. #1374

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Sep 20, 2019

Fixes #1368

This PR makes sure that no selection changes are propagated after the aztec text view is no longer the first responder.
This way when the Link Modal interface is called and the URL input is activated (becomes the first responder) the selection change on the original Aztec content is not propagated to the link UI.

To test:

  • Open the demo app
  • Add or edit a text block, for example paragraph
  • Write some text
  • Select some of the text
  • Tap on the link button on the toolbar
  • Make sure the link text is pre-filled with the selected text

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@SergioEstevao SergioEstevao added the [Type] Bug Something isn't working label Sep 20, 2019
@SergioEstevao SergioEstevao added this to the Open Beta milestone Sep 20, 2019
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Thank you @SergioEstevao for taking care of this!

I was able to reproduce the issue on an iPad running iOS 13, and this PR solves the issue 🎉 .


I did notice some weirdness on selection while testing this PR, but it seems to be a completely different problem worthy of its own ticket:

Steps:

  • Have a paragraph block with content.
  • Select a word.
  • Add a link to it.
  • Open the link settings again.
  • You will see no link and all the content from the block as Link Text.

This is reproducible in both iOS 12 (from develop), and iOS 13 with the fix on this PR.
I wasn't able to reproduce it on Android, seems to be iOS only.

selection

@@ -571,6 +571,9 @@ class RCTAztecView: Aztec.TextView {
extension RCTAztecView: UITextViewDelegate {

func textViewDidChangeSelection(_ textView: UITextView) {
guard isFirstResponder else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add this statement to the previously existing guard?
Not sure if we get an advantage by having them separated.

@rachelmcr
Copy link
Member

@etoledom That weirdness you mentioned looks like the issue reported in #1161.

@etoledom
Copy link
Contributor

@etoledom That weirdness you mentioned looks like the issue reported in #1161.

@rachelmcr you are right! Looks like it's the same issue. Since it's already a separated ticket, let's 🚢 this one ✅

@SergioEstevao SergioEstevao merged commit e220399 into develop Sep 25, 2019
@SergioEstevao SergioEstevao deleted the issue/1368_add_link_to_selected_text branch September 25, 2019 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] Can't add link to selected text
3 participants