Changing View's importantForAccessibility prop re-mounts all children recursively

[juhasuni]

Problem Description

Changing View's importantForAccessibility prop causes all children recursively to be re-mounted (unmounted and then mounted again). This in turn may cause serious performance issues.

It seems like the issue is caused by the use of React.Children.map function, which has an un-documented feature where each mapped child gets a new key prop. The map function is being used here:

https://github.com/microsoft/react-native-windows/blob/a54f8ccd6ad0a78815e8b89d5027a494e6614647/vnext/src/Libraries/Components/View/View.windows.js#L79

It might be possible to switch from map to using React.Children.forEach instead.

Affected libraries

The issue affects at least the most popular navigation library, React Navigation where the previous screen gets unmounted when you navigate to a new screen.

Specifically the issue happens here: https://github.com/react-navigation/react-navigation/blob/b28bb2b45ee39a8062020fba667e155c342c6629/packages/stack/src/views/Stack/Card.tsx#L520

The rest object passed as props to View contains importantForAccessibility prop.

Steps To Reproduce

Use the example code to below to reproduce the issue. Clicking the <Button /> causes the <Child /> to be re-mounted.

import React from 'react';
import { View, Button } from 'react-native';

const Container = ({ route, navigation }: Props) => {
  const [value, setValue] = useState<'auto' | 'no-hide-descendants'>('auto');

  return (<View style={{
    flex: 1, alignItems: 'center', justifyContent: 'center'
  }}>
    <View importantForAccessibility={value}><Child /></View>
    <Button title='Toggle' onPress={() => setValue((prev) => prev === 'auto' ? 'no-hide-descendants' : 'auto')} />
  </View >);
};

const Child = () => {
  useEffect(() => {
    return () => { console.log('Child just got unmounted'); };
  }, []);

  return <View />;
};

Expected Results

<Child /> shouldn't be unmounted when the parent's importantForAccessibility prop changes.

CLI version

7.0.4

Environment

System:
    OS: Windows 10 10.0.19044
    CPU: (12) x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
    Memory: 20.39 GB / 31.90 GB
  Binaries:
    Node: 16.16.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 8.11.0 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK:
      AllowDevelopmentWithoutDevLicense: Enabled
      Versions: 10.0.18362.0, 10.0.19041.0
  IDEs:
    Android Studio: Not Found
    Visual Studio: 16.11.32802.440 (Visual Studio Community 2019)
  Languages:
    Java: Not Found
  npmPackages:
    @react-native-community/cli: Not Found
    react: Not Found
    react-native: 0.68.2 => 0.68.2
    react-native-windows: 0.68.14 => 0.68.14
  npmGlobalPackages:
    *react-native*: Not Found

Target Platform Version

10.0.19041

Target Device(s)

Desktop

Visual Studio Version

Visual Studio 2019

Build Configuration

Debug

Snack, code example, screenshot, or link to a repository

No response

[juhasuni]

Here's a patch for @react-navigation/stack (>= 5) in case someone else bumps into this. Note that the original implementation passes lots of props to View that are not even recognized by the View (like onClose, onOpen ...).

diff --git a/node_modules/@react-navigation/stack/src/views/Stack/Card.tsx b/node_modules/@react-navigation/stack/src/views/Stack/Card.tsx
index 4ce3b31..9ae9c9f 100644
--- a/node_modules/@react-navigation/stack/src/views/Stack/Card.tsx
+++ b/node_modules/@react-navigation/stack/src/views/Stack/Card.tsx
@@ -517,7 +517,7 @@ export default class Card extends React.Component<Props> {
           // Make sure that this view isn't removed. If this view is removed, our style with animated value won't apply
           collapsable={false}
         />
-        <View pointerEvents="box-none" {...rest}>
+        <View pointerEvents={rest.pointerEvents || "box-none"} style={rest.style} accessibilityElementsHidden={rest.accessibilityElementsHidden}>
           {overlayEnabled ? (
             <View pointerEvents="box-none" style={StyleSheet.absoluteFill}>
               {overlay({ style: overlayStyle })}

[tero-paananen]

Issue exists after this PR https://github.com/microsoft/react-native-windows/pull/10280 into 0.67.

We do not have this problem on 0.65 but now on 0.68 and 0.69.

Note for @TatianaKapos

[tero-paananen]

Proposed solution - about something...

How about if we do not clone children in childrenWithImportantForAccessibility function in View.windows.js https://github.com/microsoft/react-native-windows/blob/6d351951e0ec461c6b70c7f8899861f03b7b8cba/vnext/src/Libraries/Components/View/View.windows.js#L123

We could send children prop and later in c++ side update accessible value

accessible={
              props.importantForAccessibility === 'no-hide-descendants'
                ? false
                : props.accessible
            }
            children={
               props.children
            }

We could set accessible for children in here https://github.com/microsoft/react-native-windows/blob/6d351951e0ec461c6b70c7f8899861f03b7b8cba/vnext/Microsoft.ReactNative/Views/FrameworkElementViewManager.cpp#L280

else if (propertyName == "accessible") {
      if (propertyValue.Type() == winrt::Microsoft::ReactNative::JSValueType::Boolean) {
        if (propertyValue.AsBoolean()) {
          ApplyAccessability(element, winrt::AccessibilityView::Content);
        } else {
          ApplyAccessability(element, winrt::AccessibilityView::Raw);
        }
      } else if (propertyValue.IsNull()) {
        ClearAccessability(element);
      }
    } 

void FrameworkElementViewManager::ApplyAccessability(winrt::Windows::UI::Xaml::FrameworkElement const& element,
  winrt::Windows::UI::Xaml::Automation::Peers::AccessibilityView const& value) {

  xaml::Automation::AutomationProperties::SetAccessibilityView(element, value);
  
  auto children = element.GetChildrenInTabFocusOrder();
  if (children != nullptr) {
    for (auto const& child : children)
    {
      xaml::Automation::AutomationProperties::SetAccessibilityView(child, value);

     // maybe recursively ?
      if (auto childElement = child.try_as<winrt::Windows::UI::Xaml::FrameworkElement>()) {
        ApplyAccessability(childElement, value);
       }
    }
  }
}

void FrameworkElementViewManager::ClearAccessability(winrt::Windows::UI::Xaml::FrameworkElement const& element) {


  element.ClearValue(xaml::Automation::AutomationProperties::AccessibilityViewProperty());
  auto children = element.GetChildrenInTabFocusOrder();
  if (children != nullptr) {
    for (auto const& child : children)
    {
      child.ClearValue(xaml::Automation::AutomationProperties::AccessibilityViewProperty());
      
      // maybe recursively ?
      if (auto childElement = child.try_as<winrt::Windows::UI::Xaml::FrameworkElement>()) {
        ClearAccessability(childElement);
      }
    }
  }
}

I had also to import iterator to get our from error "std::iterator_traits<<error-type>>" has no member "iterator_category"

// TODO: not sure about this and the error
#include <iterator>

[TatianaKapos]

@chiaramooney @AgneLukoseviciute, I believe you guys were looking into "no-hide-descendants" (correct me if I'm wrong) but would this proposed solution help with that?

[AgneLukoseviciute]

I was seeing some strange behavior from gallery when a touchable component is involved, I haven't narrowed down precisely what the bug is so haven't opened up an issue yet. This doesn't immediately strike me as related that, but I do like the idea of having this logic in FrameworkElementViewManager.

[tero-paananen]

There is another way to get children if element is Xaml::Controls::Panel but it seems that element.GetChildrenInTabFocusOrder(); may get same children.

if (auto panel = element.try_as<winrt::Windows::UI::Xaml::Controls::Panel>()) {
    auto children = panel.Children();
    if (children != nullptr) {
      for (auto const& child : children)
      {
        xaml::Automation::AutomationProperties::SetAccessibilityView(child, value);
      }
    }
  }

I am not WinRT C++ expert so my code examples are only best guess...

[chiaramooney]

Yep all makes sense to me based on current implementation. We can investigate a native solution approach in order to improve performance for the prop.

[gtRfnkN]

Thank you, @juhasuni, your patch resolved the issue for me 🎉

Here's a patch for @react-navigation/stack (>= 5) in case someone else bumps into this. Note that the original implementation passes lots of props to View that are not even recognized by the View (like onClose, onOpen ...).

I'd like to add a few keywords to make this easier to find: after updating from 0.63 to 0.69, the background image of our app was disappearing / flickering when navigating to another screen. This was more obvious for remote images than for local images which makes sense due to the remount. This could be related to the gallery issue mentioned by @AgneLukoseviciute.

[chiaramooney]

Not something we plan to address on the Paper renderer as we move towards Fabric. Not sure if this behavior is still applicable on the Fabric renderer. If so, we'd want a fresh analysis.