Fix measureInWindow feature parity between iOS and Android #29691
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: 2e88b25 |
Base commit: 2e88b25 |
gedeagas
changed the title
|
@dulmandakh @safaiyeh @ShikaSD can anybody from the core team confirm this? i still get this issue on latest release. |
|
What is require to merge the PR? I think would be great, if we can fix it, as currently the implementation on Android is not in sync with iOS, which behaves correctly. |
|
Just time, the team is looking into PRs recently. Maybe @lunaleaps can bump this one. |
|
My concern is that we might be depending on this behavior somewhere.. but let me ask around |
|
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -712,7 +712,7 @@ public synchronized void measureInWindow(int tag, int[] outputBuffer) { | |||
| Rect visibleWindowFrame = new Rect(); | |||
| v.getWindowVisibleDisplayFrame(visibleWindowFrame); | |||
| outputBuffer[0] = outputBuffer[0] - visibleWindowFrame.left; | |||
| outputBuffer[1] = outputBuffer[1] - visibleWindowFrame.top; | |||
| outputBuffer[1] = outputBuffer[1] + visibleWindowFrame.top; | |||
There was a problem hiding this comment.
From discussion with @yungsters, why are we adding visibleWindowFrame.top versus removing this line altogether?
Also from the PR that introduced this change: #28449
visibleWindowFrame.top might include more than just status bar height.. is that expected that we want this included in the measure values?
Has this change been tested in split screen? What should the expected behavior be for that -- say for a split screen of:
---
A
--
B
---
What should B's measureInWindow return?
There was a problem hiding this comment.
Hi @lunaleaps, let me check again. This PR is more than 1 year old already and I forget about the context. Let me check and confirm your concern and test the behavior on split-screen.
There was a problem hiding this comment.
Hi @lunaleaps i will work on this on the weekend, sorry for the delay.
There was a problem hiding this comment.
No worries, thanks for working on this!
|
This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days |
github-actions
bot
added
the
Stale
|
This PR was closed because the author hasn't provided the requested feedback after 7 days. |
Summary
This PR aims to fix a feature parity problem between iOS and Android. The iOS return y with status bar height while the android implementation does not. This issue is first discovered on #19497 and mentioned on RN Issue Triage Group 2 #24029.
Related Issue:
resolve #29638 #19497
visibleWindowFrame.topreturns the StatusBar height.outputBuffer[1]does not contain the StatusBar height.Changelog
[Android] [Fixed] - Fix measureInWindow y axis not including StatusBar
Test Plan
RNTester for Android
This is the correct result when
measureInWindowis called on hello.yis44from the top and24from the status bar. (Hellohave a 20 margin-top and status bar height is 24 ).