Fix initial logical scrollable state #13066
Conversation
Since #10803, the `ScrollContentPresenter.Content` binding is managed internally, not via a binding in the template.
grokys
added
the
backport-candidate-11.0.x
| @@ -1182,7 +1182,6 @@ private static FuncControlTemplate CreateScrollViewerTemplate() | |||
| new ScrollContentPresenter | |||
| { | |||
| Name = "PART_ContentPresenter", | |||
| [~ScrollContentPresenter.ContentProperty] = parent.GetObservable(ScrollViewer.ContentProperty).ToBinding(), | |||
There was a problem hiding this comment.
While investigating this I noticed that some tests still have a content binding in their ScrollViewer templates which shouldn't be the case ever since #10803. These tests are unrelated, but having this binding here may make the tests incorrect (as the new test add in this PR passed with this binding in place for example).
| @@ -323,6 +323,7 @@ internal void AttachToScrollViewer() | |||
| } | |||
|
|
|||
| _ownerSubscriptions?.Dispose(); | |||
| _owner = owner; | |||
There was a problem hiding this comment.
This line being moved is the actual fix.
There was a problem hiding this comment.
This should be OK.
I think I put it after subscription setup because the property changed code just sets values on the owner, but when this method is executing the owner is where those values came from in the first place. So that seems like a redundant assignment each time.
If it fixes the bug though, then clearly there is some other side effect that I'm not aware of.
There was a problem hiding this comment.
Yeah the side effect is that it pushes the ScrollViewer.Content to the SCP, which causes the SCP to read the ILogicalScrollable properties, which need to then be synced back with the ScrollViewer.
There was a problem hiding this comment.
Perhaps a comment in the code about the rationale would be helpful for future eyes on this.
|
You can test this PR using the following package version. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
grokys
added
backported-11.0.x
and removed
backport-candidate-11.0.x
What does the pull request do?
As described in #13064, the initial state for a
ILogicalScrollablecontrol isn't correctly synced to the ownerScrollViewer.This seems to have been introduced in #10803 because at the time that the subscriptions in
ScrollContentPresenterare set up, the_owneris stillnulland so when the changes arrive atOnPropertyChangedthey're not written to the owner.@TomEdwardsEnscape you wrote #13064, is moving the
_ownerassignment like this going to break anything else do you think? Was it placed after the subscription setup for a reason?Fixed issues
Fixes #13064