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

Additional alignment of focus and selection logic with the web's #1076

Merged
merged 20 commits into from Jun 8, 2019

Conversation

@hypest
Copy link
Contributor

commented Jun 5, 2019

Fixes #1029

Gutenberg PR: WordPress/gutenberg#15999

This PR makes changes to the Aztec wrapper (Android) as well as the RN app to bring it closer to the web as far as focus and selection logic goes.

Important changes:

  1. Try to avoid having Aztec (at the wrapper level and down) request focus automatically. We rely on the JS logic to do that.

  2. setFocusableInTouchMode, setFocusable is set to false until Aztec needs to receive focus. Also setting it to false when Aztec loses focus.

  3. Completely removed the isSelected() prop since the component should not use it as a flag to request focus

  4. Schedule a requestRectangleOnScreen to scroll into view
    When creating a new RichText based block, Aztec receives focus OK but the container doesn't seem to get scrolled to make sure the edit box is in the viewport. https://github.com/wordpress-mobile/gutenberg-mobile/pull/1076/files#diff-5693404c4c8f8d757cd298cc5aabc561R185 asks the system to bring the caret into the viewport. In a future iteration of this logic, we should try to bring the whole block view into the viewport.

  5. Schedule the showSoftKeyboard to happen on a next frame

  6. Remove the 500ms delay for onContentSizeChange and just schedule it for next frame. This will actually dramatically improve the performance perception of the editor on Android, making the writing flow smoother. There's still the visible glitch where blocks start with minimum height and then expand though and fixing that (separate PR) will also greatly benefit the performance.

  7. No need to try blurring other components in block-holder's onFocus. Component will self-blur via https://github.com/WordPress/gutenberg/pull/15999/files#diff-4828a21853e899e5a36faecfa96d83e8R753

  8. Align block-holder's withDispatch with the web's https://github.com/wordpress-mobile/gutenberg-mobile/pull/1076/files#diff-2004b9687d71de0f0a9816269bb4a6a2R276

To test:

  1. Run the demo app and place the caret in the middle (or the end for that matter) of a paragraph block
  2. Tap "Enter" to split the block into two paragraph blocks
  3. Notice that the virtual keyboard is still visible and the caret is normally placed at the beginning of the newly created block.

Also, test focus and selection in general, it should work as expected.

Known issue: at least on Android v7.1.2, sometimes a paragraph block will still have the caret blinking inside it even though a different block might be selected. This seems to be an issue with how the accessibility flag is implemented on ReactNative, in combination with the Android OS underneath it. In particular, setting a block to accessible=true seems to cause a request for focus. Other Android OS versions (at least 8.0 and 9.0) don't seem to exhibit that behavior.

Update release notes:

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

hypest added some commits Jun 5, 2019

Pop the keyboard in the next frame
For better coordination with animations. This way the RichText does get
focus properly when for example a new paragraph block is added.

@hypest hypest added the Android label Jun 5, 2019

@hypest hypest added this to the v1.7 milestone Jun 5, 2019

@hypest hypest referenced this pull request Jun 5, 2019
5 of 5 tasks complete

hypest added some commits Jun 5, 2019

Request caret's line to be visible, not the whole view
That way, the view doesn't jump around when merging blocks.
Just schedule the onContentSizeChange for the next frame
No need for fixed delay, at least when testing this on a Pixel 2XL.
@hypest

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

👋 @daniloercoli , I removed the delay onContentSizeChange and I think you have good context on this particular area of the code so, would be useful if you can take a look and try this PR out. Thanks!

hypest added some commits Jun 6, 2019

@hypest hypest added the bugfix label Jun 6, 2019

@hypest hypest marked this pull request as ready for review Jun 6, 2019

@hypest hypest requested a review from daniloercoli Jun 6, 2019

@hypest

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

👋 @marecar3 and @mkevins, I added Danilo as a reviewer but, please have a try of the app(s) on Android with this PR. It makes some important changes and it'd be good to test out the app for regressions. Thanks in advance!

@hypest hypest requested a review from mkevins Jun 6, 2019

@marecar3

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Hey @hypest it's offtopic but just curious. Is it possible to do a similar approach with selection.start, selection.end? Currently, I think that we are doing on both sides calculation of selection.start, selection.end, and ideally maybe we can move that on JS side only? Maybe I am missing something as I don't have a bigger picture, but this was on my mind when I was working on selection changes after blocks are merged and etc..

@marecar3 marecar3 self-requested a review Jun 6, 2019

@marecar3

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Hey @hypest, nice work here :)

Here are the issues that I have found and which I can't confirm on develop branch.

1. Red screen

  1. Open the WPAndroid app
  2. Create a new post
  3. Create 3 paragraph blocks
  4. Move the last paragraph block to be the first place
  5. Click on the end of that block

Video:

ezgif com-video-to-gif

Result: Red screen

aztec_view_crash

2. Keyboard not dismissed

  1. Open the WPAndroid app
  2. Create a new post
  3. Create 2 empty paragraph blocks
  4. Move focus on the first one
  5. Type some text
  6. Remove all text

Video:

ezgif com-video-to-gif (1)

Result: Keyboard is not dismissed

Issues that don't have a clear scenario (and maybe not affected by this PR)

  1. Red screen - setSpan out of bounds

span_out_of_bounds

  1. Block is stuck on selected state

ezgif com-video-to-gif (2)

@mkevins

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I was able to reproduce the above red screen on the demo app. I cannot reproduce the same on develop. I was able to reproduce it with less steps. Simply moving a rich text block and then selecting it (tap anywhere in the recently moved block) seems to trigger the error. I reproduced this on heading, paragraph, and image caption. If a different block is tapped after moving the block, and then the moved block is tapped, the error does not occur.

Here is the logcat:

2019-06-07 12:05:29.096 31176-31176/com.gutenberg E/unknown:ViewManager: Error while updating prop text
    java.lang.reflect.InvocationTargetException
        at java.lang.reflect.Method.invoke(Native Method)
        at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:83)
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132)
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51)
        at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37)
        at com.facebook.react.uimanager.NativeViewHierarchyManager.updateProperties(NativeViewHierarchyManager.java:136)
        at com.facebook.react.uimanager.UIViewOperationQueue$UpdatePropertiesOperation.execute(UIViewOperationQueue.java:95)
        at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:917)
        at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1025)
        at com.facebook.react.uimanager.UIViewOperationQueue.access$2600(UIViewOperationQueue.java:46)
        at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1085)
        at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29)
        at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:166)
        at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:84)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:909)
        at android.view.Choreographer.doCallbacks(Choreographer.java:723)
        at android.view.Choreographer.doFrame(Choreographer.java:655)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897)
        at android.os.Handler.handleCallback(Handler.java:790)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6494)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)
     Caused by: com.facebook.react.bridge.NoSuchKeyException: start
        at com.facebook.react.bridge.ReadableNativeMap.getValue(ReadableNativeMap.java:124)
        at com.facebook.react.bridge.ReadableNativeMap.getValue(ReadableNativeMap.java:128)
        at com.facebook.react.bridge.ReadableNativeMap.getInt(ReadableNativeMap.java:182)
        at org.wordpress.mobile.ReactNativeAztec.ReactAztecManager.updateSelectionIfNeeded(ReactAztecManager.java:198)
        at org.wordpress.mobile.ReactNativeAztec.ReactAztecManager.setTextfromJS(ReactAztecManager.java:193)
        at org.wordpress.mobile.ReactNativeAztec.ReactAztecManager.setText(ReactAztecManager.java:175)
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:83) 
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132) 
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51) 
        at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37) 
        at com.facebook.react.uimanager.NativeViewHierarchyManager.updateProperties(NativeViewHierarchyManager.java:136) 
        at com.facebook.react.uimanager.UIViewOperationQueue$UpdatePropertiesOperation.execute(UIViewOperationQueue.java:95) 
        at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:917) 
        at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1025) 
        at com.facebook.react.uimanager.UIViewOperationQueue.access$2600(UIViewOperationQueue.java:46) 
        at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1085) 
        at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29) 
        at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:166) 
        at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:84) 
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:909) 
        at android.view.Choreographer.doCallbacks(Choreographer.java:723) 
        at android.view.Choreographer.doFrame(Choreographer.java:655) 
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897) 
        at android.os.Handler.handleCallback(Handler.java:790) 
        at android.os.Handler.dispatchMessage(Handler.java:99) 
        at android.os.Looper.loop(Looper.java:164) 
        at android.app.ActivityThread.main(ActivityThread.java:6494) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807) 
2019-06-07 12:05:29.097 31176-31176/com.gutenberg E/unknown:ReactNative: Exception in native call
    com.facebook.react.bridge.JSApplicationIllegalArgumentException: Error while updating property 'text' of a view managed by: RCTAztecView
        at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:95)
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132)
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51)
        at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37)
        at com.facebook.react.uimanager.NativeViewHierarchyManager.updateProperties(NativeViewHierarchyManager.java:136)
        at com.facebook.react.uimanager.UIViewOperationQueue$UpdatePropertiesOperation.execute(UIViewOperationQueue.java:95)
        at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:917)
        at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1025)
        at com.facebook.react.uimanager.UIViewOperationQueue.access$2600(UIViewOperationQueue.java:46)
        at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1085)
        at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29)
        at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:166)
        at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:84)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:909)
        at android.view.Choreographer.doCallbacks(Choreographer.java:723)
        at android.view.Choreographer.doFrame(Choreographer.java:655)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897)
        at android.os.Handler.handleCallback(Handler.java:790)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6494)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)
     Caused by: java.lang.reflect.InvocationTargetException
        at java.lang.reflect.Method.invoke(Native Method)
        at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:83)
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132) 
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51) 
        at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37) 
        at com.facebook.react.uimanager.NativeViewHierarchyManager.updateProperties(NativeViewHierarchyManager.java:136) 
        at com.facebook.react.uimanager.UIViewOperationQueue$UpdatePropertiesOperation.execute(UIViewOperationQueue.java:95) 
        at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:917) 
        at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1025) 
        at com.facebook.react.uimanager.UIViewOperationQueue.access$2600(UIViewOperationQueue.java:46) 
        at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1085) 
        at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29) 
        at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:166) 
        at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:84) 
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:909) 
        at android.view.Choreographer.doCallbacks(Choreographer.java:723) 
        at android.view.Choreographer.doFrame(Choreographer.java:655) 
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897) 
        at android.os.Handler.handleCallback(Handler.java:790) 
        at android.os.Handler.dispatchMessage(Handler.java:99) 
        at android.os.Looper.loop(Looper.java:164) 
        at android.app.ActivityThread.main(ActivityThread.java:6494) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807) 
     Caused by: com.facebook.react.bridge.NoSuchKeyException: start
        at com.facebook.react.bridge.ReadableNativeMap.getValue(ReadableNativeMap.java:124)
        at com.facebook.react.bridge.ReadableNativeMap.getValue(ReadableNativeMap.java:128)
        at com.facebook.react.bridge.ReadableNativeMap.getInt(ReadableNativeMap.java:182)
        at org.wordpress.mobile.ReactNativeAztec.ReactAztecManager.updateSelectionIfNeeded(ReactAztecManager.java:198)
        at org.wordpress.mobile.ReactNativeAztec.ReactAztecManager.setTextfromJS(ReactAztecManager.java:193)
        at org.wordpress.mobile.ReactNativeAztec.ReactAztecManager.setText(ReactAztecManager.java:175)
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:83) 
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132) 
        at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51) 
        at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37) 
        at com.facebook.react.uimanager.NativeViewHierarchyManager.updateProperties(NativeViewHierarchyManager.java:136) 
        at com.facebook.react.uimanager.UIViewOperationQueue$UpdatePropertiesOperation.execute(UIViewOperationQueue.java:95) 
        at com.facebook.react.uimanager.UIViewOperationQueue$1.run(UIViewOperationQueue.java:917) 
        at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:1025) 
        at com.facebook.react.uimanager.UIViewOperationQueue.access$2600(UIViewOperationQueue.java:46) 
        at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:1085) 
        at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29) 
        at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:166) 
        at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:84) 
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:909) 
        at android.view.Choreographer.doCallbacks(Choreographer.java:723) 
        at android.view.Choreographer.doFrame(Choreographer.java:655) 
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897) 
        at android.os.Handler.handleCallback(Handler.java:790) 
        at android.os.Handler.dispatchMessage(Handler.java:99) 
        at android.os.Looper.loop(Looper.java:164) 
        at android.app.ActivityThread.main(ActivityThread.java:6494) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807) 

hypest added some commits Jun 7, 2019

@hypest

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

I've addressed the crash on selection with WordPress/gutenberg@c034c1a.

@mkevins

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I have tested this via demo app and the crash has been fixed. 🎉

Also:

Remove the 500ms delay for onContentSizeChange and just schedule it for next frame. This will actually dramatically improve the performance perception of the editor on Android, making the writing flow smoother.

I agree, this is looking and feeling a lot better after this fix.

@mkevins mkevins self-requested a review Jun 7, 2019

@mkevins

mkevins approved these changes Jun 7, 2019

Copy link
Contributor

left a comment

I have tested the scenarios described here: #1076 (comment) and everything is working as expected.

Once GB ref is updated, LGTM! 🎉

@hypest

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Thanks for the review @mkevins !

FWIW, I'm still trying to address the 2. Keyboard not dismissed issue reported by Marko.

@etoledom

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

I did a tests run on iOS and it looks good!

Tried to reproduce the issues mentioned above and I couldn't. I also test various scenarios and everything seems to be in order on iOS 👍

@hypest

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

If everyone is OK, I'd propose to update this PR from develop and merge, leaving the UX issues Marko brought up to follow up PR. Cool?

@marecar3

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Hey, @hypest let me try to test if from my side again, I am having some problems with building the WPAndroid due to the latest changes on develop.

update: let me try from gutenberg-mobile demo app :)

@marecar3

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Hey @hypest, I can't reproduce above issues on demo application, so I guess we are ok to merge this one :)

@marecar3
Copy link
Contributor

left a comment

LGTM! :)

@hypest

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Thanks y'all! I'll continue with merging 👍

@hypest hypest merged commit a39972e into develop Jun 8, 2019

6 checks passed

Peril All green. Woo!
Details
ci/circleci: Check Correctness Your tests passed on CircleCI!
Details
ci/circleci: Test Android Your tests passed on CircleCI!
Details
ci/circleci: Test Android on Device Your tests passed on CircleCI!
Details
ci/circleci: Test iOS Your tests passed on CircleCI!
Details
ci/circleci: Test iOS on Device Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.