Propagate LocalLayoutDirection into PopupLayout #562
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we overwrite (by re-providing) a bunch of locals in
ProvideCommonCompositionLocals
even within a child owner. This, for example, resets a user-providedLocalLayoutDirection
in popups.Initially I had wanted to change
ProvideCommonCompositionLocals
to only override the locals that seem to be tied to the owner. Following a conversation with Andrey Kulikov [G], however, I decided to manually forward the localLayoutDirection
to the child owner inPopupLayout
, which is how Android does this.While this solves the immediate problem with
LayoutDirection
, it doesn't appear to solve it for any of the other locals overridden byProvideCommonCompositionLocals
. We will need to do that when/if we discover use-cases where our overriding is undesirable.Proposed Changes
PopupLayout
forward the localLayoutDirection
to the child owner.SkiaBasedOwner
keep the layout direction in a state variable, and when changed also propagate it to the focus owner, and root layout node.Testing
Test: Added a unit test that makes sure
LocalLayoutDirection
is propagated intoPopup
.Issues Fixed
Fixes: JetBrains/compose-multiplatform#3142