Skip to content

Conversation

@acoates-ms
Copy link
Contributor

Summary:

When a scale factor change occurs, RN does not fully invalidate the layout. This can cause issues with pixel alignment and means that the LayoutMetrics on native components do not reliably get the updated scale factor.

Essentially the layout caching in calculateLayoutInternal, will not recalculate layout on a scale factor change.

This likely affects desktop platforms more than mobile, since on desktop you can have the scale factor change often while running by moving windows between monitors with different scale factors.  I'm not sure under what scenarios this would affect mobile.

Changelog:

[INTERNAL] [FIXED] - Recalculate layout on scale factor changes.

Test Plan:

Using this change to validate running react-native-windows fabric in environments with multiple monitors of different scale factors.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 18, 2024
@rozele
Copy link
Contributor

rozele commented Jun 18, 2024

Should the same occur for font scale changes?

@rozele
Copy link
Contributor

rozele commented Jun 18, 2024

Might be worth contributing parts of this to https://github.com/facebook/yoga and adding a test case as well.

LayoutResults* layout = &node->getLayout();

depth++;

Copy link
Contributor

Choose a reason for hiding this comment

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

These files will ultimately get synced everywhere, but we usually prefer changes against Yoga to be made against the Yoga repo (which will run some additional validation in OSS as well).

Yoga is also pretty heavily unit tested, so we would want to add a test alongside this. Unit tests are run automatically against the Yoga repo, but there is also a unit_tests.bat that will build and run the tests on Windows/MSVC so long as CMake is set up correctly (I think you can run from VS command shell)?

Comment on lines +2143 to +2144
layout->lastOwnerDirection != ownerDirection ||
layout->lastScale != node->getConfig()->getPointScaleFactor();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is logic today, to try to account for pointScaleFactor when we are comparing layout constraints for cache reuse (compared to logic that has gotten chained here for dirtying all results): https://github.com/facebook/yoga/blob/baf670e261892ad4e72c5c02b6ff671537b661a7/yoga/algorithm/Cache.cpp#L64

I wouldn't be surprised if there are issues with the current interaction of layout rounding and caching, but we should probably only do this invalidation in one place (and make sure we have tests that it works, if we don't right now).

Comment on lines -52 to 56

if (layoutConstraints != getConcreteProps().layoutConstraints) {
if (layoutConstraints != getConcreteProps().layoutConstraints ||
layoutContext.pointScaleFactor !=
getConcreteProps().layoutContext.pointScaleFactor) {
newRootShadowNode->dirtyLayout();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a down-propagation/inheritance step, during commit, once we have the full shadow-tree constructed, where we propagate pointScaleFactor to every Yoga node.

YGConfigSetPointScaleFactor(&yogaConfig_, pointScaleFactor);

YGNodeSetConfig will do a comparison of previous and current config to dirty node (though changing something on the actual config after-the-fact does not, which is an API bug). So this code has a defect where it is not dirtying underlying Yoga node, if pointScaleFactor changes.

We probably want to fixup the invalidation when setting pointScaleFactor (either there in RN, or in Yoga), instead of just invalidating the root node.

An aside, I know the code already does this, but this method of dirtying that Fabric uses is a smell (and Yoga's public API only allows dirtying nodes with measure functions which can be impacted by external changes). Yoga tries to manage dirtying in all other cases, when APIs are called which mutate style or config. RootShadowNode::layoutIfNeeded should normally be able to run, even if the node is not dirty, and work only happens if the passed in constraints have changed.

@NickGerleman
Copy link
Contributor

Okay, so right now we invalidate based on pointScaleFactor in a couple places:

  1. Dirties the node during YGNodeSetConfig, and Yoga as of recent enforces pointScaleFactor is set per-node
  2. We do rounded comparison during constraints check
  3. This PR would add explicit pointScaleFactor check in dirty checks

I think we could replace all of this, with a single solution, that solves for the config invalidation API issue Yoga has.

  1. Add dirty flag to YGConfig, that setters like YGConfigSetPointScaleFactor can set on changed value (instead of current API, which checks when setting the config object)
  2. Check and reset the YGConfig-level dirty flag, around where your change is.
  3. Make YGNodeIsDirty() incorporate config dirty flag

I think we could potentially then remove any of the other derived pointScaleFactor comparisons, since we fully bust layout and measurement caches (of the single node) when a layout-effecting config parameter changes.

@acoates-ms
Copy link
Contributor Author

@NickGerleman - Does storing the dirty bit on the yoga config work? Doesn't the API allow for the same config object to be shared across multiple nodes?

@NickGerleman
Copy link
Contributor

@acoates-ms, you are right. Multiple nodes sharing single config would break a single dirty flag.

I think the core behavior we want, is that YGConfigSetPointScaleFactor() can lead to invalidation (but we don't want the config to back-reference the nodes).

The first extension I could imagine for fixing n:1 issue of dirty flag, could be keeping revision count on config (and associated last-config-revision on node) instead, but that might be too extreme. At the same time, implementing as config-level dirtniess is appealing to me though, just to generalize this fix to other config attributes.

NickGerleman pushed a commit to NickGerleman/react-native that referenced this pull request Jul 2, 2024
Summary:
This is a continuation of the previous PR: facebook#45047

I made the change more generic for allowing any kind of config change to invalidate layout.

X-link: facebook/yoga#1674

Differential Revision: D59286992

Pulled By: NickGerleman
NickGerleman pushed a commit to NickGerleman/react-native that referenced this pull request Jul 3, 2024
…45259)

Summary:
Pull Request resolved: facebook#45259

This is a continuation of the previous PR: facebook#45047

I made the change more generic for allowing any kind of config change to invalidate layout.

X-link: facebook/yoga#1674

Reviewed By: rozele

Differential Revision: D59286992

Pulled By: NickGerleman
NickGerleman pushed a commit to NickGerleman/yoga that referenced this pull request Jul 3, 2024
…1674)

Summary:
This is a continuation of the previous PR: facebook/react-native#45047

I made the change more generic for allowing any kind of config change to invalidate layout.

Pull Request resolved: facebook#1674

Differential Revision: D59286992

Pulled By: NickGerleman
facebook-github-bot pushed a commit to facebook/litho that referenced this pull request Jul 3, 2024
Summary:
X-link: facebook/react-native#45259

This is a continuation of the previous PR: facebook/react-native#45047

I made the change more generic for allowing any kind of config change to invalidate layout.

Changelog: [Internal]

X-link: facebook/yoga#1674

Reviewed By: rozele

Differential Revision: D59286992

Pulled By: NickGerleman

fbshipit-source-id: f46f35b03d5d9a743b798844ee3e1a02c271ccde
facebook-github-bot pushed a commit that referenced this pull request Jul 3, 2024
Summary:
Pull Request resolved: #45259

This is a continuation of the previous PR: #45047

I made the change more generic for allowing any kind of config change to invalidate layout.

Changelog: [Internal]

X-link: facebook/yoga#1674

Reviewed By: rozele

Differential Revision: D59286992

Pulled By: NickGerleman

fbshipit-source-id: f46f35b03d5d9a743b798844ee3e1a02c271ccde
facebook-github-bot pushed a commit to facebook/yoga that referenced this pull request Jul 3, 2024
Summary:
X-link: facebook/react-native#45259

This is a continuation of the previous PR: facebook/react-native#45047

I made the change more generic for allowing any kind of config change to invalidate layout.

Changelog: [Internal]

Pull Request resolved: #1674

Reviewed By: rozele

Differential Revision: D59286992

Pulled By: NickGerleman

fbshipit-source-id: f46f35b03d5d9a743b798844ee3e1a02c271ccde
@acoates-ms
Copy link
Contributor Author

Replaced by #45259

@acoates-ms acoates-ms closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants