Request toolbar layout after drawable loads #13876
Conversation
facebook-github-bot
added
CLA Signed
|
Works like a charm in my case! thanks very much. |
|
I would really appreciate it if someone from Facebook could take a look at this. This issue has been present for as long as I've been using React Native and it's a show stopper when trying to update the action bar dynamically on Android e.g. displaying/enabling a "check" (confirm) action item in response to valid text being entered in a text field. It's just a simple single line fix. The test repository doesn't do anything complicated, I just needed to bundle up some action item images somewhere for testing. All the relevant test code is contained in one file: https://github.com/Benjamin-Dobell/DynamicToolbar/blob/master/index.android.js I've been having a lot of trouble making release builds with my current patched Thanks |
|
Awww, 0.45 was released without this being considered. Please don't let this fix slip into obscurity... |
Benjamin-Dobell
force-pushed
the
dynamic-toolbar
branch
from
dcc7af6
to
528cc24
Compare
Benjamin-Dobell
force-pushed
the
dynamic-toolbar
branch
from
528cc24
to
bea1068
Compare
|
@Benjamin-Dobell Could you please rebase it on master? |
Benjamin-Dobell
force-pushed
the
dynamic-toolbar
branch
from
1b47976
to
6d141a1
Compare
|
@shergin Done. |
|
any news why this isn't merged? |
|
I can verify that this change also fixes #13780 - Please please please merge it. |
|
@Benjamin-Dobell one question: using your repro works well when I use the apk. but with "reac-native.cmd run-android" I get:
we see the same thing when we try to use your fork against our app. Do you know what that issue might be? |
|
In #15120 I verified that this commit cherry-picked on master is green on CI. Please consider to merge and release it. It's really important for us |
Benjamin-Dobell
force-pushed
the
dynamic-toolbar
branch
from
6d141a1
to
ecd379a
Compare
|
Something is flaky with the Travis build. But seems unrelated |
|
@forki Yeah, I just rebased in hope I'd see some green lights. 1 out of 2, that's not too bad 😉 Regarding the packager issue you encountered; maybe just make sure you're using yarn (there's a |
|
We use yarn and we did git clean -xdf |
|
@Benjamin-Dobell have you pushed your changes after rebasing? I don't see any new commits here. That should trigger Travis to run again. |
|
Yes he did. But Travis is flaky with unrelated issues |
facebook-github-bot
added
the
Import Started
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@shergin what is the status here? Anything we can do to help? |
facebook-github-bot
added
GH Review: accepted
and removed
GH Review: review-needed
labels
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
💋 |
|
any chance this can be merge into 0.47-stable? It's still not in RN 047-rc5 |
Summary: Fixes #11209 Updating action items in a `ToolbarAndroid`, or more specifically the native `ReactToolbar`, after the initial render presently will not always work as expected - typically manifesting itself as new action items not being displayed at all (under certain circumstances). This seems to be happening because Fresco gets back to us asynchronously and updates the `MenuItem` in a listener. However when a keyboard is displayed the `Toolbar` is in a weird state where updating the `MenuItem` doesn't automatically trigger a layout. The solution is to trigger one manually. This is a bit wacky, so I created a sample project: https://github.com/Benjamin-Dobell/DynamicToolbar `master` demonstrates the problem. Run the project, the toolbar action item is scheduled to update every 2 seconds. It works fine _until_ you give the `TextInput` focus. Once you give it focus the action item disappears and it never recovers (despite on-going renders due to state changes). You can then checkout the `fixed` branch, run `yarn` again, and see that problem is fixed. This branch is using a prebuilt version of React Native with this patch applied. Of course you could (and probably should) also modify `master` to use a version of RN built by the Facebook CI (assuming that's a thing you guys do). Closes #13876 Differential Revision: D5476858 Pulled By: shergin fbshipit-source-id: 6634d8cb3ee18fd99f7dc4e1eef348accc1c45ad
Motivation
Fixes #11209
Updating action items in a
ToolbarAndroid, or more specifically the nativeReactToolbar, after the initial render presently will not always work as expected - typically manifesting itself as new action items not being displayed at all (under certain circumstances).This seems to be happening because Fresco gets back to us asynchronously and updates the
MenuItemin a listener. However when a keyboard is displayed theToolbaris in a weird state where updating theMenuItemdoesn't automatically trigger a layout.The solution is to trigger one manually.
Test Plan
This is a bit wacky, so I created a sample project:
https://github.com/Benjamin-Dobell/DynamicToolbar
masterdemonstrates the problem. Run the project, the toolbar action item is scheduled to update every 2 seconds. It works fine until you give theTextInputfocus. Once you give it focus the action item disappears and it never recovers (despite on-going renders due to state changes).You can then checkout the
fixedbranch, runyarnagain, and see that problem is fixed. This branch is using a prebuilt version of React Native with this patch applied. Of course you could (and probably should) also modifymasterto use a version of RN built by the Facebook CI (assuming that's a thing you guys do).