Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alleviate excessive layout jittering when resizing window #439

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

tido64
Copy link
Member

@tido64 tido64 commented Jun 7, 2020

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

Resize events overwhelm RN, causing seemingly infinite jittering when the window is resized. By throttling layout, we can help RN catch up. Resolves #322.

Changelog

[macOS] [Fixed] - Alleviate excessive layout jittering when resizing window

Test Plan

resize-fix

Microsoft Reviewers: Open in CodeFlow

@tido64 tido64 requested a review from tom-un as a code owner June 7, 2020 20:30
Copy link

@anandrajeswaran anandrajeswaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 0.25s delay in layout is usually deeply perceptible. We should not sacrifice that in the layout of simple scenarios and should instead look for the actual inefficiencies in layout when we have a scrollview

@tido64
Copy link
Member Author

tido64 commented Jun 7, 2020

From investigating issue #322, the cause of the jittering comes from calling updateAvailableSize which queues work on the UI manager queue. Unless we can optimize away the thread jump, I'd argue this is still a lot better than the current behaviour:

@ghost ghost removed the Needs: Author Feedback label Jun 7, 2020
@elicwhite
Copy link
Collaborator

Is resizing done purely in native? Or is resizing the window causing events into JS like onLayout that is triggering additional re-renders that jitter? If the issue is some back and forth between native and JS, perhaps we would want to throttle the events to JS on resizing but let everything on the native side happen live?

@tom-un
Copy link
Collaborator

tom-un commented Jun 9, 2020

I tried throttling the number of onLayout events fired in RCTUIManager.m and even with intervals of seconds the layout jitter still occurs.

I found, as @tido64 discovered, the excessive thread hops from native to JS is during the barrage of calls on main thread by UIKit to [RCTRootContentView layoutSubview] -> [RCTRootContentView updateAvailableSize] -> [RCTUIManager setAvailableSize:] -> [RCTUIManager _executeBlockWithShadowView:]. The final call queues blocks to the JS thread. Perhaps a coalescing of these thread hops: only queue the latest update after an interval.

@tido64
Copy link
Member Author

tido64 commented Jun 9, 2020

Perhaps a coalescing of these thread hops: only queue the latest update after an interval.

I did try this initially but ran into an issue with RN layout randomly not being updated properly if resized quickly:

resize-bug

Where we throttle now in the current iteration does not exhibit this behaviour.

@tom-un
Copy link
Collaborator

tom-un commented Jun 18, 2020

I tried throttling just the calls across the bridge in [RCTRootContentView updateAvailableSize]. At first I thought I looked very promising but then I would hit cases occasionally where, as you describe above, the final layout would look as if it missed a frame. I could not figure why. Just for the record, this is what I changed in updateAvailableSize:

- (void)updateAvailableSize
{
  if (!self.reactTag || !_bridge.isValid) {
    return;
  }

#if !TARGET_OS_OSX // [TODO(macOS ISS#2323203)
  [_bridge.uiManager setAvailableSize:self.availableSize forRootView:self];
#else
  CGSize availableSize = [self availableSize];
  if (!_throttleUpdateAvailableSize) {
    NSTimeInterval interval = [[NSDate date] timeIntervalSinceDate:_lastUpdateAvailableSize];
    if (interval >= RCT_UPDATE_AVAILABLE_SIZE_THROTTLE) {
      _lastUpdateAvailableSize = [NSDate new];
      NSLog(@"*** non-deferred update");
      [_bridge.uiManager setAvailableSize:availableSize forRootView:self];
    } else {
      _throttleUpdateAvailableSize = YES;
      __weak typeof(self) weakSelf = self;
      dispatch_after(dispatch_time(DISPATCH_TIME_NOW, RCT_UPDATE_AVAILABLE_SIZE_THROTTLE * NSEC_PER_SEC), dispatch_get_main_queue(), ^{
        typeof(self) strongSelf = weakSelf;
        if (strongSelf != nil) {
          strongSelf->_throttleUpdateAvailableSize = NO;
          CGSize currentAvailableSize = [strongSelf availableSize];
          if (!CGSizeEqualToSize(currentAvailableSize, strongSelf->_lastAvailableSize)) {
            NSLog(@"**** Should not happen ****");
          }
          NSLog(@"*** deferred update");
          [self->_bridge.uiManager setAvailableSize:strongSelf->_lastAvailableSize forRootView:strongSelf];
        }
      });
    }
  }
  _lastAvailableSize = availableSize;
#endif // ]TODO(macOS ISS#2323203)
}

Then I tried scoping your solution here to just impact RCTRootView.m: moved the layout override from RCTUIView to RCTRootView. It works, it doesn't skip any layout steps, but it feels like a safer, more scoped change than doing the workaround to all the RCTViews. I propose we go with this scoped fix for now and revisit the layout when Fabric is enabled for macOS.

@tido64
Copy link
Member Author

tido64 commented Jun 18, 2020

Thanks, @tom-un. I've moved the throttling to RCTRootView as suggested.

@tido64
Copy link
Member Author

tido64 commented Jun 25, 2020

@anandrajeswaran: Can you please take another look? I think this is as good as we can get it with the current limitations.

@anandrajeswaran
Copy link

@tido64 I'm still a bit nervous about it because it seems like it impacts all cases while the problem only manifests when a scrollview is in the hierarchy? But if the consensus is that this behavior feels that much better for a relatively common case and should be easy to undo as we get to fabric then it seems reasonable to go ahead with it

@elicwhite
Copy link
Collaborator

On web I think it would be expected to throttle resize events vs all layout events. Is that something we are able to intercept and throttle in macOS?

@anandrajeswaran
Copy link

On web I think it would be expected to throttle resize events vs all layout events. Is that something we are able to intercept and throttle in macOS?

@TheSavior the system automatically does that for you using WindowServer. The application resizing slowly doesn't block the user interaction, but the quicker it can layout the quicker the user can see what layout at the new size would look like. You can see that in the video in this PR - the window background resizes smoothly but it's just the contents that flicker because layout in the canvas lags so far behind and the system is heavily throttling us.

@tom-un tom-un merged commit 6386729 into microsoft:master Jun 29, 2020
@tido64 tido64 deleted the tido64/fix-resize-jitter branch June 29, 2020 21:34
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Mar 5, 2022
…crosoft#439)"

This reverts commit 6386729.

# Conflicts:
#	React/Base/RCTRootView.m
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 2, 2022
…crosoft#439)"

This change reverts microsoft#459 - but still tries to address the original issues:
- microsoft#422
- microsoft#322

This also addresses an issue when programmatically resizing windows where the RCTRootContentView may end up at the wrong size because an in-flight layout gets resolved after the resize on the main thread.
We now keep sync dispatch on the shadow queue for live resizing windows (to prevent tearing) but also dispatch async (as done on iOS) so the latest new size is sure to win.
The block has a check to bail if the size doesn't change, so this isn't a perf drain running the block twice.
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 2, 2022
…crosoft#439)"

This change reverts microsoft#459 - but still tries to address the original issues:
- microsoft#422
- microsoft#322

This also addresses an issue when programmatically resizing windows where the RCTRootContentView may end up at the wrong size because an in-flight layout gets resolved after the resize on the main thread.
We now keep sync dispatch on the shadow queue for live resizing windows (to prevent tearing) but also dispatch async (as done on iOS) so the latest new size is sure to win.
The block has a check to bail if the size doesn't change, so this isn't a perf drain running the block twice.
christophpurrer added a commit to christophpurrer/react-native-macos that referenced this pull request Aug 4, 2022
…crosoft#439)" (microsoft#1318)

This change reverts microsoft#459 - but still tries to address the original issues:
- microsoft#422
- microsoft#322

This also addresses an issue when programmatically resizing windows where the RCTRootContentView may end up at the wrong size because an in-flight layout gets resolved after the resize on the main thread.
We now keep sync dispatch on the shadow queue for live resizing windows (to prevent tearing) but also dispatch async (as done on iOS) so the latest new size is sure to win.
The block has a check to bail if the size doesn't change, so this isn't a perf drain running the block twice.

Co-authored-by: Scott Kyle <skyle@fb.com>
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 9, 2022
Summary:
# Problem
Sometimes, at the end of a live resize, the window size is not propagated to resize the content. This leads to situations like this one, where the content is accidentally bigger than the window (see the "Type a message" textbox):
https://www.internalfb.com/intern/px/p/24pVJ/

# Solution
This diff schedules a relayout at the end of a live resize to ensure that the latest value is used.

# How did I discover this?
First I added [logging to product code here](https://www.internalfb.com/code/fbsource/[fbdb38f5d118608951d4807629af599b638f6b74]/xplat/archon/packages/rn/src/messenger/ui/chatNavigation/ChatNavigationContainer.tsx?lines=100) and determined that the last window dimensions value logged is always correct.

I then added a lot of native logging to log the sizes of the RootView and RootContentView, and the events being fired. (Full diff with all my logging in V0.1 of D36719158)

From the logging I determined that the problem ALWAYS occurred when `onWindowDidEndLiveResize` was the most recent event fired. The size in that event was always correct, but [`setAvailableSize`](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/js/react-native-github/React/Modules/RCTUIManager.m?lines=393) was not called. `onWindowDidResize` events did result in `setAvailableSize` being called.

From googling this problem I discovered the overridable method [`viewDidEndLiveResize`](https://developer.apple.com/documentation/appkit/nsview/1483543-viewdidendliveresize).

With this fix, the size always ends up correct at the end and `setAvailableSize` is called after `onWindowDidEndLiveResize` if the size changed.
# Other things I tried
Pulling in microsoft#439 does seem to fix the issue (as in it can't be reproduced anymore), but makes resizing look extremely janky.

# Other things I tried that did not work
I was still able to reproduce the issue doing these. Adding these here for posterity.
* Removing all event coalescing
* Removing event coalescing for just onWindowDidEndLiveResize
* Removing event coalescing for just onWindowDidResize
* Removing the synchronous relayout [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/third-party/microsoft-fork-of-react-native/react-native/React/Modules/RCTUIManager.m?lines=421)
* Sending an onWindowDidResize event at the same time as the onWindowDidEndLiveResize event [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/archon/native/rn/src/NativeWindowReact-macOS.mm?lines=587)
* Set `autoresizingmask` in RCTRootContentView, and set `self.autoresizesSubviews = YES` and `self.clipsToBounds = YES` in RCTRootView.

Full diff with all my logging and fix attempts in V0.1 of D36719158

Test Plan:
Logged the size of the Root View on every resize and end of live resize to ensure they were the correct value. See logging in V0.1 of D36719158

Tried to trigger the bug for a long time (minutes) when before this diff it can trigger pretty quickly.

A short video of the window getting "stuck" in the wrong size, and then fixing itself at the end of the live resize:
https://pxl.cl/257PV

NOTE: There are several resizes that happen on the way to the corrected layout. This is because of the way layout is calculated in `ChatNavigationContainer.tsx`. They are using [debounce to wait 1s to resize the left panel](ChatNavigationContainer.tsx). I have some thoughts on how to improve `ChatNavigationContainer.tsx` in the comments of T107249807.

A longer video showing many resizes:
https://pxl.cl/257Q9


Reviewers: skyle, lyahdav

Reviewed By: skyle

Subscribers: joesavona, generatedunixname499725568, kacieb, laurentan

Differential Revision: https://phabricator.intern.facebook.com/D36785342

Tasks: T107249807
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 9, 2022
Summary:
# Problem
Sometimes, at the end of a live resize, the window size is not propagated to resize the content. This leads to situations like this one, where the content is accidentally bigger than the window (see the "Type a message" textbox):
https://www.internalfb.com/intern/px/p/24pVJ/

# Solution
This diff schedules a relayout at the end of a live resize to ensure that the latest value is used.

# How did I discover this?
First I added [logging to product code here](https://www.internalfb.com/code/fbsource/[fbdb38f5d118608951d4807629af599b638f6b74]/xplat/archon/packages/rn/src/messenger/ui/chatNavigation/ChatNavigationContainer.tsx?lines=100) and determined that the last window dimensions value logged is always correct.

I then added a lot of native logging to log the sizes of the RootView and RootContentView, and the events being fired. (Full diff with all my logging in V0.1 of D36719158)

From the logging I determined that the problem ALWAYS occurred when `onWindowDidEndLiveResize` was the most recent event fired. The size in that event was always correct, but [`setAvailableSize`](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/js/react-native-github/React/Modules/RCTUIManager.m?lines=393) was not called. `onWindowDidResize` events did result in `setAvailableSize` being called.

From googling this problem I discovered the overridable method [`viewDidEndLiveResize`](https://developer.apple.com/documentation/appkit/nsview/1483543-viewdidendliveresize).

With this fix, the size always ends up correct at the end and `setAvailableSize` is called after `onWindowDidEndLiveResize` if the size changed.
# Other things I tried
Pulling in microsoft#439 does seem to fix the issue (as in it can't be reproduced anymore), but makes resizing look extremely janky.

# Other things I tried that did not work
I was still able to reproduce the issue doing these. Adding these here for posterity.
* Removing all event coalescing
* Removing event coalescing for just onWindowDidEndLiveResize
* Removing event coalescing for just onWindowDidResize
* Removing the synchronous relayout [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/third-party/microsoft-fork-of-react-native/react-native/React/Modules/RCTUIManager.m?lines=421)
* Sending an onWindowDidResize event at the same time as the onWindowDidEndLiveResize event [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/archon/native/rn/src/NativeWindowReact-macOS.mm?lines=587)
* Set `autoresizingmask` in RCTRootContentView, and set `self.autoresizesSubviews = YES` and `self.clipsToBounds = YES` in RCTRootView.

Full diff with all my logging and fix attempts in V0.1 of D36719158

Test Plan:
Logged the size of the Root View on every resize and end of live resize to ensure they were the correct value. See logging in V0.1 of D36719158

Tried to trigger the bug for a long time (minutes) when before this diff it can trigger pretty quickly.

A short video of the window getting "stuck" in the wrong size, and then fixing itself at the end of the live resize:
https://pxl.cl/257PV

NOTE: There are several resizes that happen on the way to the corrected layout. This is because of the way layout is calculated in `ChatNavigationContainer.tsx`. They are using [debounce to wait 1s to resize the left panel](ChatNavigationContainer.tsx). I have some thoughts on how to improve `ChatNavigationContainer.tsx` in the comments of T107249807.

A longer video showing many resizes:
https://pxl.cl/257Q9


Reviewers: skyle, lyahdav

Reviewed By: skyle

Subscribers: joesavona, generatedunixname499725568, kacieb, laurentan

Differential Revision: https://phabricator.intern.facebook.com/D36785342

Tasks: T107249807
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 10, 2022
Summary:
# Problem
Sometimes, at the end of a live resize, the window size is not propagated to resize the content. This leads to situations like this one, where the content is accidentally bigger than the window (see the "Type a message" textbox):
https://www.internalfb.com/intern/px/p/24pVJ/

# Solution
This diff schedules a relayout at the end of a live resize to ensure that the latest value is used.

# How did I discover this?
First I added [logging to product code here](https://www.internalfb.com/code/fbsource/[fbdb38f5d118608951d4807629af599b638f6b74]/xplat/archon/packages/rn/src/messenger/ui/chatNavigation/ChatNavigationContainer.tsx?lines=100) and determined that the last window dimensions value logged is always correct.

I then added a lot of native logging to log the sizes of the RootView and RootContentView, and the events being fired. (Full diff with all my logging in V0.1 of D36719158)

From the logging I determined that the problem ALWAYS occurred when `onWindowDidEndLiveResize` was the most recent event fired. The size in that event was always correct, but [`setAvailableSize`](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/js/react-native-github/React/Modules/RCTUIManager.m?lines=393) was not called. `onWindowDidResize` events did result in `setAvailableSize` being called.

From googling this problem I discovered the overridable method [`viewDidEndLiveResize`](https://developer.apple.com/documentation/appkit/nsview/1483543-viewdidendliveresize).

With this fix, the size always ends up correct at the end and `setAvailableSize` is called after `onWindowDidEndLiveResize` if the size changed.
# Other things I tried
Pulling in microsoft#439 does seem to fix the issue (as in it can't be reproduced anymore), but makes resizing look extremely janky.

# Other things I tried that did not work
I was still able to reproduce the issue doing these. Adding these here for posterity.
* Removing all event coalescing
* Removing event coalescing for just onWindowDidEndLiveResize
* Removing event coalescing for just onWindowDidResize
* Removing the synchronous relayout [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/third-party/microsoft-fork-of-react-native/react-native/React/Modules/RCTUIManager.m?lines=421)
* Sending an onWindowDidResize event at the same time as the onWindowDidEndLiveResize event [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/archon/native/rn/src/NativeWindowReact-macOS.mm?lines=587)
* Set `autoresizingmask` in RCTRootContentView, and set `self.autoresizesSubviews = YES` and `self.clipsToBounds = YES` in RCTRootView.

Full diff with all my logging and fix attempts in V0.1 of D36719158

Test Plan:
Logged the size of the Root View on every resize and end of live resize to ensure they were the correct value. See logging in V0.1 of D36719158

Tried to trigger the bug for a long time (minutes) when before this diff it can trigger pretty quickly.

A short video of the window getting "stuck" in the wrong size, and then fixing itself at the end of the live resize:
https://pxl.cl/257PV

NOTE: There are several resizes that happen on the way to the corrected layout. This is because of the way layout is calculated in `ChatNavigationContainer.tsx`. They are using [debounce to wait 1s to resize the left panel](ChatNavigationContainer.tsx). I have some thoughts on how to improve `ChatNavigationContainer.tsx` in the comments of T107249807.

A longer video showing many resizes:
https://pxl.cl/257Q9


Reviewers: skyle, lyahdav

Reviewed By: skyle

Subscribers: joesavona, generatedunixname499725568, kacieb, laurentan

Differential Revision: https://phabricator.intern.facebook.com/D36785342

Tasks: T107249807
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
…crosoft#439)" (microsoft#1318)

This change reverts microsoft#459 - but still tries to address the original issues:
- microsoft#422
- microsoft#322

This also addresses an issue when programmatically resizing windows where the RCTRootContentView may end up at the wrong size because an in-flight layout gets resolved after the resize on the main thread.
We now keep sync dispatch on the shadow queue for live resizing windows (to prevent tearing) but also dispatch async (as done on iOS) so the latest new size is sure to win.
The block has a check to bail if the size doesn't change, so this isn't a perf drain running the block twice.

Co-authored-by: Scott Kyle <skyle@fb.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Summary:
# Problem
Sometimes, at the end of a live resize, the window size is not propagated to resize the content. This leads to situations like this one, where the content is accidentally bigger than the window (see the "Type a message" textbox):
https://www.internalfb.com/intern/px/p/24pVJ/

# Solution
This diff schedules a relayout at the end of a live resize to ensure that the latest value is used.

# How did I discover this?
First I added [logging to product code here](https://www.internalfb.com/code/fbsource/[fbdb38f5d118608951d4807629af599b638f6b74]/xplat/archon/packages/rn/src/messenger/ui/chatNavigation/ChatNavigationContainer.tsx?lines=100) and determined that the last window dimensions value logged is always correct.

I then added a lot of native logging to log the sizes of the RootView and RootContentView, and the events being fired. (Full diff with all my logging in V0.1 of D36719158)

From the logging I determined that the problem ALWAYS occurred when `onWindowDidEndLiveResize` was the most recent event fired. The size in that event was always correct, but [`setAvailableSize`](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/js/react-native-github/React/Modules/RCTUIManager.m?lines=393) was not called. `onWindowDidResize` events did result in `setAvailableSize` being called.

From googling this problem I discovered the overridable method [`viewDidEndLiveResize`](https://developer.apple.com/documentation/appkit/nsview/1483543-viewdidendliveresize).

With this fix, the size always ends up correct at the end and `setAvailableSize` is called after `onWindowDidEndLiveResize` if the size changed.
# Other things I tried
Pulling in microsoft#439 does seem to fix the issue (as in it can't be reproduced anymore), but makes resizing look extremely janky.

# Other things I tried that did not work
I was still able to reproduce the issue doing these. Adding these here for posterity.
* Removing all event coalescing
* Removing event coalescing for just onWindowDidEndLiveResize
* Removing event coalescing for just onWindowDidResize
* Removing the synchronous relayout [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/third-party/microsoft-fork-of-react-native/react-native/React/Modules/RCTUIManager.m?lines=421)
* Sending an onWindowDidResize event at the same time as the onWindowDidEndLiveResize event [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/archon/native/rn/src/NativeWindowReact-macOS.mm?lines=587)
* Set `autoresizingmask` in RCTRootContentView, and set `self.autoresizesSubviews = YES` and `self.clipsToBounds = YES` in RCTRootView.

Full diff with all my logging and fix attempts in V0.1 of D36719158

Test Plan:
Logged the size of the Root View on every resize and end of live resize to ensure they were the correct value. See logging in V0.1 of D36719158

Tried to trigger the bug for a long time (minutes) when before this diff it can trigger pretty quickly.

A short video of the window getting "stuck" in the wrong size, and then fixing itself at the end of the live resize:
https://pxl.cl/257PV

NOTE: There are several resizes that happen on the way to the corrected layout. This is because of the way layout is calculated in `ChatNavigationContainer.tsx`. They are using [debounce to wait 1s to resize the left panel](ChatNavigationContainer.tsx). I have some thoughts on how to improve `ChatNavigationContainer.tsx` in the comments of T107249807.

A longer video showing many resizes:
https://pxl.cl/257Q9


Reviewers: skyle, lyahdav

Reviewed By: skyle

Subscribers: joesavona, generatedunixname499725568, kacieb, laurentan

Differential Revision: https://phabricator.intern.facebook.com/D36785342

Tasks: T107249807
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
…crosoft#439)" (microsoft#1318)

This change reverts microsoft#459 - but still tries to address the original issues:
- microsoft#422
- microsoft#322

This also addresses an issue when programmatically resizing windows where the RCTRootContentView may end up at the wrong size because an in-flight layout gets resolved after the resize on the main thread.
We now keep sync dispatch on the shadow queue for live resizing windows (to prevent tearing) but also dispatch async (as done on iOS) so the latest new size is sure to win.
The block has a check to bail if the size doesn't change, so this isn't a perf drain running the block twice.

Co-authored-by: Scott Kyle <skyle@fb.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
Summary:
# Problem
Sometimes, at the end of a live resize, the window size is not propagated to resize the content. This leads to situations like this one, where the content is accidentally bigger than the window (see the "Type a message" textbox):
https://www.internalfb.com/intern/px/p/24pVJ/

# Solution
This diff schedules a relayout at the end of a live resize to ensure that the latest value is used.

# How did I discover this?
First I added [logging to product code here](https://www.internalfb.com/code/fbsource/[fbdb38f5d118608951d4807629af599b638f6b74]/xplat/archon/packages/rn/src/messenger/ui/chatNavigation/ChatNavigationContainer.tsx?lines=100) and determined that the last window dimensions value logged is always correct.

I then added a lot of native logging to log the sizes of the RootView and RootContentView, and the events being fired. (Full diff with all my logging in V0.1 of D36719158)

From the logging I determined that the problem ALWAYS occurred when `onWindowDidEndLiveResize` was the most recent event fired. The size in that event was always correct, but [`setAvailableSize`](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/js/react-native-github/React/Modules/RCTUIManager.m?lines=393) was not called. `onWindowDidResize` events did result in `setAvailableSize` being called.

From googling this problem I discovered the overridable method [`viewDidEndLiveResize`](https://developer.apple.com/documentation/appkit/nsview/1483543-viewdidendliveresize).

With this fix, the size always ends up correct at the end and `setAvailableSize` is called after `onWindowDidEndLiveResize` if the size changed.
# Other things I tried
Pulling in microsoft#439 does seem to fix the issue (as in it can't be reproduced anymore), but makes resizing look extremely janky.

# Other things I tried that did not work
I was still able to reproduce the issue doing these. Adding these here for posterity.
* Removing all event coalescing
* Removing event coalescing for just onWindowDidEndLiveResize
* Removing event coalescing for just onWindowDidResize
* Removing the synchronous relayout [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/third-party/microsoft-fork-of-react-native/react-native/React/Modules/RCTUIManager.m?lines=421)
* Sending an onWindowDidResize event at the same time as the onWindowDidEndLiveResize event [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/archon/native/rn/src/NativeWindowReact-macOS.mm?lines=587)
* Set `autoresizingmask` in RCTRootContentView, and set `self.autoresizesSubviews = YES` and `self.clipsToBounds = YES` in RCTRootView.

Full diff with all my logging and fix attempts in V0.1 of D36719158

Test Plan:
Logged the size of the Root View on every resize and end of live resize to ensure they were the correct value. See logging in V0.1 of D36719158

Tried to trigger the bug for a long time (minutes) when before this diff it can trigger pretty quickly.

A short video of the window getting "stuck" in the wrong size, and then fixing itself at the end of the live resize:
https://pxl.cl/257PV

NOTE: There are several resizes that happen on the way to the corrected layout. This is because of the way layout is calculated in `ChatNavigationContainer.tsx`. They are using [debounce to wait 1s to resize the left panel](ChatNavigationContainer.tsx). I have some thoughts on how to improve `ChatNavigationContainer.tsx` in the comments of T107249807.

A longer video showing many resizes:
https://pxl.cl/257Q9


Reviewers: skyle, lyahdav

Reviewed By: skyle

Subscribers: joesavona, generatedunixname499725568, kacieb, laurentan

Differential Revision: https://phabricator.intern.facebook.com/D36785342

Tasks: T107249807
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request May 1, 2024
Summary:
# Problem
Sometimes, at the end of a live resize, the window size is not propagated to resize the content. This leads to situations like this one, where the content is accidentally bigger than the window (see the "Type a message" textbox):
https://www.internalfb.com/intern/px/p/24pVJ/

# Solution
This diff schedules a relayout at the end of a live resize to ensure that the latest value is used.

# How did I discover this?
First I added [logging to product code here](https://www.internalfb.com/code/fbsource/[fbdb38f5d118608951d4807629af599b638f6b74]/xplat/archon/packages/rn/src/messenger/ui/chatNavigation/ChatNavigationContainer.tsx?lines=100) and determined that the last window dimensions value logged is always correct.

I then added a lot of native logging to log the sizes of the RootView and RootContentView, and the events being fired. (Full diff with all my logging in V0.1 of D36719158)

From the logging I determined that the problem ALWAYS occurred when `onWindowDidEndLiveResize` was the most recent event fired. The size in that event was always correct, but [`setAvailableSize`](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/js/react-native-github/React/Modules/RCTUIManager.m?lines=393) was not called. `onWindowDidResize` events did result in `setAvailableSize` being called.

From googling this problem I discovered the overridable method [`viewDidEndLiveResize`](https://developer.apple.com/documentation/appkit/nsview/1483543-viewdidendliveresize).

With this fix, the size always ends up correct at the end and `setAvailableSize` is called after `onWindowDidEndLiveResize` if the size changed.
# Other things I tried
Pulling in microsoft#439 does seem to fix the issue (as in it can't be reproduced anymore), but makes resizing look extremely janky.

# Other things I tried that did not work
I was still able to reproduce the issue doing these. Adding these here for posterity.
* Removing all event coalescing
* Removing event coalescing for just onWindowDidEndLiveResize
* Removing event coalescing for just onWindowDidResize
* Removing the synchronous relayout [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/third-party/microsoft-fork-of-react-native/react-native/React/Modules/RCTUIManager.m?lines=421)
* Sending an onWindowDidResize event at the same time as the onWindowDidEndLiveResize event [here](https://www.internalfb.com/code/fbsource/[b530355767b24e87c64e8197fae2475133bede2f]/xplat/archon/native/rn/src/NativeWindowReact-macOS.mm?lines=587)
* Set `autoresizingmask` in RCTRootContentView, and set `self.autoresizesSubviews = YES` and `self.clipsToBounds = YES` in RCTRootView.

Full diff with all my logging and fix attempts in V0.1 of D36719158

Test Plan:
Logged the size of the Root View on every resize and end of live resize to ensure they were the correct value. See logging in V0.1 of D36719158

Tried to trigger the bug for a long time (minutes) when before this diff it can trigger pretty quickly.

A short video of the window getting "stuck" in the wrong size, and then fixing itself at the end of the live resize:
https://pxl.cl/257PV

NOTE: There are several resizes that happen on the way to the corrected layout. This is because of the way layout is calculated in `ChatNavigationContainer.tsx`. They are using [debounce to wait 1s to resize the left panel](ChatNavigationContainer.tsx). I have some thoughts on how to improve `ChatNavigationContainer.tsx` in the comments of T107249807.

A longer video showing many resizes:
https://pxl.cl/257Q9

Reviewers: skyle, lyahdav

Reviewed By: skyle

Subscribers: joesavona, generatedunixname499725568, kacieb, laurentan

Differential Revision: https://phabricator.intern.facebook.com/D36785342

Tasks: T107249807

# Conflicts:
#	React/Base/RCTRootView.m
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layout bug when resizing main window
4 participants