Basic implementation of RCTModalHostView #1996
Conversation
|
|
|
I believe the AppVeyor build failed since I apparently didn't push the RNTester submodule, which I have since pushed |
| public ReactModalShadowNode() | ||
| : base(false, true) | ||
| { | ||
| ApplicationView.GetForCurrentView().VisibleBoundsChanged += ApplicationViewOnVisibleBoundsChanged; |
There was a problem hiding this comment.
ApplicationView.GetForCurrentView().VisibleBoundsChanged [](start = 12, length = 56)
This won't work, unfortunately. This code is executed by layout manager thread.. It is a "Dispatcher" thread but with no active window.
You may have fallen into the trap of the DEBUG build that uses same main UI thread as LM thread as a workaround for some OS/VS/Debugging issue (Release has the CREATE_LAYOUT_THREAD defined)
There was a problem hiding this comment.
I just tested this running the Visual Studio release configuration, which I can see is enabling the CREATE_LAYOUT_THREAD, but the VisibleBoundsChanged event is still firing and resizing the dialog, am I missing something?
There was a problem hiding this comment.
After some more testing on a phone I noticed a few sizing issues. While fixing these issues I found a way to remove the VisibleBoundsChanged event.
There was a problem hiding this comment.
@howlettt I tested your branch since you made me curious about your findings. Even though you removed the bounds change, you still have the "ApplicationView.GetForCurrentView().VisibleBounds" when adding the child.
It's true that he visible bounds of the LM view seem to mimic the main view, looks like a code hazard to me.
The problem becomes more visible when you open new views. For ex, in RNTester:
- Go to last item in list and choose "Multi Window", and then "New top level window". That creates a clone like RNTester window.
- Size the 2 windows differently.
- You'll see that LMView.VisibleBounds returns the values for main view even if you actually present the modal from the second.
This is not correct conceptually, though to be honest I played with various knobs and things look nice on screen in all cases (you may know better than me when those dimensions returned by the view really matter).
For RootViews RN cheats since the sizes come from JS side and UIImplementation just shoves them into the shadow nodes.
There was a problem hiding this comment.
Hmm that could explain a oddity I saw (and had to work around) with the VisibleBounds returning slight incorrect sizing. The VisibleBounds call in the shadow node is currently only required for phone, desktop works fine without it. I haven't been able to find any other way to get phone to play nice. This solution is based off the Android code ModalHostShadowNode.java where they do a similar sizing call. I'll play with it some more to see if I get an idea of how to remove this VisibleBounds call, but currently I don't have any theories.
|
I tried using this implementation with ReactNative The modal gets displayed with its RN content. But the RN is not reacting - for example click events are not performed. It seems that the modal node is not being orchestrated by React Native. Shouldn't the modal be positioned under the Am i missing anything or is the PR still missing that part? |
|
@LuisMiguelFilipe I've tested the modal using RN 0.55.0 and click events work. I specifically had to implement a TouchHandler to get the events firing. Would you be able to update to 55 and see if that fixes the events? |
|
@reseul I've pushed the best workaround that I've come up with where the shadow node no longer requires the visible bounds |
| /// Instantiates the <see cref="ReactModalShadowNode"/>. | ||
| /// </summary> | ||
| public ReactModalShadowNode() | ||
| : base(false, true) |
There was a problem hiding this comment.
true [](start = 25, length = 5)
Did you use "delegated layout" on purpose? It would make me happy, that would mean there's more than one usage scenario for that.
There was a problem hiding this comment.
I had originally used it on purpose, but I don't recall exactly why, so I did some testing and it seems it's not required anymore. Sorry for getting your hopes up hah
| } | ||
|
|
||
| /// <summary> | ||
| /// Shows the dialog if not visible otherwise updates it's properties |
There was a problem hiding this comment.
it's [](start = 62, length = 4)
small typo
| contentSize.Height -= inputPane.OccludedRect.Height; | ||
|
|
||
| uiManagerModule.ActionQueue.Dispatch(() => | ||
| { |
| // If multiple events have been dispatched, ignore all but the newest | ||
| if (currentCount == _resizeCount) | ||
| { | ||
| uiManagerModule.UIImplementation.UpdateRootNodeSize(tag, contentSize.Width, contentSize.Height); |
There was a problem hiding this comment.
uiManagerModule.UIImplementation.UpdateRootNodeSize(tag, contentSize.Width, contentSize.Height); [](start = 20, length = 96)
You should add a small comment pointing to the fact the tag is not really a root view, but it kind of works like one in some regards.
|
@howlettt This looks good, thanks for addressing the comments. |
|
Awesome, thanks @reseul |
|
Hello, how can I use this modal or how can I update the react-native-windows files to include your implemented modal ? |
|
This modal implementation was done before the move to C++ so it's no longer supported. |
My first stab at creating a modal implementation #618