-
Notifications
You must be signed in to change notification settings - Fork 24k
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 inspector reporting wrong frames when using DrawerLayoutAndroid
on the new arch
#44426
Fix inspector reporting wrong frames when using DrawerLayoutAndroid
on the new arch
#44426
Conversation
Base commit: 362abb9 |
...act-native/ReactAndroid/src/main/java/com/facebook/react/views/drawer/ReactDrawerLayout.java
Show resolved
Hide resolved
...act-native/ReactAndroid/src/main/java/com/facebook/react/views/drawer/ReactDrawerLayout.java
Outdated
Show resolved
Hide resolved
|
||
if (isOpen != stateDrawerOpened || onLeft != stateOnLeft || Math.abs(stateContainerWidth - containerWidth) > delta || Math.abs(stateDrawerWidth - drawerWidth) > delta) { | ||
WritableNativeMap newState = new WritableNativeMap(); | ||
newState.putBoolean("drawerOnLeft", onLeft); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about assigning these strings to private final static fields to eliminate risk of typos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's the convention, in other places plain strings are used as well:
Lines 467 to 468 in 362abb9
newStateData.putDouble("screenWidth", realWidth.toDouble()) | |
newStateData.putDouble("screenHeight", realHeight.toDouble()) |
Lines 1117 to 1118 in 362abb9
newStateData.putInt("mostRecentEventCount", mEditText.incrementAndGetEventCounter()); | |
newStateData.putInt("opaqueCacheId", mEditText.getId()); |
|
||
folly::dynamic getDynamic() const; | ||
MapBuffer getMapBuffer() const { | ||
return MapBufferBuilder::EMPTY(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really safe to remove an empty map buffer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is, as long as we're not calling stateWrapper.getStateDataMapBuffer()
, which we're not doing, instead calling getStateData
which returns the dynamic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's done exactly the same way in modal component:
react-native/packages/react-native/ReactCommon/react/renderer/components/modal/ModalHostViewState.h
Lines 44 to 46 in 362abb9
MapBuffer getMapBuffer() const { | |
return MapBufferBuilder::EMPTY(); | |
}; |
...nents/drawerlayout/android/react/renderer/components/drawerlayout/AndroidDrawerLayoutState.h
Outdated
Show resolved
Hide resolved
Superseded by #44707 |
Summary:
Due to
getContentOriginOffset
not being overridden byAndroidDrawerLayoutShadowNode
,findNodeAtPoint
function fromUIManagerBinding
was also taking into account the content of the drawer even if it was closed. This was also causing the frames always to be reported as if the drawer was on the left side.This PR adds a custom shadow node and native state, so that
getContentOriginOffset
can return the correct values depending on the state of the drawer.Fixes #44425
Changelog:
[ANDROID] [FIXED] - Fixed layout inspector reporting wrong frames when using
DrawerLayoutAndroid
on the new archTest Plan:
Tested on the reproducer from the issue.