React-native-windows: [RNTester] button at bottom of page looks wrong

[asklar]

Looks like the image is being "centered" around its top, left (which is the XAML behavior) as opposed to the center of the image (default for web):

[NickGerleman]

Is this islands specific?

Locally I haven't seen the issue in either the normal Playground or Rex.

[NickGerleman]

Hmm, I'm able to repro this in playground now, but not with Rex, and versions of Playground built last week didn't exhibit this behavior.

It looks like something broke very recently.

[NickGerleman]

No apparent changes to either the RNTester component for this, or the Image component.

Repros both when using packager and a release bundle for XAML. Doesn't repro in either case for NetUI running much older native bits. Timing lines up with potentially happening during the last RN integration.

[NickGerleman]

Still reproes on 0cb98f4d8aae7434b5be41970e69d11ae9b980b2 so not a result of the recent integration

[NickGerleman]

Does not repo on 6c96303249b236b1a828318239ae6cb541a31f8e (October 31st nightly build)

[NickGerleman]

Does repro on b9a14fb42022c24c6241da393cbec2543de3c856 (November 12th nightly build)

[NickGerleman]

Does not repro on 7ab4c7e0d778b3e011890fdaa644177ea7a9b1cd (November 6th nightly build)

[NickGerleman]

And... the lucky winner is @rectified95 and @kmelmon 😀

Reverting "#6451: Fix TextInput not rendering sometimes" from November 9th seems to fix the issue. Somehow setting TextInputViewManager to force layout is causing this component to no longer be positioned consistently with other platforms.

[NickGerleman]

Relevant sources for the button in RNTesterNavbar.js

const BookmarkTab = ({handleNavBarPress, isBookmarkActive, theme}) => (
  <View style={styles.centerBox}>
    <Image
      style={styles.centralBoxCutout}
      source={require('./../assets/bottom-nav-center-box.png')}
    />
    <View style={styles.floatContainer}>
      <Pressable
        testID="bookmarks-tab"
        onPress={() => handleNavBarPress({screen: 'bookmarks'})}>
        <View
          style={[styles.floatingButton, {backgroundColor: theme.BorderColor}]}>
          <Image
            style={styles.bookmarkIcon}
            source={
              isBookmarkActive
                ? require('../assets/bottom-nav-bookmark-fill.png')
                : require('../assets/bottom-nav-bookmark-outline.png')
            }
          />
        </View>
      </Pressable>
    </View>
  </View>
);

const styles = StyleSheet.create({
  floatContainer: {
    flex: 1,
    zIndex: 2,
    alignItems: 'center',
  },
...
  floatingButton: {
    top: -20,
    width: 50,
    height: 50,
    borderRadius: 500,
    alignContent: 'center',
    shadowColor: 'black',
    shadowOffset: {
      height: 5,
      width: 0,
    },
    shadowOpacity: 0.9,
    shadowRadius: 10,
    elevation: 5,
  },
  bookmarkIcon: {
    width: 30,
    height: 30,
    margin: 10,
  },
 ...
  centralBoxCutout: {
    height: '100%',
    width: '100%',
    position: 'absolute',
    top: 0,
  },
  centerBox: {
    flex: 1,
    height: 65,
  },
...
});

[NickGerleman]

From @kmelmon

the symptom is making me think that XAML is somehow applying its default layout policy and stomping over whatever yoga is trying to do

Context in https://github.com/microsoft/react-native-windows/issues/2642 related to why we need to force layout. Related to not doing layout throughout the entire tree for perf reasons.

Theory is that the extra UpdateLayout may be causing side effects that propagate into different areas.

[NickGerleman]

Going to try to take a quick stab at understanding more about this one

[NickGerleman]

The bookmarkIcon Image width/height/margin seems to be correctly interpreted by Yoga and propagated to positions applied to the XAML element. This visually does not show when comparing to its parent. I.e. ViewPanel.Left and ViewPanel.Top are not visually accounted for when comparing to the parent floatingButton view.

Some amount of dirtying does seem to fix the issue. E.g. changing the width/height in the live property editor for just the leaf node is enough to reset the image to the correct position. I'm not sure what all that forces however.

[kmelmon]

Thanks @NickGerleman. Those ViewPanel properties are our own custom properties, they are not pushed into XAML until ViewPanel::ArrangeOverride() is called. Can you confirm this is happening? It would be the parent that arranges the child at its top/left that we're looking for.

[NickGerleman]

Figured out a trick to break on the first run of ArrangeOverride for floatingButton. It seems to have a stable react tag of 473 on each startup, so I broken on VM creation of a ViewPanel from that tag, so I could track the this ptr ViewPanel we create for it.

At time of first arrange, left and top of the image are both evaluated to zero, and we don't ever arrange again before showing things.

So, it seems like we arranged before running the real Yoga-based layout, and then never tried to arrange again after?

[NickGerleman]

Can confirm that left/top properties are correctly assigned in the first pass of DoLayout over the image node. So, the Arrange did happen before we were able to run layout on it.

[kmelmon]

Interesting. It's weird that adding an extra measure of a cousin element is having this side-effect. The arrange pass in XAML is incremental, based on dirty flags. There could be a dirty flag bug in XAML, or something else going on that makes XAML think the relevant sub-tree is no longer dirty for arrange when it actually is dirty.

When we set Top/Left on ViewPanel, it calls InvdliateArrange which seems correct. I'd be curious to see if calling InvalidateMeasure has any effect on the bug.

I'll take a crack at debugging through the XAML side of this and see if I can spot what's going on.

[NickGerleman]

I'd be curious to see if calling InvalidateMeasure has any effect on the bug.

No change in behavior when changing this.

[kmelmon]

Bug understood:

This code is making an incorrect assumption:

/*static*/ void ViewPanel::SetTop(xaml::UIElement const &element, double value) {
  element.SetValue(TopProperty(), winrt::box_value<double>(value));
  element.InvalidateArrange();

This is making the assumption that the ViewPanel is responsible for positioning itself. This isn't correct. It is the parent's responsibility for positioning its children. Thus we are invalidating the wrong element.

I also noticed we're using a custom DP to store the top/left. This means only ViewPanel knows how to position child ViewPanels. We should use Canvas.Top/Left instead, as that will allow any parent to position a child ViewPanel. It seems likely this is also causing issues.