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

Refine Bottom Sheets in/out transition #820

Merged
merged 10 commits into from
Apr 16, 2019
Merged

Conversation

koke
Copy link
Member

@koke koke commented Apr 4, 2019

Details in Gutenberg PR: WordPress/gutenberg#14831

On the gutenberg-mobile side, this also fixes how we were presenting the block picker so it actually animates out instead of dismissing immediately.

Fixes #595

koke added 2 commits April 4, 2019 23:30
Instead of conditionally rendering, just pass the isVisible property (which
will be passed down to BottomSheet). This prevents the picker being dismissed
immediately when closed, instead of using the desired animation.
@koke koke added [Type] Enhancement Improves a current area of the editor Editor Chrome labels Apr 4, 2019
@koke koke requested a review from etoledom April 4, 2019 21:34
@koke
Copy link
Member Author

koke commented Apr 4, 2019

I'm a bit confused by the test error:

TypeError: Cannot read property 'maxWidth' of undefined

      156 | 
      157 | function getWidth() {
    > 158 | 	return Math.min( Dimensions.get( 'window' ).width, styles.background.maxWidth );
          | 	                                                                     ^

Is jest not expected to import the css properly?

@etoledom
Copy link
Contributor

etoledom commented Apr 4, 2019

Is jest not expected to import the css properly?

That's it.
We need to add the css properties referenced directly to the css mock file here:

@koke
Copy link
Member Author

koke commented Apr 4, 2019

Thanks! Tests are passing locally now. That file kind of makes me want to make a case of dropping SCSS for native styles 😬

@koke koke added this to the v1.3 milestone Apr 5, 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.

Looking good!

I left a comment about why we were removing the component instead of using the isVisible prop.
The referenced PR is not too clear, so this is a simpler explanation:

When we add a block with a text input (any kind), that start focused by default, when the picker is dismissed via isVisible = false, the focus will change to the previous block (if it also has a text input).

There might be another cause of this behavior, but when I investigated it many months ago, this was the only solution I could find.

focus_bouncing_back

src/block-management/block-manager.js Outdated Show resolved Hide resolved
@koke
Copy link
Member Author

koke commented Apr 12, 2019

Oh interesting. I debugged it a bit, and I think I know what's going on.

When you select a block from the picker:

  1. Gutenberg dismisses the inserter
  2. Gutenberg creates the new block and focuses on it
  3. Since the inserter is a modal dismissed with animation, when the animation finishes iOS tries to restore the focus to what it was on the underlying view

Relevant stack trace:

frame #0: 0x000000010902a729 RNTAztecView`RCTAztecView.textViewDidBeginEditing(textView=0x00007fde7bd2e400, self=0x00007fde7bd2e400) at RCTAztecView.swift:528
    frame #1: 0x000000010902a85c RNTAztecView`@objc RCTAztecView.textViewDidBeginEditing(_:) at <compiler-generated>:0
    frame #2: 0x0000000112e1af3c UIKitCore`-[UITextView _notifyDidBeginEditing] + 80
    frame #3: 0x0000000112e1aa90 UIKitCore`-[UITextView becomeFirstResponder] + 703
    frame #4: 0x0000000112d9615a UIKitCore`-[UITextInteractionAssistant(UITextInteractionAssistant_Internal) setFirstResponderIfNecessary] + 208
    frame #5: 0x0000000112d970e3 UIKitCore`-[UITextInteractionAssistant(UITextInteractionAssistant_Internal) checkEditabilityAndSetFirstResponderIfNecessary] + 380
    frame #6: 0x0000000112e1c03c UIKitCore`-[UITextView _restoreFirstResponder] + 65
    frame #7: 0x0000000112932f8b UIKitCore`-[UIPeripheralHost(UIKitInternal) _restoreInputViewsWithId:animated:] + 768
    frame #8: 0x00000001124b8aac UIKitCore`-[UIViewController _restoreInputViewsForPresentation] + 269
    frame #9: 0x00000001123baabc UIKitCore`-[UIPresentationController runTransitionForCurrentState] + 1564
    frame #10: 0x00000001123b875c UIKitCore`-[UIPresentationController _dismissWithAnimationController:interactionController:target:didEndSelector:] + 730
    frame #11: 0x00000001124b881b UIKitCore`__99-[UIViewController _dismissViewControllerWithAnimationController:interactionController:completion:]_block_invoke.2142 + 77
    frame #12: 0x0000000112f9b235 UIKitCore`+[UIView(Animation) performWithoutAnimation:] + 90
    frame #13: 0x00000001124b85ec UIKitCore`-[UIViewController _dismissViewControllerWithAnimationController:interactionController:completion:] + 643
    frame #14: 0x0000000112f9b235 UIKitCore`+[UIView(Animation) performWithoutAnimation:] + 90
    frame #15: 0x00000001124b820a UIKitCore`-[UIViewController _dismissViewControllerWithTransition:from:completion:] + 752
    frame #16: 0x00000001124b78b3 UIKitCore`-[UIViewController dismissViewControllerWithTransition:completion:] + 1189
    frame #17: 0x00000001124b7662 UIKitCore`-[UIViewController dismissViewControllerWithTransition:completion:] + 596
    frame #18: 0x00000001124b6dd9 UIKitCore`-[UIViewController _performCoordinatedPresentOrDismiss:animated:] + 511
    frame #19: 0x00000001124b9f3c UIKitCore`-[UIViewController dismissViewControllerAnimated:completion:] + 135
  * frame #20: 0x00000001091fbbea React`-[RCTModalHostViewManager dismissModalHostView:withViewController:animated:](self=0x00006000024cb8a0, _cmd="dismissModalHostView:withViewController:animated:", modalHostView=0x00007fde754f7870, viewController=0x00007fde754f7ac0, animated=NO) at RCTModalHostViewManager.m:92
    frame #21: 0x00000001091f98f9 React`-[RCTModalHostView dismissModalViewController](self=0x00007fde754f7870, _cmd="dismissModalViewController") at RCTModalHostView.m:164

I think I'll revert the change for now and move this to a new issue. The first thing I'd try to do is hook into react-native-modal onModalHide, and have one callback for when the block type is tapped, so the new block is inserted immediately, and a second one for when the bottom sheet is fully dismissed, so we can focus on the right block.

@koke
Copy link
Member Author

koke commented Apr 12, 2019

This should be ready to review again

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.

The first thing I'd try to do is hook into react-native-modal onModalHide, and have one callback for when the block type is tapped, so the new block is inserted immediately, and a second one for when the bottom sheet is fully dismissed, so we can focus on the right block.

That sounds like it could work. Thanks for checking it out :)

@koke koke merged commit a29bec3 into develop Apr 16, 2019
@koke koke deleted the issue/595-bottom-sheet-animation branch April 16, 2019 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editor Chrome [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine Bottom Sheets in/out transition
2 participants