MaterialDesignInXAML / MaterialDesignInXamlToolkit

Google's Material Design in XAML & WPF, for C# & VB.Net.
http://materialdesigninxaml.net
MIT License
14.82k stars 3.4k forks source link

ScrollViewer-Padding Fixed #3579

Closed JorisCleVR closed 1 week ago

JorisCleVR commented 1 month ago

Ticket: https://github.com/MaterialDesignInXAML/MaterialDesignInXamlToolkit/issues/3570

Currently when you set the padding to the ScrollViewer it is by default ignored, because if it was enabled it adds the padding in an unexpected way. This results in the contents of the ScrollViewer being cut off when scrolling.

Old situation

Below a screenshot and the corresponding xaml code that shows the unexpected cut off text because of the padding.

Old

<smtx:XamlDisplay UniqueKey="groupbox_7">
<GroupBox Width="300" Height="100" Header="Header"
          materialDesign:GroupBoxAssist.HeaderPadding="4" Padding="0"
          Style="{StaticResource MaterialDesignGroupBox}">
  <ScrollViewer Padding="4" materialDesign:ScrollViewerAssist.IgnorePadding="False">
    <TextBlock Margin="4" TextWrapping="Wrap"
      Text="Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum." />
  </ScrollViewer>
</GroupBox>
</smtx:XamlDisplay>

New situation

Below a screenshot and the corresponding xaml code that shows the new situation where the padding is correctly applied to the content and where the contents are not cut off when scrolling.

New

      <smtx:XamlDisplay UniqueKey="groupbox_7">
        <GroupBox Width="300" Height="100" Header="Header"
                  materialDesign:GroupBoxAssist.HeaderPadding="4" Padding="0"
                  Style="{StaticResource MaterialDesignGroupBox}">
          <ScrollViewer Padding="4">
            <TextBlock Margin="4" TextWrapping="Wrap"
              Text="Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum." />
          </ScrollViewer>
        </GroupBox>
      </smtx:XamlDisplay>
JorisCleVR commented 3 weeks ago

In my previous commit I forgot to set IgnorePadding to false. I introduced it using a new commit on the same branch, but now that all checks have passed it does not seem to sync with the branch anymore. Can you please include the other commit in here?

Keboo commented 3 weeks ago

In my previous commit I forgot to set IgnorePadding to false. I introduced it using a new commit on the same branch, but now that all checks have passed it does not seem to sync with the branch anymore. Can you please include the other commit in here?

O that is interesting, normally just pushing a commit to your branch should synchronize it. Yes I will take a look and include it.

JorisCleVR commented 3 weeks ago

Good to know that it should still synchronize. I looked further into it and it seemed that my git tool indicated that it was pushed to my remote Github, but in reality it was not. Now found a way to force it through :).

Keboo commented 2 weeks ago

So, I did a big more digging on this one. The current behavior matches the default WPF ScrollViewer, so I am a bit hesitant to change that. With that said, I can certainly understand the desire to be able to apply a padding like that generically.

Perhaps an alternate solution with an attached property for ContentPadding so that the two concepts remain distinct. Thoughts?

JorisCleVR commented 2 weeks ago

I certainly get your hesitation. Though I am wondering why anyone would want to use this default WPF ScrollViewer behavior, since it basicly creates borders/margins around the scrollviewer itself. Therefore I think if someone would want to achieve this the Margin or Border properties could be used instead. (Noting that currently the Border properties are not applied by the ControlTemplate, but we could reintroduce that)

That being said I respect your opinion and we can certainly look into other options. Personally I am not a big fan of the ContentPadding, since the normal go to Padding does something unexpected and you need to know that there is something like a ContentPadding you need to use instead. Furthermore we then also need to introduce an ScrollViewerAssist.IgnoreContentPadding next to the already existing ScrollViewerAssist.IgnorePadding. I think we then could better introduce an ScrollViewerAssist.PaddingMode, though this makes the implementation more complex.

What are your thoughs about that the Margin/Border can be used to achieved the same? And if my reasoning about the margin/border still raises too much hesitation: What do you think about the PaddingMode?

JorisCleVR commented 2 weeks ago

After thinking about it a bit more I came to the conclusion that Margin would not work, since I overlooked that the Padding currently is not applied around the scroll bars. We could still use Border, but you might also suspect that to be around the scroll bars as well. Still curious about your thoughts on this and the PaddingMode instead of ContentPadding property.

Keboo commented 2 weeks ago

I think PaddingMode would probably be a reasonable way to approach it. I do agree that the current behavior of the WPF ScrollViewer is less ideal, and I do like this suggestion as it allows for a more generic way for app authors to provide that additional space around their content. I have been bit a few times deviating from the default WPF behavior as that can cause confusion for people looking through docs and old tutorial that assume default behavior, so whenever possible I try to maintain it.

Also, thank you very much for all of the work and PRs, it is greatly appreciated.

JorisCleVR commented 2 weeks ago

How unfortunate that you have been bit a few times by deviating from the default WPF behavior. In that case PaddingMode sounds indeed like the most reasonable approach. Is it then still reasonable to by default set it to PaddingMode.Content and that for people wanting to use the WPF default behavior to set it to PaddingMode.WPFBehavior? Or do we also need to make the default PaddingMode.WPFBehavior? I will look into creating an implementation for this.

JorisCleVR commented 1 week ago

In the last commit I added the discussed ScrollViewerAssist.PaddingMode. The implementation was pretty straightforward. Let me know what you think.

Keboo commented 1 week ago

In the last commit I added the discussed ScrollViewerAssist.PaddingMode. The implementation was pretty straightforward. Let me know what you think.

Great, I will get it reviewed a little later tonight.

JorisCleVR commented 1 week ago

I have sent you a message on Discord.