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

Fixes visual glitch near the inner arcs of selected TabViewItems when scale is 150% #6001

Closed
wants to merge 1 commit into from

Conversation

eugenegff
Copy link
Contributor

@eugenegff eugenegff commented Sep 30, 2021

Description

Selected TabViewItems are drawn with borders, that are connected by inner arc with bottom border. When scale is 150% there are visual glitches, as shown here:
image

The reason is that at 150% border line position is somewhat rounded, as well as the position of the triangle that should cover and hide bottom part of it. This fix extends the shape of small covering 4x4 triangle into trapezoid by additional 4x4 rect on the inner side of tab, so that when border line width became rounded up it would still be covered.

Idea of the fix:
image

Screenshot with the fix
image

It is not important that additional rect is "so big" as 4x4, as TabViewItem.TabContainer is drawn above it, and at the same time TabViewItem.TabContainer.Border below it - this was tested by filling those small rectangles (actually trapezoids now) with red color - it does not leak past the border to inner part of the TabViewItem.TabContainer

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Sep 30, 2021
@eugenegff eugenegff changed the title Fixes thrash near inner arcs of selected TabViewItems when scale is 150% Fixes visual glitch near the inner arcs of selected TabViewItems when scale is 150% Sep 30, 2021
@ranjeshj ranjeshj added area-TabView team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Sep 30, 2021
@beervoley
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@beervoley
Copy link
Contributor

@eugenegff could you please test couple of things:

  1. When having Width mode set to Equal, switch between tabs and see if there's no visual moving of the tabs once you switch. I believe due to the added negative margin there will be annoying jiggling of tabs once selected tab is changed.
  2. Could you please also try changing the background color of the Tab and seeing if that little added 4x4 square doesn't interfere with the background: try with solid/semi-transparent.

@eugenegff
Copy link
Contributor Author

eugenegff commented Sep 30, 2021

@eugenegff could you please test couple of things:

  1. When having Width mode set to Equal, switch between tabs and see if there's no visual moving of the tabs once you switch. I believe due to the added negative margin there will be annoying jiggling of tabs once selected tab is changed.

I tested it - no jumps/jiggling/etc. Those 4 pathes LeftRadiusRenderArc, LeftRadiusRenderTriangle, RightRadiusRenderArc, RightRadiusRenderTriangle are located in zero width grid columns, and negative margins are used to keep those grid columns zero width. Before my fix arcs and triangles via negative margins were extended outside of TabViewItem.LayoutRoot.Grid, after the fix triangles are extended both ways, outside as before and now inside too.

  1. Could you please also try changing the background color of the Tab and seeing if that little added 4x4 square doesn't interfere with the background: try with solid/semi-transparent.

The whole mechanic of "changing" direction of border stroke via hiding it with patch placed atop requires opaque filling color. So TabViewItemHeaderBackgroundSelected = SolidBackgroundFillColorTertiaryBrush is not a coincidense - it is requirement of the current scheme.

So, full transparency will work as good/bad as before, and semitransparency for TabViewItemHeaderBackgroundSelected will probably work less good - thats the price for fixing situation where stock TabView always looks bad on 150% scale.

The reason of all of this is because extra triangles are attached outside of the border, and are required to mimic logic that border uses to determine own thichness - and it is not trivial, as border seems to round own thickness and offset to whole number of pixels. I can imagine technique of transferring triangles logically inside the border, still keeping them hanging outside via negative margins but always aligned to inner side of border. We need to remove Border.Padding for this to work, moving it to some other element. But such modification is so massive so I believe that it will not pass.

Reducing the width of additional rect from 4pt to 1pt will make semitransparent situation better, but never ideal.

@eugenegff
Copy link
Contributor Author

Unrelated RepeaterTests are failed, pls rerun azp

@beervoley
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eugenegff
Copy link
Contributor Author

Frankly, I don't like current solution, and think that "technique of transferring triangles logically inside the border, still keeping them hanging outside via negative margins but always aligned to inner side of border." is more appropriate, even if resulting modifications would be much more massive.

@StephenLPeters
Copy link
Contributor

Thanks @eugenegff! the current design of tab view really does require solid color backgrounds... I think leaning further into that requirement is probably okay... @ranjeshj and @chigy FYI

@eugenegff
Copy link
Contributor Author

@teaP FYI

@teaP
Copy link
Contributor

teaP commented Oct 13, 2021

I'm trying this week to convert the entire shape into one path, so that we don't have seams to worry about. But it's hard to say yet if it'll work or when the fix would be ready.

@eugenegff
Copy link
Contributor Author

eugenegff commented Oct 13, 2021

I'm trying this week to convert the entire shape into one path, so that we don't have seams to worry about. But it's hard to say yet if it'll work or when the fix would be ready.

That will obsolete this pull request then. Do you keep in mind that Border position, size and Border.Thickness are rounded to the whole number of hardware pixels to keep it non-blured? Very important for 125%, 150%, 250% scale factors, especially as 150% is the default scale factor for 27" 4K monitors

@teaP
Copy link
Contributor

teaP commented Oct 13, 2021

That will obsolete this pull request then. Do you keep in mind that Border position, size and Border.Thickness are rounded to the whole number of hardware pixels to keep it non-blured? Very important for 125%, 150%, 250% scale factors, especially as 150% is the default scale factor for 27" 4K monitors

Path has a Stroke/StrokeThickness I'm hoping will work. I'm not feeling extremely confident yet, though. I keep thinking there is some adjustment or flag I can set on the current corners to make it work, but so far nothing has...

@eugenegff
Copy link
Contributor Author

eugenegff commented Oct 13, 2021

Path has a Stroke/StrokeThickness I'm hoping will work. I'm not feeling extremely confident yet, though. I keep thinking there is some adjustment or flag I can set on the current corners to make it work, but so far nothing has...

Path.StrokeThickness is not adjusted automatically to pixels, nor the Path bound is aligned to pixel grid. You can see on this 150% scale image as inner arcs are much narrower than Border, as 1pt = 1.5px here for Path, but Border's 1pt rounded to 2px.

The only way to generate non blured border - is to manually align everything to pixel grid, and fill rather than stroke the result. Otherwise result on 150% scale would be much worse than current.

image

@gabbybilka
Copy link
Member

gabbybilka commented Oct 20, 2021

Edit: Initially commented "azp run" but on the incorrect issue, sorry folks! 😅

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ranjeshj ranjeshj requested a review from teaP October 21, 2021 20:33
Copy link
Contributor

@teaP teaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created PR #6282 which fixes this problem in a different way.

@eugenegff
Copy link
Contributor Author

eugenegff commented Nov 11, 2021

I created PR #6282 which fixes this problem in a different way.

Let then close this PR

@eugenegff eugenegff closed this Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-TabView team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants