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

Fixing ScrollBar regression for Issue #4255 #4495

Merged
merged 7 commits into from
Mar 13, 2021

Conversation

RBrid
Copy link
Contributor

@RBrid RBrid commented Mar 12, 2021

Fixes #4255

We recently added a 1px margin around the ScrollBar, in its default control template. That potentially caused a layout cycle on DPI levels other than 100%. The chosen solution is to remove that ScrollBar 1px margin and add instead a 1px padding where it's used the most: the ScrollViewer control.

So the ScrollViewer's control template is declared in WinUI 2.6 now and includes 2 new Grid elements around the ScrollBar controls, with a 1px Padding around them.

Only ScrollViewer_themeresources.xaml has the additional Grid elements (just like only ScrollBar_themeresources.xaml had the 1px margin).

The same trick is used in the new WinUI ScrollView control. Its default control template now includes 2 additional Grid elements surrounding its ScrollBar controls.

While in there, I also fixed a couple of things:

  • Updated ScrollViewScrollBarsSeparatorBackground as it is meant to be transparent now like the ScrollViewer's scrollbars separator.
  • Fixed ScrollViewerScrollBarSeparatorBackground for HighContrast as it is supposed to use SystemControlTransparentBrush and not ControlFillColorTransparentBrush.

@RBrid RBrid added team-Controls Issue for the Controls team area-ScrollBar labels Mar 12, 2021
@RBrid RBrid self-assigned this Mar 12, 2021
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 12, 2021
@RBrid
Copy link
Contributor Author

RBrid commented Mar 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RBrid
Copy link
Contributor Author

RBrid commented Mar 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RBrid
Copy link
Contributor Author

RBrid commented Mar 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RBrid RBrid merged commit 3802621 into master Mar 13, 2021
@RBrid RBrid deleted the user/regisb/ScrollBarWorkaround5 branch March 13, 2021 00:13
@StephenLPeters StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Mar 13, 2021
@robloo
Copy link
Contributor

robloo commented Mar 13, 2021

@RBrid

This looks like a work-around not a fix. As someone who has spent a lot of time also trying to work-around the 'Layout Cycle Detected' bugs that crop up in apps I'm more interested in what the actual root cause of this is. Is there any plan to address what I assume are rounding errors in the layout math with WinUI 3? It shouldn't be possible to trigger a layout cycle exception by just adding a margin. Something deep is broken and it's going to keep popping up.

@RBrid
Copy link
Contributor Author

RBrid commented Mar 15, 2021

@robloo, I agree with you completely. This is a workaround rather than a fix - my title/comments should have been clearer about this, like my branch name user/regisb/ScrollBarWorkaround5.

We opted for a WinUI 2.6 workaround rather than shipping a QFE in every Windows OS version supported by WinUI 2.6. At this point we do not know the exact root cause and its location. It is likely in our layout engine, or maybe the ScrollBar code itself. It is a rounding error for sure, and again I agree that adding any Margin should not cause a layout cycle. We do have an open internal bug to track this issue for WinUI3+. Thanks.

@robloo
Copy link
Contributor

robloo commented Mar 16, 2021

@RBrid Thanks for the details. It makes sense to skip the full fix for WinUI 2.x and instead focus resources on WinUI 3. I just wanted to be sure it wasn't going untracked. Glad to hear there is an internal bug.

Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Nov 14, 2023
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ScrollBar team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Expander control causes LayoutCycle exception in winui 2.6 preview
4 participants