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 onFocus called too many times #86

Merged
merged 2 commits into from
Dec 4, 2018

Conversation

daniloercoli
Copy link
Contributor

@daniloercoli daniloercoli commented Dec 4, 2018

This PR fixes an issue where multiple instances of AztecWrapper in the same screen can lead to a focus loop. See Focus Loop when deleting content and blocks.

Removing the onFocus bind, and relying only onPress fixes the problem.

Testing steps provided in the other PR here: wordpress-mobile/gutenberg-mobile#314.

Copy link
Contributor

@mzorz mzorz 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 for finding this fix @daniloercoli !

I've tested it on various emulators and handsets and I can confirm it fixes the focus change infinite loop problem.

I've also been following the code and undesrtand how onPress on the Touchable area calls the onFocus listeners which in turn may be calling (as is the case for the mobile Gutenberg project) onFocus on RCTAztecView again, thus generating the problem.
Given the line onFocus = { () => {} } seems too weird and will surely catch more than one pair of eyes in the future, I've left a suggestion to make a more thorough comment there, explaining the problem, so future developers that find this line understand have more information to understand why this line is important and does what we actually intend to do.

Let's add that comment and then we'll be good to merge 👍

src/AztecView.js Outdated Show resolved Hide resolved
@daniloercoli daniloercoli merged commit daf2d1c into master Dec 4, 2018
@daniloercoli daniloercoli deleted the fix/loop-deleting-fast-insert-fast branch December 4, 2018 15:03
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