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

[NavigationView] Fix centering of NVI labels a bit using bottom padding #5187

Merged
merged 10 commits into from
Oct 27, 2023

Conversation

marcelwgn
Copy link
Contributor

Description

Since the text is not vertically centered inside a TextBlock but rather has a bit of top padding (even with padding set to 0), we need to adjust this by adding a bit of bottom padding.

Motivation and Context

Closes #5076

How Has This Been Tested?

Tested manually, see screenshot.

Screenshots (if appropriate):

Screenshot of settings item label using bottom

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jun 13, 2021
@mdtauk
Copy link
Contributor

mdtauk commented Jun 13, 2021

Is this change needed with Segoe UI Variable Small?

Also I think this kind of cantering happens with the buttons.

@marcelwgn
Copy link
Contributor Author

Is this change needed with Segoe UI Variable Small?

I would say "probably yes" since I don't think that the font would be much different compared to Segoe UI.

Of course this fix is not ideal, however I am not sure how else we can address this, TextBlock just occupies more space on the top than it should. Properties like TextAlignment or LineHeight don't make any difference here from my experiments.

@mdtauk
Copy link
Contributor

mdtauk commented Jun 13, 2021

Is this change needed with Segoe UI Variable Small?

I would say "probably yes" since I don't think that the font would be much different compared to Segoe UI.

Of course this fix is not ideal, however I am not sure how else we can address this, TextBlock just occupies more space on the top than it should. Properties like TextAlignment or LineHeight don't make any difference here from my experiments.

It does have a greater x-height

Using XAML Studio to test this
image
Segoe UI Variable Small

image
Segoe UI

image

@marcelwgn
Copy link
Contributor Author

That's really interesting. So if we switch to Segoe UI Variable Small, we would need to update the value again then. Makes me questions if there is a better way to solve this.

@mdtauk
Copy link
Contributor

mdtauk commented Jun 13, 2021

That's really interesting. So if we switch to Segoe UI Variable Small, we would need to update the value again then. Makes me questions if there is a better way to solve this.

This may have to be one of those things that is put under a version number conditional. The new Variable font is there in the current Insider Builds, but I have no idea if Windows 10 21H2 will get it, or if it will only be used with Windows 11/Sun Valley being announced on June 24th.

If it is to apply to all WinUI 2 and 3 apps, then does it get packaged with WinUI?

Or does it rely on an OS update for all supported version, or only Vibranium builds, so 20H2 onwards?

@marcelwgn
Copy link
Contributor Author

That's a really good question, I would argue though that it is desirable to have this be part of WinUI 3, otherwise this would introduce platform conditionals, something which is one of the things WinUI 3 wanted to eliminate.

@mdtauk
Copy link
Contributor

mdtauk commented Jun 13, 2021

That's a really good question, I would argue though that it is desirable to have this be part of WinUI 3, otherwise this would introduce platform conditionals, something which is one of the things WinUI 3 wanted to eliminate.

I would hold off on this, until there is some certainty on how fonts will be handled.

The use of the Optical Size axis to switch between Small, Text, and Display variations, would seem to be tied to the font sizing used.
Screenshot (60)
Screenshot (57)

You can see the Headings in Your Phone are using a Semibold Display font but the Live Tiles are using the Small font variant.

Text variants seem to look almost identical to Segoe UI - so will Small only apply below 14pt text? Are these to be handled by Xaml Resources, or does the Optical Size value get applied selectively.

This is something that may need input from the design team, so @chigy and the control owner @karenbtlai I think it is.

@ranjeshj ranjeshj requested a review from karkarl June 14, 2021 12:46
@roxk
Copy link

roxk commented Jun 15, 2021

Isn't that top padding accounted for accented character like � So if you put  next to Setting you wouldn't find the top padding unused - the top caret uses that. Could you please test that with this PR, whether  would appear as not center-aligned?

In general, adjusting text alignment based on one language would certainly break the layout for other languages. TextBlock's existing layout behavior seems to be correct here.

@marcelwgn
Copy link
Contributor Author

@roxk Is correct, the spacing is for accents. Below is a screenshot of how it would look with an accented character:

Screenshot of accent character next to setting icon

@StephenLPeters
Copy link
Contributor

@jevansaks and @licanhua FYI about fonts.

@StephenLPeters
Copy link
Contributor

Isn't that top padding accounted for accented character like � So if you put  next to Setting you wouldn't find the top padding unused - the top caret uses that. Could you please test that with this PR, whether  would appear as not center-aligned?

In general, adjusting text alignment based on one language would certainly break the layout for other languages. TextBlock's existing layout behavior seems to be correct here.

@chigy to confirm this has been talked about with design.

@StephenLPeters StephenLPeters added area-NavigationView NavView control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jun 15, 2021
@chigy
Copy link
Member

chigy commented Jun 16, 2021

Isn't that top padding accounted for accented character like � So if you put  next to Setting you wouldn't find the top padding unused - the top caret uses that. Could you please test that with this PR, whether  would appear as not center-aligned?
In general, adjusting text alignment based on one language would certainly break the layout for other languages. TextBlock's existing layout behavior seems to be correct here.

@chigy to confirm this has been talked about with design.

Not quite sure what this is asking but if this is asking if we account for ascenders and descenders for our design, then yes.

@mdtauk
Copy link
Contributor

mdtauk commented Jun 16, 2021

Isn't that top padding accounted for accented character like � So if you put  next to Setting you wouldn't find the top padding unused - the top caret uses that. Could you please test that with this PR, whether  would appear as not center-aligned?
In general, adjusting text alignment based on one language would certainly break the layout for other languages. TextBlock's existing layout behavior seems to be correct here.

@chigy to confirm this has been talked about with design.

Not quite sure what this is asking but if this is asking if we account for ascenders and descenders for our design, then yes.

@chigy The ask is about whether the "Settings" text needs some adjustment for vertical centering, and I asked if the change in font with Segoe UI Variable will affect this alignment.

@karkarl
Copy link
Contributor

karkarl commented Jun 16, 2021

The text being off centered is due to the Segoe UI font being 1px lower. The more logical fix is to adjust NavigationViewItemContentPresenterMargin and TopNavigationViewItemContentPresenterMargin to be 4,-1,20,0 and 8,-1,12,0 respectively. This will move the text up to the correct position.

A bottom padding of 3px on NVI is too high up, and clutters the template.

@marcelwgn
Copy link
Contributor Author

Removed the padding now and updated the margin as you suggested @karenbtlai .

Here is a screenshot of the current state (with accented A):

Screenshot of settings navigationviewitem

@chigy
Copy link
Member

chigy commented Jun 16, 2021

Thank you @mdtauk for summery. It helps.

  1. The text should be centered to the icon inclusive of ascender.
    MicrosoftTeams-image
    Top is where it is today, bottom is where it should be.

  2. Segoe UI Variable shouldn't alter the alignment. If it is, let us know.

@mdtauk
Copy link
Contributor

mdtauk commented Jun 16, 2021

Thank you @mdtauk for summery. It helps.

  1. The text should be centered to the icon inclusive of ascender.
    MicrosoftTeams-image
    Top is where it is today, bottom is where it should be.

  2. Segoe UI Variable shouldn't alter the alignment. If it is, let us know.

Segoe UI Variable in the insider build is v1.03 but in what may or may not have leaked as Windows 11, it seems to be v2.0 so maybe that version is different. But if centering it is recommended, I'm sure @chingucoding will get out done superbly

@marcelwgn
Copy link
Contributor Author

What does "Top is where it is today" mean? Today as in today on the main branch or as in today in this PR?

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karkarl
Copy link
Contributor

karkarl commented Jun 23, 2021

@chingucoding can you update the verification files? Also let me get some design eyes on it to make sure the text alignment is correct.

@marcelwgn
Copy link
Contributor Author

Updated the verification files now @karenbtlai :)

@karkarl
Copy link
Contributor

karkarl commented Jun 30, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcelwgn
Copy link
Contributor Author

I was trying to update the verification files, however it fails with the following errors:

D:\Projects\microsoft-ui-xaml\tools [dev/nvi-text-allign-fix ≡]> ./GenerateVisualVerificationUpdates.ps1 BuildId=201907
Invoke-RestMethod: D:\Projects\microsoft-ui-xaml\tools\GenerateVisualVerificationUpdates.ps1:41:17
Line |
  41 |  … $dropData = Invoke-RestMethod -Uri "https://dev.azure.com/ms/microsof …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     |          Page not found.     html {     height: 100%; }  body {     font-family: "-apple-system", BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", Helvetica, Ubuntu, Arial, sans-serif, "Apple Color
     | Emoji",         "Segoe UI Emoji", "Segoe UI Symbol";     font-size: 14px;     height: 100%;     margin: 0;     background-color: #fff; }  .container {     height: 100%;     display: flex;     flex-grow: 1;
     | flex-direction: column; }  .header {     font-size: 14px;     line-height: 20px;     padding-right: 40px;     display: flex;     box-shadow: 0px 1px 0px rgba(0, 0, 0, 0.08); }  .header > div, .header > a {
     | margin: 14px 0px 14px 32px;     text-decoration: none;     color: rgba(0, 0, 0, 0.55); }  .header > div:first-child {     flex-grow: 1;     margin-left: 14px;     color: #0078d4;     display: flex; }  .header >
     | div:first-child > div {     margin-left: 14px; }  .content-container {     display: flex;     flex-direction: column;     justify-content: center;     margin-top: 126px; }  .content {     display: flex;
     | justify-content: center;     align-items: center; }  .error-info {     display: flex;     flex-direction: column;     width: 500px; }  .title {     font-size: 28px;     font-weight: bold;     line-height: 40px;
     | letter-spacing: -0.04em;     color: #212121;     margin-bottom: 16px; }  .details {     font-size: 14px;     line-height: 20px;     color: #212121;     margin-bottom: 16px; }  .details a {     text-decoration:
     | none;     color: #0078d4; }  .reference-data {     font-size: 12px;     line-height: 16px;     color: rgba(0, 0, 0, 0.55);     margin-bottom: 32px; }  .logo {     height: 500px;     width: 500px; }
     | .action-container {     display: flex; }  .action {     background-color: #0078d4;     color: #ffffff;     align-items: center;     border-radius: 2px;     border: 1px solid transparent;     cursor: pointer;
     | display: inline-flex;     flex-direction: row;     flex-shrink: 0;     font-family: inherit;     font-size: inherit;     font-weight: 600;     justify-content: center;     outline: none;     overflow: visible;
     | padding: 6px 12px;     position: relative;     transition: background 80ms linear;     text-decoration: none; }  .action:focus {     box-shadow: 0 0 0 3px rgba(0, 120, 212, 0.35); }  .action:hover {
     | background-color: rgb(0, 103, 181) }  .action:active {     background-color: rgb(0, 91, 161) }  .secondary-action {     text-decoration: none;     color: #0078d4;     margin-top: 16px; }  .more-info {
     | margin: auto;     display: flex;     flex-direction: column;     padding: 0 40px; }  .more-info > div:first-child {     font-size: 15px;     font-weight: 700; }
     | Azure DevOps                  Service Status         Support         @AzureDevOps                                                                                  404 - Page not found             Looks like this
     | page doesn’t exist or can’t be found. Make sure the URL is correct.                            7/20/2021 6:38:37 PM (UTC)               53aececa-9788-43b4-9d69-c68e25e4f640
     | Go back home

Downloading 'helixTestOutput.zip'. Please wait, this will take a few moments...
Invoke-WebRequest: D:\Projects\microsoft-ui-xaml\tools\GenerateVisualVerificationUpdates.ps1:49:32
Line |
  49 |          Invoke-WebRequest -Uri $dropData.resource.downloadUrl -OutFil …
     |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot validate argument on parameter 'Uri'. The argument is null or empty. Provide an argument that is not null or empty, and then try the command again.

Done!
Downloaded file to C:\Users\Marcel\AppData\Local\Temp\had3tr0r.yms\helixTestOutput.zip
Extracting files to C:\Users\Marcel\AppData\Local\Temp\ybbc3slu.51p
Expand-Archive: D:\Projects\microsoft-ui-xaml\tools\GenerateVisualVerificationUpdates.ps1:60:5
Line |
  60 |      Expand-Archive -Path $downloadFilePath -DestinationPath $artifact …
     |      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | The path 'C:\Users\Marcel\AppData\Local\Temp\had3tr0r.yms\helixTestOutput.zip' either does not exist or is not a valid file system path.

Found these updated verification files: 
No verification files were found, did you call the script on the correct directory?

Not sure why this happens, maybe this was due to the switch to a different build pool?

@beervoley
Copy link
Contributor

@chingucoding cam you merge up to date main? I will kick off the pipeline after that

@marcelwgn
Copy link
Contributor Author

@beervoley Branch is up to date now :)

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Aug 21, 2023

@chingucoding Looks like there are new conflicts. Would you be able to resolve them?

@ojhad
Copy link
Contributor

ojhad commented Aug 22, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karkarl
Copy link
Contributor

karkarl commented Aug 23, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Sep 26, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Oct 24, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad ojhad merged commit cf6e864 into microsoft:winui2/main Oct 27, 2023
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NavigationView] Settings label not vertically aligned with settings icon
8 participants