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

Revert "Alleviate excessive layout jittering when resizing window (#439)" #1318

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

christophpurrer
Copy link

Please select one of the following

  • 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

This change reverts #439 - but still tries to address the original issues:

and the follow up #459

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.

Changelog

[macOS] [Changed] - Revert "Alleviate excessive layout jittering when resizing window (#439)"

Test Plan

Before

Works - but resizing drags ...

https://youtu.be/obQwaxcL48k

Revert 439

Doesn't work

https://youtu.be/LV00uFOBddY

Ater

Works and resizing is smooth

https://youtu.be/rzp1H_tfqGY

…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.
@Saadnajmi
Copy link
Collaborator

Q: Why post the video demos to YouTube instead of directly in GitHub?

@Saadnajmi
Copy link
Collaborator

Maybe a question for @rozele , did RNW have a similar concern / how did they fix this issue? I've been wondering how we can get rid of this throttle, so thank you for the change =)

@christophpurrer
Copy link
Author

Q: Why post the video demos to YouTube instead of directly in GitHub?

There is a 10 MB for videos. Is there a way to post larger video files?

@Saadnajmi
Copy link
Collaborator

Q: Why post the video demos to YouTube instead of directly in GitHub?

There is a 10 MB for videos. Is there a way to post larger video files?

Ah, ok. I guess I never hit that limit personally. Thanks! I'm sure Github is happy we aren't using their storage :P

@christophpurrer
Copy link
Author

Q: Why post the video demos to YouTube instead of directly in GitHub?

There is a 10 MB for videos. Is there a way to post larger video files?

Ah, ok. I guess I never hit that limit personally. Thanks! I'm sure Github is happy we aren't using their storage :P

This could be a per repo-setting.
Maybe we can bump it to 25 MB?

@rozele
Copy link

rozele commented Aug 3, 2022

We haven't solved this issue on Windows yet. There are a couple of issues on Windows:

  1. We don't even start to resize the content until the root control fires SizeChanged, which only happens after the first layout pass by XAML to update the size of the root when it's stretched to fit the window.
  2. On size changed, we run Yoga layout on the root Yoga node, which, for complex apps, generally takes at least a couple frames, then we commit the changes to XAML, and that commit likely takes another frame or so.

Fortunately, throttling isn't really needed, because XAML only fires the SizeChanged event after committing the XAML layout changes to draw (I think), and this seems to occur on frame boundaries.

Another design decision of Windows that prevents the need for throttling is that Yoga layout runs on the UI thread, rather than a background thread, so when Yoga layout doesn't occur within a frame, all layout (even XAML layout) is delayed.

In react-native-windows v0.59, when we used to perform Yoga layout on a background thread, we actually did implement a form of throttling, namely that we simply did not compute Yoga layout for stale root view resizes:
https://github.com/microsoft/react-native-windows/blob/07a4bbbfe0d5860d5cd919ccbb2e470759c9b309/current/ReactWindows/ReactNative.Shared/UIManager/UIManagerModule.cs#L163

@Saadnajmi Saadnajmi merged commit e6482e0 into microsoft:main Aug 4, 2022
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 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>
@psalishol
Copy link

Hi @christophpurrer, I noticed that you were able to successfully fix the excessive layout drag issue. I'm still having some trouble with it and I was wondering if you could share any details on how you were able to resolve it? Your insights would be really helpful in resolving this issue. Thank you!

@Saadnajmi
Copy link
Collaborator

Hi @christophpurrer, I noticed that you were able to successfully fix the excessive layout drag issue. I'm still having some trouble with it and I was wondering if you could share any details on how you were able to resolve it? Your insights would be really helpful in resolving this issue. Thank you!

Could you try again with React Native macOS 0.71? I think you should see improvements there.

@psalishol
Copy link

Hi @christophpurrer, I noticed that you were able to successfully fix the excessive layout drag issue. I'm still having some trouble with it and I was wondering if you could share any details on how you were able to resolve it? Your insights would be really helpful in resolving this issue. Thank you!

Could you try again with React Native macOS 0.71? I think you should see improvements there.

thanks Saadnajmi, it's fixed in .71... thumbs up to the team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants