Align TabView visuals with Edge #2201

marcelwgn · 2020-03-31T21:11:00Z

Description

This PR aligns the visuals of the TabView control with chromium Edge by adding the corner radii on the bottom of the TabViewItems

Motivation and Context

Align visuals with Edge

How Has This Been Tested?

Tested manually.

Screenshots (if appropriate):

mdtauk · 2020-03-31T21:21:43Z

Glad you were able to implement this. I think there may need to be a provision to remove those inset corners when the CornerRadius value is set to something other than the default. If I read the code right, you have used path geometry to do them.

marcelwgn · 2020-03-31T21:23:56Z

Yes I use path, however if we change the OverlayCornerRadius themeresource, this changes all corner radii of the TabViewItems, that is setting it to 10 sets it to 10 for all 4 corners, and setting it to 0 removes them. That is because of the Stretch="Uniform" .

mdtauk · 2020-03-31T21:25:36Z

Yes I use path, however if we change the OverlayCornerRadius themeresource, this changes all corner radii of the TabViewItems, that is setting it to 10 sets it to 10 for all 4 corners, and setting it to 0 removes them. That is because of the Stretch="Uniform" .

And does that affect the paths used for the bottom inset corners?

marcelwgn · 2020-03-31T21:32:26Z

Yes, it does:

mdtauk · 2020-03-31T21:41:44Z

Yes, it does:

One more question. Does this work with the TabBar Background using a HostBackdrop Acrylic of say 60% and the Current TabView colour using the same Acrylic, but at 80-90%?

I am guessing the answer is yes, but I gotta ask lol

marcelwgn · 2020-03-31T22:08:24Z

One more question. Does this work with the TabBar Background using a HostBackdrop Acrylic of say 60% and the Current TabView colour using the same Acrylic, but at 80-90%?
I am guessing the answer is yes, but I gotta ask lol

Interesting question! That also works:

mdtauk · 2020-03-31T22:19:56Z

One more question. Does this work with the TabBar Background using a HostBackdrop Acrylic of say 60% and the Current TabView colour using the same Acrylic, but at 80-90%?
I am guessing the answer is yes, but I gotta ask lol

Interesting question! That also works:

BRILLIANT! Just wanted to make sure there would be no problem with overlaps or gaps between elements which can be disguised fine with opaque colours, but not with transparencies!

dev/TabView/TabView.xaml

ranjeshj · 2020-04-01T13:25:48Z

dev/TabView/TabView.idl

@@ -182,8 +182,12 @@ unsealed runtimeclass TabViewItemTemplateSettings : Windows.UI.Xaml.DependencyOb
    TabViewItemTemplateSettings();

    Windows.UI.Xaml.Controls.IconElement IconElement;
+    Windows.UI.Xaml.Thickness LeftInsetRadiusMargin;


@stmoy need API review for these ?

ranjeshj · 2020-04-01T13:28:23Z

dev/TabView/TabViewItem.cpp

@@ -84,6 +92,15 @@ void TabViewItem::OnApplyTemplate()

 void TabViewItem::OnIsSelectedPropertyChanged(const winrt::DependencyObject& sender, const winrt::DependencyProperty& args)
 {
+    if (IsSelected())
+    {
+        SetValue(winrt::Canvas::ZIndexProperty(),box_value(20));


can this be done in the visual state ?

If Tab Placement for the bottom or sides is added, there will need to be new visual states added. Would that cause any issues?

can this be done in the visual state ?

If we want the inset corners to be able to register hits, so that clicking on them does not switch the selection to the item they are on top, unfortunately not.

If Tab Placement for the bottom or sides is added, there will need to be new visual states added. Would that cause any issues?

Since we are talking about ZIndex here, this won't be problematic with the bottoms or side. The inset corners may become problematic, but that is something we need to deal with later.

can this be done in the visual state ?

If we want the inset corners to be able to register hits, so that clicking on them does not switch the selection to the item they are on top, unfortunately not.

I don't understand this, I agree the property needs to be set to achieve this but it should be settable from the visual state shouldn't it?

Setting the ZIndex does not work in the visual state as we can't set the ZIndex of the TabViewItem itsself. Setting it on the LayoutRoot does not work, as setting ZIndex of the layoutroot only "lifts" the LayoutRoot relative to other childs with same parent. Since the TabViewItem needs to be lifted relative to the other TabViewItems, we need to set that items ZIndex.

Sorry I didn't see this a while ago.. I thought adding this to the selected state in the TabViewItem's VSM would set the ZIndex on the TVI itself:

<Setter Property="Canvas.ZIndex" Value="20"/>

I think ommiting a target makes the property get set on the TVI itself

Trying to define it using Setter.Property results in the following compile error:

1>D:\Projects\microsoft-ui-xaml\BuildOutput\Debug\x64\Microsoft.UI.Xaml\rs2_generic.xaml(556,54): XamlCompiler error WMC0610: The XAML Binary Format (XBF) generator reported syntax error '0x09ff'

Any ideas @StephenLPeters what could cause this?

I suspect the syntax is a bit different but I'm not sure what it is precisely, I think its fine to leave as is.

dev/TabView/TabViewItem.h

kmahone · 2020-04-03T17:26:59Z

/azp run

azure-pipelines · 2020-04-03T17:27:10Z

Azure Pipelines successfully started running 1 pipeline(s).

kmahone · 2020-04-03T19:21:13Z

Looks like this change introduced a number of test failures:

Failing tests:
InteractionTests.TabViewTests.TabSizeAndScrollButtonsTest
InteractionTests.TabViewTests.CloseSelectionTest
InteractionTests.TabViewTests.GamePadTest
InteractionTests.TabViewTests.KeyboardTest
InteractionTests.TabViewTests.SelectionTest

stmoy · 2020-04-30T17:05:02Z

If you look closely you will see that Edge does have it, its very subtle but still there.

Agreed, it's very subtle. I'm not opposed to adding it, but we're so close to finishing this PR that I'd prefer to get this one over the finish line and not block on adding them to the hover states.

StephenLPeters

dev/TabView/TabView.idl

ranjeshj · 2020-05-14T12:18:51Z

/azp run

azure-pipelines · 2020-05-14T12:19:02Z

Azure Pipelines successfully started running 1 pipeline(s).

marcelwgn · 2020-05-14T17:43:34Z

I don't think this test failure is related to the TabView, so maybe we can try a rerun?

StephenLPeters · 2020-05-18T21:19:45Z

/azp run

azure-pipelines · 2020-05-18T21:19:55Z

Azure Pipelines successfully started running 1 pipeline(s).

MikeHillberg · 2020-06-30T19:41:45Z

@chingucoding, why is it necessary to set the margins on the two Paths (and thus the two new APIs)?

marcelwgn · 2020-06-30T19:45:41Z

@MikeHillberg Since the two paths need to render outside of the TabViewItem, we need a negative margin. Since the two paths are sized based on the OverlayCornerRadius, we need to calculate the negative margin based off of that. The only two ways to that is either use a converter or add that to the TemplateSettings.

MikeHillberg · 2020-07-01T00:22:31Z

@chingucoding, that makes sense, thanks!

What's difficult is that it violates the separation between the view (template) and the control. The control is returning the correct value for this property only because it knows that the template is using OverlayCornerRadius.

marcelwgn · 2020-07-01T08:44:17Z

Yeah that's problematic. However what other way is there to handle this? Unfortunately, there isn't a CornerRadius property on the TabViewItem which we could leverage here. All the rounded corners are deeply rooted inside the template. We could get the corner radius from the "TabContainer", however we then would rely on the template providing this specific part of the template and visual tree lookups are generally expensive.

MikeHillberg · 2020-07-01T18:43:19Z

This is a nice demonstration of why we need code support in control templates. The other way we've handled this, as you mentioned, is with a converter. CornerRadiusFilterConverter is almost the same, where we needed to pull a component(s) out of a struct. We could do that here, the difference being that it support a Thickness and negation.

marcelwgn · 2020-07-01T19:16:23Z

@ranjeshj @teaP @StephenLPeters Was there a reason we chose to not use a custom converter here? I think it was because it would be quite specific, but not sure.

@MikeHillberg Would it be better if we switch to a custom converter here?

MikeHillberg · 2020-07-01T23:54:51Z

I think a custom converter would be better.

Overall, the goal is for a Control and ControlTemplate to be a model and a view, so following normal model/view separation and motivations and guidelines. In the page or user control scenario, we allow for code-behind in the view, and we have similar though limited support in data template as well. But we currently have no capability in control templates. So in the interim, when we need some code in a control template, we use template settings or converters, and no clear guidance about when to use which. I generally prefer template settings because they're lighter weight, but I think converters are preferable when necessary to maintain the view's separation.

StephenLPeters · 2020-07-02T00:51:37Z

It wouldn't be the first rather specific converter in Winui, See the CornerRadiusToThicknessConverter used by teaching tip. I think a converter makes sense here. Even though it is likely to only be used by tabview the calculations that the converter would be doing is cleanly separated from the tab view and thus i think the separation of the code makes sense

ranjeshj · 2020-07-02T22:47:59Z

@MikeHillberg @StephenLPeters Can we make this as the new guidance no matter how specific the converter is ? I like the separation so that we don't end up with template specific properties on the control API surface.

ranjeshj · 2020-07-03T03:54:39Z

@StephenLPeters @MikeHillberg Looks like we are in agreement that this should be a converter. @chingucoding would you like to make that change ?

marcelwgn · 2020-07-03T11:45:00Z

Sure I'll make that change @ranjeshj.

MikeHillberg · 2020-07-03T17:42:50Z

Thanks @chingucoding!

@jtippet

See: https://github.com/microsoft/microsoft-ui-xaml/releases/tag/v2.5.0-prerelease.200609001 > ### Notable Changes: > > Resize tab view items only once the pointer has left the TabViewItem strip (microsoft/microsoft-ui-xaml#2569) > Align TabView visuals with Edge (microsoft/microsoft-ui-xaml#2201) > Fix background of MenuFlyout in white high contrast (microsoft/microsoft-ui-xaml#2446) > TabView: Make TabViewItem consume the TabViewItemHeaderForeground theme resource (microsoft/microsoft-ui-xaml#2348) > TabView: Add tooltips to its scrolling buttons. (microsoft/microsoft-ui-xaml#2369) * [x] Related to #5360 (@jtippet confirms that this alone does not close it.) * [x] I work here

@jtippet

See: https://github.com/microsoft/microsoft-ui-xaml/releases/tag/v2.5.0-prerelease.200609001 > ### Notable Changes: > > Resize tab view items only once the pointer has left the TabViewItem strip (microsoft/microsoft-ui-xaml#2569) > Align TabView visuals with Edge (microsoft/microsoft-ui-xaml#2201) > Fix background of MenuFlyout in white high contrast (microsoft/microsoft-ui-xaml#2446) > TabView: Make TabViewItem consume the TabViewItemHeaderForeground theme resource (microsoft/microsoft-ui-xaml#2348) > TabView: Add tooltips to its scrolling buttons. (microsoft/microsoft-ui-xaml#2369) * [x] Related to #5360 (@jtippet confirms that this alone does not close it.) * [x] I work here

marcelwgn added 9 commits March 30, 2020 18:40

Add initial wip

645166f

More improvements, added shadow

3fa2014

remove unused corners

0c3b202

Improve hit testing

67283df

Switch to rounded corners

12ec668

Add rounded corners

e6fa0b2

Switch to single shadow

323d614

Switch to TemplateSettings

eb4e278

Cleanup

7644520

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Mar 31, 2020

stmoy requested a review from teaP March 31, 2020 21:13

Felix-Dev reviewed Apr 1, 2020

View reviewed changes

dev/TabView/TabView.xaml Outdated Show resolved Hide resolved

ranjeshj reviewed Apr 1, 2020

View reviewed changes

dev/TabView/TabViewItem.h Outdated Show resolved Hide resolved

CR feedback

4c6a410

StephenLPeters added area-TabView team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Apr 1, 2020

Merge branch 'master' into tabview-inset-radii

cc6cdf8

StephenLPeters approved these changes May 7, 2020

View reviewed changes

StephenLPeters reviewed May 13, 2020

View reviewed changes

dev/TabView/TabView.idl Outdated Show resolved Hide resolved

StephenLPeters reviewed May 13, 2020

View reviewed changes

dev/TabView/TabView.idl Outdated Show resolved Hide resolved

Mark new templateproperties as preview

1bedd9b

StephenLPeters merged commit 98a6b74 into microsoft:master May 19, 2020

marcelwgn deleted the tabview-inset-radii branch May 19, 2020 17:33

zadjii-msft mentioned this pull request May 19, 2020

Move to Microsoft.UI.Xaml 2.4.0 microsoft/terminal#5778

Merged

marcelwgn mentioned this pull request Jul 4, 2020

Tabview switch to converter instead of template settings #2827

Merged

zadjii-msft mentioned this pull request Jul 7, 2020

Update MUX to 2.5.0-prerelease.200609001 microsoft/terminal#6819

Merged

2 tasks

marcelwgn mentioned this pull request Jul 21, 2020

Updates to CornerRadiusToThicknessConverter for more filters microsoft/microsoft-ui-xaml-specs#90

Open

Align TabView visuals with Edge #2201

Align TabView visuals with Edge #2201

Conversation

marcelwgn commented Mar 31, 2020

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

mdtauk commented Mar 31, 2020

marcelwgn commented Mar 31, 2020

mdtauk commented Mar 31, 2020

marcelwgn commented Mar 31, 2020

mdtauk commented Mar 31, 2020

marcelwgn commented Mar 31, 2020

mdtauk commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmahone commented Apr 3, 2020

azure-pipelines bot commented Apr 3, 2020

kmahone commented Apr 3, 2020

stmoy commented Apr 30, 2020

StephenLPeters left a comment

Choose a reason for hiding this comment

ranjeshj commented May 14, 2020

azure-pipelines bot commented May 14, 2020

marcelwgn commented May 14, 2020

StephenLPeters commented May 18, 2020

azure-pipelines bot commented May 18, 2020

MikeHillberg commented Jun 30, 2020

marcelwgn commented Jun 30, 2020

MikeHillberg commented Jul 1, 2020

marcelwgn commented Jul 1, 2020

MikeHillberg commented Jul 1, 2020

marcelwgn commented Jul 1, 2020

MikeHillberg commented Jul 1, 2020

StephenLPeters commented Jul 2, 2020

ranjeshj commented Jul 2, 2020

ranjeshj commented Jul 3, 2020

marcelwgn commented Jul 3, 2020

MikeHillberg commented Jul 3, 2020