microsoft / react-native-windows

A framework for building native Windows apps with React.
https://microsoft.github.io/react-native-windows/
Other
16.27k stars 1.14k forks source link

Root-level perspective transform causes blurry Popups and Flyouts #4466

Open aschultz opened 4 years ago

aschultz commented 4 years ago

PopupViewManager and FlyoutViewManager attempt to work around a z-index shadowing issue by forcing a translation along the Z axis. This interacts poorly with the PerspectiveTransform3D set on the React root view (ReactControl), causing Popups and Flyouts to scale and text to become blurry as their Z position increases.

The code in ReactControl.cpp and ReactRootControl.cpp says:

    // Xaml's default projection in 3D is orthographic (all lines are parallel)
    // However React Native's default projection is a one-point perspective.
    // Set a default perspective projection on the main control to mimic this.

However, while React Native's "transform" property supports a perspective setting, its not required. An orthographic transform should be possible.

I suspect this root-level transform should be removed in favor of components independently setting perspective if needed.

chrisglein commented 4 years ago

Hacky workaround exists to unblock partner (to remove the transform), not desirable. This is preventing them from deforking some files.

This needs some thought into what the right fix is. Composition doesn't support opting out of perspective in a scoped way. Or only add perspective to components that use a "true" 3D transform. Is it possible to show under a different root than the one we're using?

aschultz commented 4 years ago

@chrisglein The TransformMatrix property that FrameworkElementViewManager uses to apply React transforms accepts a 4x4 matrix. Is that not sufficient for performing 3D transforms? Why is the root level PerspectiveTransform3D needed?

chrisglein commented 4 years ago

@chrisglein The TransformMatrix property that FrameworkElementViewManager uses to apply React transforms accepts a 4x4 matrix. Is that not sufficient for performing 3D transforms? Why is the root level PerspectiveTransform3D needed?

I'll let @kmelmon weigh in here as he had the expertise at the triage meeting today.

chrisglein commented 4 years ago

@chrisglein The TransformMatrix property that FrameworkElementViewManager uses to apply React transforms accepts a 4x4 matrix. Is that not sufficient for performing 3D transforms? Why is the root level PerspectiveTransform3D needed?

I'll let @kmelmon weigh in here as he had the expertise at the triage meeting today.

kmelmon commented 4 years ago

Discussed with George Gao.

I said: Hey George,

We have an issue in react-native: https://github.com/microsoft/react-native-windows/issues/4466#issuecomment-608079023

The problem here is that in react-native, there’s a transform property which can be 3D. It’s expected to have perspective automatically applied by the native platform. We map this property to the XAML TransformMatrix property. But of course XAML doesn’t provide perspective by default, so these transforms end up with an orthographic projection, which is inconsistent with what the other platforms do. To fix this and be consistent with other platforms, we added a default perspective transform on the root of the entire react-native tree.

Now for the fun part… This perspective transform is causing issues for shadow-casting elements, which put a Z translation on themselves. These elements really shouldn’t have perspective applied to them, as it moves them closer to the user’s eye. It also causes text to render blurry as the text rendering code is apparently not smart enough to account for perspective.

I’m trying to figure out how to meet both requirements. One idea is to detect that an element has 3D in its transform, and only in this case, add perspective to just that element’s overall transform (we control the XAML transform completely and it’s not visible to the app).

So in a case like this:

Popup would not get any perspective. DudeWith3D would get a perspective.

Of course this isn’t a perfect solution but might be good enough.

Another idea would be to force Popups to be windowed, as I think in this case we won’t apply any perspective to it.

Do you have any other ideas?

And George said:

Sounds like popups use a Z translation as an implementation detail of shadows, and this detail is leaking when putting perspective on the tree.

Does react-native on Xaml care about shadows in its popups? Is there a way to opt out of shadows and the Z translation?

Otherwise doing what you suggest is the workaround. It can get tricky because if someone has a popup under their 3D content, they’ll see a perspective on their popup again. We’ll also have to do bookkeeping to make sure we don’t double-apply a perspective.

Another thing that you can try playing with – what happens if there’s a flattening transform above the popup (Z scale = 0). That will remove the perspective (along with all 3D) in that subtree, but does it prevent the shadow from showing as well?

lamxdoan commented 4 years ago

As an FYI, we are seeing this issue with Combobox as well since it looks like it's using a popup.

image

image

kmelmon commented 4 years ago

While fixing this, we'll need to also address the performance aspect of this issue. Putting a perspective transform on the root of the tree makes us lose direct flip for game-like apps that just have one big swapchain.

Jared Henderson reported this:

Hi, Keith/Chris. It looks like the Transform3D that’s added to the root XamlView is causing significant perf issues while streaming a game. When I removed it, the app appears to layout fine, but I’m not sure what I might be breaking by removing it.

Do you know why this is needed, and can you think of a workaround that wouldn’t require XAML code to run every frame?

From further down in this thread;

The problem is that this app, which owns the swapchain:

Windows.UI.Core.CoreWindow|Xbox Game Streaming (Test App)

Seemingly has a 3d transform in its tree:

Matrix4x4 = { { 1.000, 0.000, 0.000, 0.000} { 0.000, 1.000, 0.000, 0.000} { -1.765, -1.176, 1.000, -0.001} { 0.000, 0.000, 0.000, 1.000} }

Which makes for a final overall transform for the swapchain of:

Matrix4x4 = { { 1.562, 0.000, 0.000, 0.000} { 0.000, 1.562, 0.000, 0.000} { -1.765, -1.176, 1.000, -0.001} { 0.000, 156.250, 0.000, 1.000} }

Which isn’t a 2D translate-or-scale, nor is it 2D axis aligned, and can’t be represented by a transform on an MPO plane. Thus DWM composes.

Thanks, Jared

kmelmon commented 4 years ago

An interesting comment from Alex as he investigated the TransformExample RNTester page:

So I wanted to validate whether the perspective transform could be removed at all so I launched RNTester and navigated to the TransformExample page which is the page I had used to validate the original fix to add the transform. But then I noticed that even without the perspective transform at the root, the rotating block example still drew with perspective!! Very unexpected 😊 so I wanted to find out why that was. Long story short I tracked it down to the fact that the TransformExample doesn’t use the native driver for animations.

This commit added useNativeDriver: false: _animate = () => { this.state.theta.setValue(0); Animated.timing(this.state.theta, { toValue: 360, duration: 5000, useNativeDriver: false, }).start(this._animate); };

What this means is that when useNativeDriver is false, the JS code is computing a perspective matrix and sending it to native. This could be how we solve this issue - reuse that code for the non-animating case as well. This would add perspective only to nodes that use a 3d transform.

acoates-ms commented 4 years ago

Except that useNativeDriver: false is being deprecated. It relies on setNativeProps which doesn't work in fabric. So Facebook is working on removing usages of useNativeDriver:false. I know they have been running with asserts on usages of it internally for a while now.

@TheSavior to keep me honest.

kmelmon commented 4 years ago

I'm only suggesting that we possibly reuse whatever technique is being used by the JS to automatically detect a 3d transform and apply a perspective to the matrix. I don't care if useNativeDriver is being deprecated, and would actually be happy if that happened.

elicwhite commented 4 years ago

@acoates-ms, yep! Kept honest.

@kmelmon The JS driver calls React's forceUpdate on every frame, which sends the current prop value from JS to native through the normal path.

I don't know this part for sure, but I'd expect the above to mean that you can find the part handling the 3D transform in the viewConfig for that style prop.

kmelmon commented 4 years ago

I debugged through this. The JS code doesn't actually add any perspective... The TransformExample test page has a 3D example that provides a perspective property in the transform, and that explains why perspective showed up even with our root perspective disabled. If I remove the perspective, and also disable our root perspective, there is no perspective. This puts us back to square 1, but at least I understand now.

aschultz commented 3 years ago

@kmelmon You wrote:

The problem here is that in react-native, there’s a transform property which can be 3D. It’s expected to have perspective automatically applied by the native platform. We map this property to the XAML TransformMatrix property. But of course XAML doesn’t provide perspective by default, so these transforms end up with an orthographic projection, which is inconsistent with what the other platforms do. To fix this and be consistent with other platforms, we added a default perspective transform on the root of the entire react-native tree.

My understanding from playing with this page is that transforms should be orthographic by default, and only change to perspective when a {perspective: <value>} entry is added to the transform array. That seems born out in the code I see when searching the RN repo.

Am I missing something here? It seems like each view should just handle its own transform, and that adding perspective at the root would cause incorrect behavior.

Related, I also noticed in this commit they are handling the RN zIndex prop explicitly through a sorting mechanism in order to avoid causing parallax effects.