Skip to content

Ensure full layout occurs when a system scale change occurs #45047

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.

Sign up for GitHub

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

Closed
acoates-ms wants to merge 1 commit into from
Closed

Ensure full layout occurs when a system scale change occurs #45047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/react-native/ReactCommon/react/renderer/components/root/RootShadowNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ RootShadowNode::Unshared RootShadowNode::clone(
/* .props = */ props,
});

if (layoutConstraints != getConcreteProps().layoutConstraints) {
if (layoutConstraints != getConcreteProps().layoutConstraints ||
layoutContext.pointScaleFactor !=
getConcreteProps().layoutContext.pointScaleFactor) {
newRootShadowNode->dirtyLayout();
}
Comment on lines -52 to 56
Copy link
Contributor

@NickGerleman NickGerleman Jun 18, 2024

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.

react-native/packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp

Line 462 in d8739e1

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.


Expand Down
4 changes: 3 additions & 1 deletion packages/react-native/ReactCommon/yoga/yoga/algorithm/CalculateLayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2140,7 +2140,8 @@ bool calculateLayoutInternal(

Copy link
Contributor

@NickGerleman NickGerleman Jun 18, 2024

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)?

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

@NickGerleman NickGerleman Jun 18, 2024

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).


if (needToVisitNode) {
// Invalidate the cached results.
Expand Down Expand Up @@ -2255,6 +2256,7 @@ bool calculateLayoutInternal(
reason);

layout->lastOwnerDirection = ownerDirection;
layout->lastScale = node->getConfig()->getPointScaleFactor();

if (cachedResults == nullptr) {
layoutMarkerData.maxMeasureCache = std::max(
Expand Down
1 change: 1 addition & 0 deletions packages/react-native/ReactCommon/yoga/yoga/node/LayoutResults.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ bool LayoutResults::operator==(LayoutResults layout) const {
direction() == layout.direction() &&
hadOverflow() == layout.hadOverflow() &&
lastOwnerDirection == layout.lastOwnerDirection &&
lastScale == layout.lastScale &&
nextCachedMeasurementsIndex == layout.nextCachedMeasurementsIndex &&
cachedLayout == layout.cachedLayout &&
computedFlexBasis == layout.computedFlexBasis;
Expand Down
1 change: 1 addition & 0 deletions packages/react-native/ReactCommon/yoga/yoga/node/LayoutResults.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ struct LayoutResults {
// information to break early when nothing changed
uint32_t generationCount = 0;
Direction lastOwnerDirection = Direction::Inherit;
float lastScale = 1.0f;

uint32_t nextCachedMeasurementsIndex = 0;
std::array<CachedMeasurement, MaxCachedMeasurements> cachedMeasurements = {};
Expand Down