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

White borders are back in 1.17 #14744

Open
zadjii-msft opened this issue Jan 27, 2023 · 13 comments
Open

White borders are back in 1.17 #14744

zadjii-msft opened this issue Jan 27, 2023 · 13 comments
Labels
Area-Theming Anything related to the theming of elements of the window Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Jan 27, 2023

This problem is back in v1.17, I guess due to the recent changes around theming. The grey border is there to start with until you switch to another window and back (presumably it's part of the dark theme and doesn't get repainted until switching). Once you switch back it is white and will stay so even after closing/re-opening.

Steps to reproduce:

  • Delete settings.json and state.json
  • Open Terminal and switch to light theme
  • Border is grey
  • Alt tab to another window then back again
  • Border is now forever white

Originally posted by @harristom in #6620 (comment)

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 27, 2023
@zadjii-msft
Copy link
Member Author

1000% this is due to one of the last commits in #14536.

@harristom Which OS version are you on? Just to make sure I fix it for the right platform(s)

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Area-Theming Anything related to the theming of elements of the window labels Jan 27, 2023
@harristom
Copy link

harristom commented Jan 27, 2023 via email

@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 1, 2023
@zadjii-msft
Copy link
Member Author

Wait hold on, this is by design now? more or less. To try and tease this apart, I think there's two parts to this thread:

  • Part the first: The main issue here seems that when you switch to terminal light theme, the borders are white. Which is by-design now. We've got all these new toggles for different theme properties, we should probably actually respect the user's selected theme for the window. This is changed in 1.17, but seems like it changed to be more correct.
  • Part the second: "When I switch the terminal theme on Win10, the window border doesn't update till the window is unfocused/refocused", and I'm guessing that's a NCPAINT issue - we previously noted that the SetWindowAttribute thing in TerminalThemeHelpers needed to be before a NCPAINT. I'm betting that on win11, where that API is officially supported, they actually do force the NCPAINT when the API is called. On win 10, not so much.

Does all that make sense/?

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 4, 2023
@zadjii-msft zadjii-msft added the Resolution-By-Design It's supposed to be this way. Sometimes for compatibility reasons. label Apr 4, 2023
@harristom
Copy link

Is it definitely by design? As mentioned in the original report and the PR fixing it the white border makes the Terminal look out of place compared to other apps which all use a grey border. Obviously it's only a very minor issue but it makes the app look out of place and hard to tell whether it has focus (since for other apps a darker border means focussed and a lighter one means unfocussed whereas Terminal is doing the opposite).

@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Apr 21, 2023
@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

2 similar comments
@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@DHowett
Copy link
Member

DHowett commented Apr 21, 2023

Sorry, the bot went haywire. As usual. I think this is worth talking about :)

@microsoft-github-policy-service microsoft-github-policy-service bot removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Apr 21, 2023
@DHowett DHowett added Needs-Discussion Something that requires a team discussion before we can proceed and removed Resolution-By-Design It's supposed to be this way. Sometimes for compatibility reasons. labels Apr 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Apr 25, 2023
@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

1 similar comment
@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@microsoft-github-policy-service microsoft-github-policy-service bot removed this from the Terminal v1.18 milestone Apr 29, 2023
@carlos-zamora carlos-zamora added Priority-3 A description (P3) and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. Priority-1 A description (P1) Needs-Discussion Something that requires a team discussion before we can proceed labels Jun 5, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jun 5, 2023
@carlos-zamora carlos-zamora reopened this Jun 5, 2023
@carlos-zamora carlos-zamora added this to the Terminal v1.19 milestone Jun 5, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 5, 2023
@zadjii-msft
Copy link
Member Author

Hey just found this camping in the backlog. This has kinda turned into a complex space now.

  • Before 1.17, we'd always use the dark theme border for the Terminal window, even in light mode.
    • (personally, I agree that was preferable, but that's my opinion)
  • Now in 1.17, we'll use the border color that matches the window.applicationTheme. In light mode, that means a white border.
  • BUT NOW IN 1.18: we added a pair of window.frame and window.unfocusedFrame theme properties in Add support for setting the window frame color #15441. This will let a user override the frame color and use whatever.
    • UNFORTUNATELY, that API doesn't work on Windows 10, so this isn't necessarily immediately helpful for you, but it does work

I dunno how I feel about reverting the "the window frame follows the OS theme" bit (back to "always use the dark border"). I feel like this is more correct now, even if UWPs and Win32 apps have been using different borders for years.

@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 19, 2023
@zadjii-msft
Copy link
Member Author

discussion notes:

  • Sure, we should revert this back. So that null for window.border will always use the Dark theme border
  • While we're here, if people do use window.border on Windows 10, we should try to compute the lightness and use that to figure out what theme border to use (since the real RGB color API won't work there)

@zadjii-msft zadjii-msft modified the milestones: Terminal v1.19, Backlog Oct 2, 2023
@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Oct 2, 2023
@zadjii-msft zadjii-msft removed their assignment Oct 2, 2023
@github-project-automation github-project-automation bot moved this to Should be written in Terminal Walkthroughs Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Theming Anything related to the theming of elements of the window Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
Status: Should be written
Development

No branches or pull requests

4 participants