[iOS] Fabric: Added ScrollEndDragEvent for scrollEndDrag event #48319
Conversation
facebook-github-bot
added
the
CLA Signed
facebook-github-bot
added
the
Shared with Meta
| @@ -530,9 +530,8 @@ - (BOOL)_shouldDisableScrollInteraction | |||
| return NO; | |||
| } | |||
|
|
|||
| - (ScrollViewEventEmitter::Metrics)_scrollViewMetrics | |||
| - (void)_setScrollViewMetrics:(ScrollViewEventEmitter::Metrics &)metrics | |||
There was a problem hiding this comment.
this approach is not really iOS-sy...
What do you feel about having two methods: _scrollViewMetrics and _scrollViewMetricsWithVelocity:(CGPoint)velocity andTargetContentOffset:(CGPoint)targetContentOffset?
The first one, behaves as before. The second one does:
auto metrics = [self __scrollViewMetrics];
metrics.targetContentOffset.x = targetContentOffset->x;
metrics.targetContentOffset.y = targetContentOffset->y;
metrics.velocity.x = velocity.x;
metrics.velocity.y = velocity.y;
return metrics;
This should also minimize the changes in RCTScrollViewComponentView.
...s/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm
Show resolved
Hide resolved
| dispatchScrollViewEvent("scrollEndDrag", scrollEvent); | ||
| const ScrollEndDragEvent& scrollEvent) const { | ||
| dispatchEvent( | ||
| "scrollEndDrag", std::make_shared<ScrollEndDragEvent>(scrollEvent)); |
There was a problem hiding this comment.
I'm not sure we need to create a new pointer with make_shared here.
The type of the scrollEvent is already the right one. If previously we were passing the scrollEvent, we should be able to pass the scrollEvent even now, types should match.
There was a problem hiding this comment.
@cipolleschi Previously we call dispatchScrollViewEvent but it std::make_shared to create ScrollEvent shared ptr.
There was a problem hiding this comment.
ScrollEndDragEvent event is a subclass of ScrollEvent, we probably don't need to change this method at all, as you are allowed to pass a subclass to a method that accepts a superclass. The language should take care of it. (LSP - Liskov substitution principle)
Can you remove these changes and test if this works properly without these?
| dispatchScrollViewEvent("scrollEndDrag", scrollEvent); | ||
| const ScrollEndDragEvent& scrollEvent) const { | ||
| dispatchEvent( | ||
| "scrollEndDrag", std::make_shared<ScrollEndDragEvent>(scrollEvent)); |
There was a problem hiding this comment.
ScrollEndDragEvent event is a subclass of ScrollEvent, we probably don't need to change this method at all, as you are allowed to pass a subclass to a method that accepts a superclass. The language should take care of it. (LSP - Liskov substitution principle)
Can you remove these changes and test if this works properly without these?
| using EndDragMetrics = ScrollEndDragEvent; | ||
|
|
||
| void onScroll(const ScrollEvent& scrollEvent) const; | ||
| void onScrollBeginDrag(const ScrollEvent& scrollEvent) const; | ||
| void onScrollEndDrag(const ScrollEvent& scrollEvent) const; | ||
| void onScrollEndDrag(const ScrollEndDragEvent& scrollEvent) const; |
There was a problem hiding this comment.
Same for LSP, these changes should not be needed.
| @@ -26,7 +26,15 @@ void ScrollViewEventEmitter::onScrollBeginDrag( | |||
|
|
|||
| void ScrollViewEventEmitter::onScrollEndDrag( | |||
| const ScrollEvent& scrollEvent) const { | |||
| dispatchScrollViewEvent("scrollEndDrag", scrollEvent); | |||
| const auto* endDragScrollEvent = | |||
| dynamic_cast<const ScrollEndDragEvent*>(&scrollEvent); | |||
There was a problem hiding this comment.
@cipolleschi Hi, this is the another way by dynamic_cast scrollEvent to ScrollEndDragEvent if we removed those changes.
There was a problem hiding this comment.
uhm... sorry for the extra back and forth.
I tested locally and actually I prefer the previous version with the properly typed methods and without the cast.
I haven't realized that even if the dispatchScrollViewEvent takes an endDragScrollEvent, it then creates a shared pointer of type ScrollEvent: creating the new event pointer with explicit ScrollEvent type makes us lose the information of the actual subclass and therefore we can't serialize the event properly. :(
I modified the imported code, nothing required to do on your side! 😉
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
Fixes #42533 .
Changelog:
[IOS] [FIXED] - Fabric: Added ScrollEndDragEvent for scrollEndDrag event
Test Plan:
Repro please see #42533 .