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

FloatingToolbar ideal version #1414

Closed
pinarol opened this issue Oct 7, 2019 · 15 comments · Fixed by #2118
Closed

FloatingToolbar ideal version #1414

pinarol opened this issue Oct 7, 2019 · 15 comments · Fixed by #2118
Assignees

Comments

@pinarol
Copy link
Contributor

pinarol commented Oct 7, 2019

This is the follow-up task for issue: #1413

We should update FloatingToolbar to gain the Floating behavior, here's the comparison between temp & ideal solutions:

FloatingToolbar

cc: @iamthomasbishop

@chipsnyder
Copy link
Contributor

After spending some time looking at this issue here's what I found so far:

iOS

iOS is the easier one here. For iOS, you can add a custom view "OverflowView" to the view hierarchy and override (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event with a custom implementation. If you normalize the point in the window with:
CGPoint normalizedPoint = [self convertPoint:point toView:nil];

Then if you compare that to the normalized frame:

CGRect adjsutedFrame = subview.frame;
adjsutedFrame.origin = [parent convertPoint:subview.frame.origin toView:nil];

Then you can traverse the view hierarchy and respond appropriately to the touch. The end result looks something like this:

- (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event {
    
    CGPoint normalizedPoint = [self convertPoint:point toView:nil];
    UIView *successfulHit = [self checkHitTest:normalizedPoint withEvent:event inView:self];

    if(successfulHit) {
        return successfulHit;
    }
    
    return [super hitTest:point withEvent:event];
}

- (UIView *)checkHitTest:(CGPoint)point withEvent:(UIEvent *)event inView:(UIView *)parent {
    
    for (UIView *subview in parent.subviews) {
        
        CGRect adjsutedFrame = subview.frame;
        adjsutedFrame.origin = [parent convertPoint:subview.frame.origin toView:nil];
        
        if (CGRectContainsPoint(adjsutedFrame, point)) {
            NSLog(@"HIT!");
            return subview;
        } else {
            return [self checkHitTest:point withEvent:event inView:subview];
        }
    }
    
    return nil;
}

I was also able to get this to work in a less ideal way in iOS with no code changes but by increasing the hitslop on the JS component.

Android

Android has been a little more problematic.

As a refresher for handling touch events in Android, I would recommend looking at

I set up a simplified example of this problem in Expo
screenshot-1581431743233

In the above image, the Red and Orange boxes represent a block, and the Green box is the floating toolbar.
I then made the following adjustments

  • On the FlatList I added a CellRendererComponent to return a View subclass OverflowView
  • I adjusted the OverflowView and subviews to have an increased hitslop down to the view that housed the Floating toolbar

With these changes, if you entered the inspector and tapped on the Floating Toolbar the correct Block would be highlighted but no clicks would process.

Using the example above: If I tapped on the Floatingtoolbar (green box) and had break points in dispatchTouchEvent() and onInterceptTouchEvent() my expectation would be that the Orange block would get these events. In actuality, the Red box gets these events and Orange never fires. So somewhere in the line, the events are being swallowed.

I did find this PR facebook/react-native#26374 which seems to address part of our problem. However, trying this locally doesn't seem to fully resolve the issue.

I think by overriding ReactViewGroup to parse through the views in an order based on z-index and having the views follow the Android touch detection pattern in that order would allow us to capture the taps and adjust accordingly.

Will need to come back to this later

@dratwas
Copy link
Contributor

dratwas commented Feb 12, 2020

@chipsnyder

iOS is the easier one here. For iOS, you can add a custom view "OverflowView" to the view hierarchy and override (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event with a custom implementation. If you normalize the point in the window with:
CGPoint normalizedPoint = [self convertPoint:point toView:nil];

Do we really need a native change to make it works on iOS? As far as I remember we were able to implement that on iOS and only Android was the problem, but i'm not 100% sure :)

@pinarol
Copy link
Contributor Author

pinarol commented Feb 12, 2020

@dratwas Correct me if I am wrong but I think it was still a kind of workaround. AFAIR, the solution was putting the top most block's FloatingToolbar in the parent level instead of the block level. It worked on iOS but it also introduced an exception in the code design which we don't prefer in the long term.

@dratwas
Copy link
Contributor

dratwas commented Feb 12, 2020

@pinarol I think it was the Android case. I created a simple snack with https://snack.expo.io/rJiqFLbXU and it seems to work on iOS. Also, I decided to check that in our app since it could work differently in the snack.

To reproduce it please open gutenberg/packages/block-editor/src/components/block-list/block-mobile-floating-toolbar.native.scss and add

	position:absolute;
	top: -50;

to the .floatingToolbar styles

Should look like

.floatingToolbar {
	background-color: $dark-gray-500;
	margin: auto;
	min-width: 100;
	max-height: $floating-toolbar-height;
	border-radius: 22px;
	flex-direction: row;
	z-index: 100;
	height: $floating-toolbar-height;
	align-items: center;
	justify-content: center;
	align-self: center;
	margin-bottom: 8px;
	position:absolute;
	top: -50;
}

The floating toolbar is positioned absolute and all is clickable inside w/o any native change.

@chipsnyder
Copy link
Contributor

@dratwas You're right I had forgotten I got that working in my Snack with no native changes. But I wasn't able to get the correct highlights in Android without doing some increase on the hitslop. Once I started adding the hitslop spacing, it would still work, but I would probably need some native code still.

@dratwas
Copy link
Contributor

dratwas commented Feb 12, 2020

but I would probably need some native code still.

In iOS or Android? I agree that we need some native changes on Android but not sure why we need these on iOS.

@chipsnyder
Copy link
Contributor

Oh sorry yeah I didn't really complete that thought. I meant in both but only if hitslop is required for the end Android Implementation. However, we could only add that if the platform is Android. The issue I was running into with iOS and an increased hitslop then the view below would get the taps on iOS when next to the floating toolbar. That all depends on where Android ends up though.

@pinarol
Copy link
Contributor Author

pinarol commented Feb 12, 2020

I dug into history to be sure. Unfortunately, it wasn't working on iOS without the workaround at RN side.
Please refer to this comment.

And locate this commit under it:

Screen Shot 2020-02-12 at 15 22 29

Only after that workaround, we had a working result for iOS.

Then there’s a new comment saying that workaround didn’t work for Android: WordPress/gutenberg#17399 (comment)

@pinarol
Copy link
Contributor Author

pinarol commented Feb 12, 2020

I'd like to underline one thing to avoid confusions, the unreceived touch issue on the FloatingBar only manifests itself at the top most block in the FlatList.

@pinarol
Copy link
Contributor Author

pinarol commented Feb 12, 2020

I'll drop a rather more primitive version of the iOS side solution I had in my mind for documentation purposes:

Assuming:

  • OverflowView is the block container view
  • OverflowView inherits RCTView so that styling settings can be applied with no extra development

on JS side, pass the tag of FloatingToolbar instance explicitly:

<OverflowView overflowingViewTag={ this.state.floatingToolbarTag } >
   <FloatingToolbar onRef={ (ref) => { this.setState({ floatingToolbarTag: ReactNative.findNodeHandle(ref)}) } }/>
</OverflowView>

on native side, inside OverflowView class, override hitTest and just check for FloatingToolbar intercepts with the touch point:

- (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event {
 
   UIView *floatingToolbar = [uiManager viewForReactTag: overflowingViewTag];
   //detect if the touch is on floatingToolbar, if so, return it

@dratwas
Copy link
Contributor

dratwas commented Feb 14, 2020

Hey, @chipsnyder I wanted to point to one thing. Please test your solution for the group block as well since the solution that I posted above doesn't work for the first block in the group block. It happens because the floating toolbar is rendered outside the Group and is covered by the block that is above the Group.
Screenshot 2020-02-14 at 12 26 33

@chipsnyder
Copy link
Contributor

@dratwas Good call out. My solution does work for Group Block, which is the primary way I was testing it.

@pinarol
Copy link
Contributor Author

pinarol commented Feb 26, 2020

@chipsnyder could you push your changes to a remote branch and link them here?

@chipsnyder
Copy link
Contributor

chipsnyder commented Feb 27, 2020

@chipsnyder could you push your changes to a remote branch and link them here?

Yup no problem!

The work from where I left off is available in these branches:
gutenberg-mobile issue/1413-floatingToolbar
gutenberg issue/1414-floatingToolbar-react

@iamthomasbishop
Copy link
Contributor

As a bit of an update, we are still struggling to get the positioning aspects of the toolbar to cooperate in a scalable way, so per some discussion amongst the team (@pinarol @chipsnyder), we are going take a slightly different approach.

  • Positioning will move to become "fixed" to the top of the block toolbar, centered, with ~8px space in between.
  • The toolbar will remain fixed in this position while the user is scrolling.
  • Bonus points if we can do the following: When the user taps on the breadcrumbs, scroll the user to the selected block.
  • Contents of the toolbar aren't changing at all. Still showing the up icon, separator, and breadcrumbs (parent block icon, nesting icon, selected block icon, and name of selected block).

Here is a rough sketch to demonstrate:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants