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

Terminal has white borders in dark mode (Update ThemeUtils::SetWindowFrameDarkMode to use new DWM API) #3425

Closed
beviu opened this issue Nov 3, 2019 · 25 comments · Fixed by #4817
Assignees
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@beviu
Copy link
Contributor

beviu commented Nov 3, 2019

From #3394:

I'd be most comfortable merging this without the dark theme fix (which I know will cause there to be white borders around the entire window instead of just at the bottom) and then bringing in the dark theme changes when the other fix has flowed back down from Windows.

@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 Nov 3, 2019
@beviu beviu closed this as completed Nov 3, 2019
@beviu beviu reopened this Nov 4, 2019
@DHowett-MSFT DHowett-MSFT added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. 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 labels Nov 4, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 4, 2019
@DHowett-MSFT DHowett-MSFT added Help Wanted We encourage anyone to jump in on these. Needs-Tag-Fix Doesn't match tag requirements labels Nov 4, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 4, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Nov 4, 2019
@DHowett-MSFT DHowett-MSFT changed the title Update ThemeUtils::SetWindowFrameDarkMode to use new DWM API Terminal has white borders in dark mode (Update ThemeUtils::SetWindowFrameDarkMode to use new DWM API) Nov 26, 2019
@DHowett-MSFT
Copy link
Contributor

Finally figured this out!

If we set the extended frame margins to {0}, we can fully suppress the white border. The cost, though, is that we have to paint our own accent color -- but only on the top of the window. The borders stay the same.

Accent On

image

Accent Off

image

Notice how the right border is visible, just dim. This looks like Chrome and Firefox as well.

@beviu
Copy link
Contributor Author

beviu commented Nov 27, 2019

Maybe one way is to use DwmGetColorizationColor and WM_DWMCOLORIZATIONCOLORCHANGED to get the accent color and HKCU\Software\Microsoft\Windows\DWM\ColorPrevalence to detect if we need to put the accent color in the title bar or not. Last time I tried, watching for registry changes for this key didn't work in the app but worked in another Win32 project (so maybe because it's a store app? I don't really know how this works) so I couldn't make it update when you change the setting at runtime. So I abandoned this idea to draw the top border ourselves but looking back I think that it's not a big deal.

Also, note that the border's color is a little bit brighter than the title bar's color so the value from DwmGetColorizationColor needs to be modified. You can see a 1px border with lighter color than the title bar if you zoom in this screenshot and you look at the top and left of the window:
image

@DHowett-MSFT
Copy link
Contributor

@greg904 we probably need both actually -- to listen to WM_DWMCOLORIZATIONCOLORCHANGED and to read ColorPrevalence. We shouldn't need to listen to the registry key directly, since it should be wrapped up in the WM.... Chromium seems to have an algorithm for figuring out the accent border color; perhaps there's something we can learn from it?

@beviu
Copy link
Contributor Author

beviu commented Nov 28, 2019

I would assume that WM_DWMCOLORIZATIONCOLORCHANGED is only sent when the accent changes (DwmGetColorizationColor) but not when the accent color on title bar and borders setting (ColorPrevalence) is toggled based on the name. But I need to check this. EDIT: I was wrong, it's also sent when that setting changes.

I found this from Chromium: https://github.com/chromium/chromium/blob/af5c81b48faf773c19b7f3c495a1c0202f9c5b00/chrome/browser/themes/theme_service_win.cc#L60
https://github.com/chromium/chromium/blob/af5c81b48faf773c19b7f3c495a1c0202f9c5b00/chrome/browser/ui/views/frame/glass_browser_frame_view.cc#L609

Note that the algorithm that Chromium uses isn't working correctly in 1809+ when ColorPrevalence is disabled: https://bugs.chromium.org/p/chromium/issues/detail?id=950362.

The top border has the accent color but it should not, it should be gray like the other left, right and bottom borders:
image

@lukeblevins
Copy link

Not to get too off-topic here, but whichever method Microsoft Edge uses to draw the top border has the unintended consequence of mis-matching a transparent top border with the other 3 sides. This is visible only when the Windows theme is set to "dark" while the app theme is "light."
image

@Stanzilla
Copy link
Contributor

@DHowett-MSFT is a fix for this coming in the next release?

@Piroro-hs
Copy link

The terminal still shows a 1 pixel thin white border on the left of unfocused window in some cases. In my case, the border only appears when the screen scale is 125% and disappears after window resizing.

just after launch

resized

Windows 10 18362.535
Windows Terminal 0.7.3451.0

@DHowett-MSFT
Copy link
Contributor

@Stanzilla a fix will be coming for this bug in the release that comes after the bug is closed 😄

@gwojan
Copy link

gwojan commented Jan 14, 2020

One thing I've just noticed is the white border is much more prominent on laptops with NVIDIA video adapters. I have three different Dell laptops and the one with AMD graphics the border is not nearly as noticeable. Does that make any sense at all?

All three laptops have Dark Mode enabled with Windows 10 1903.

@ghost ghost removed the In-PR This issue has a related PR label Feb 20, 2020
@beviu
Copy link
Contributor Author

beviu commented Feb 20, 2020

This issue should probably be split into two issues:

  1. Terminal has white borders in dark mode
  2. Update ThemeUtils::SetWindowFrameDarkMode to use new DWM API

Because 2 is one of many solutions to 1 but it's not the only solution. For instance, #3425 (comment) is another solution.

@cinnamon-msft cinnamon-msft added Priority-2 A description (P2) and removed Priority-3 A description (P3) labels Feb 28, 2020
@beviu
Copy link
Contributor Author

beviu commented Mar 2, 2020

The more that I think about it, the more I wonder if it is possible that one of the devs who work at Microsoft can ask the people working on DWM (because it's also from Microsoft) why calling DwmExtendFrameIntoClientArea gives us a white border and how we can prevent it to fix this?

Since the beginning, I have been trying a lot of different weird workarounds to patch this because I have no idea how it works and I really want it fixed because it burns my eyes :) but nothing is really good. However maybe the people working on DWM could find a really simple fix in a few seconds because they know what's happening under the hood?

I used a program to see what API calls the ApplicationFrameHost.exe executable (which I think is the thing that manages UWP apps' windows) makes and it does call DwmExtendFrameIntoClientArea with parameters { 0, 0, 32, 0 } but it doesn't have that weird white border so I'm missing something.

@DHowett-MSFT
Copy link
Contributor

@greg904 you're totally right. The main issue is, though, that we want to ship something that'll work properly (or quasi-properly) all the way back to 19H1 (build 18362), and any solution we get from the DWM team is going to have a very long lead time.

I've been around the block with them a few times about what we "should" be doing to be good platform citizens but also blend in with other applications on the system. 😄

I wouldn't be upset about a solution that calls DwmExtendFrameIntoClientArea(hwnd, {0}) and paints the top of the window -- Chromium (and therefore Edge) does it and Firefox does it. There's some downsides to doing it this way, but I would accept minor downsides for the visual improvement we'd get.

@DHowett-MSFT
Copy link
Contributor

@greg904 (be careful investigating ApplicationFrameHost -- windows for modern applications get a little bit of special treatment, and they're actually slightly different in win32k, so we can't derive too much info from looking at how they work.)

@DHowett-MSFT
Copy link
Contributor

Alright, so here's the deal. I got agreement from the DWM team to ship an interim solution while they work to resolve the issue for Windows vNext. This interim solution comes in the form of a single DLL consumed through a NuGet package.

It's not amazing from the perspective of an open-source project, and I understand that. I'm happy to answer any questions.

@ghost ghost closed this as completed in #4817 Mar 5, 2020
ghost pushed a commit that referenced this issue Mar 5, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Mar 5, 2020
@andrew-boyarshin
Copy link

andrew-boyarshin commented Mar 6, 2020

@DHowett-MSFT it's great to see collaboration with DWM team!
It is, however, disappointing to see the solution to the issue being private and non-opensource. I imagine this is due to it using private DWM API such as window attributes №19-20 and UXTHEME ShouldAppsUseDarkMode method resolved by ordinal. It appears DWM team doesn't want to widen the API surface, make these official. I hope Windows Composition team will consider sharing more (maybe non-documented, without examples, but at least headers and/or LIB files) to enable advanced 3rd party developers, those who understand the responsibility of using non-documented APIs, to solve the issues they face.

@DHowett-MSFT
Copy link
Contributor

@andrew-boyarshin watch this space 😄

I think us using UXTHEME isn't entirely required; DWM likely does this for us, but we were following somebody else's example.

We'll get it all ironed out in the end.

@DHowett-MSFT DHowett-MSFT unpinned this issue Mar 18, 2020
@michalhudecek
Copy link

michalhudecek commented Jun 18, 2020

Why is the issue closed? The part about white border when the terminal window is focused is not fixed. I have accent color for title bars and window borders turned off. I cannot find an issue for that in GitHub

image

@DHowett
Copy link
Member

DHowett commented Jun 18, 2020

Light system theme, light terminal theme, light borders.

@michalhudecek
Copy link

But they are usually black. See Windows Settings screenshot

image

@michalhudecek
Copy link

Or Edge
image

@michalhudecek
Copy link

Some more examples - all Microsoft apps!

image
image
image
image
image

@DHowett
Copy link
Member

DHowett commented Jun 18, 2020

Your point is made.

@DHowett
Copy link
Member

DHowett commented Jun 18, 2020

However, to answer your question:

Why is the issue closed?

Because this is the issue for terminal has white borders in dark mode, which is 100% fixed.

If you want to discuss changing the default for light mode, please file a separate issue 😄

@michalhudecek
Copy link

There are many similar issues but they were closed too. It's true that some of them are only about dark mode but some of them are about white borders in general, eg. #4837

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.