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

Draw top border ourselves to remove white borders on the window #4577

Closed
wants to merge 16 commits into from
Closed

Draw top border ourselves to remove white borders on the window #4577

wants to merge 16 commits into from

Conversation

beviu
Copy link
Contributor

@beviu beviu commented Feb 13, 2020

Summary of the Pull Request

Previously we used DwmExtendFrameIntoClientArea to let DWM draw the native top frame border behind our custom title bar. But calling this makes the window border white (I don't know why). So instead now, we just stop using DwmExtendFrameIntoClientArea and try to draw the border like DWM ourselves (see #3425 (comment)).

I took the code from Chromium to get the correct border color:
https://github.com/chromium/chromium/blob/af5c81b48faf773c19b7f3c495a1c0202f9c5b00/chrome/browser/themes/theme_service_win.cc#L193

Issue: When this setting is disabled:
image
The result is not perfect because then the border should be transparent and I don't know how to make it transparent.
So it will look the same on a white background than on a black background and that means that depending the background behind the window, the top border can look slightly off, especially when the custom tab color PR get merged and the title bar's color can be changed.

This is a temporary solution to the white border problem.

I'm sorry if I explain this badly. If you don't understand a part, make sure to let me know and I will explain it better.

References

PR Checklist

  • Closes a part of Terminal has white borders in dark mode (Update ThemeUtils::SetWindowFrameDarkMode to use new DWM API) #3425 (the "Terminal has white borders in dark mode" part, but not the "Update ThemeUtils::SetWindowFrameDarkMode to use new DWM API" part)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

In the issue liked above (#3425) there were two ideas to solve the problem:

Imitation of the system top border (this PR, #3425 (comment)):

  • Done by Chromium and it looks like also Firefox and the thing that powers UWP apps (because they are affected by the same weird bugs, see below)
  • Not perfect in this PR (see PR description)
  • Must be updated if an OS update changes the behavior (like 1809 which removed accented borders when "Title bars and window borders" is disabled)
  • No one seems to do it correctly when the accent color setting is disabled
    • Chrome (continues to paint the top border with the accent color even when the setting is disabled): Chrome
    • Firefox (no transparency, wrong color): image
    • UWP apps, the best imitation (have transparency ✔️ but the color isn't the exact same, really hard to notice):
      UWP apps
  • BTW, weird bugs start happening once you starting switching between light/dark mode (I'm running 1903)
    • Settings app: UWP Settings
    • Firefox: Firefox
  • But you have to be looking really hard to notice any difference

DwmExtendFrameIntoClientArea + DWM API to force dark border (the original idea in the title of the issue):

  • Makes sure that all of the borders are consistent at least
  • Solution used by explorer.exe (I think) when dark mode is enabled to not have a white border across the window like we have now
  • Unofficial API = might break = must be updated if the API changes

Validation Steps Performed

I launched the app with dark/light and accent color enabled/disabled combinations.

But I had a bug where Windows would put a white border on every window after I switch from dark to white mode so every app including the Terminal was bugged until the next sign out. I will test it again better later, by signing out every time to "reset" that bug (to do). EDIT: done.

@beviu
Copy link
Contributor Author

beviu commented Feb 13, 2020

Oops, I totally forgot about light mode. This is not ready.
EDIT: done

@DHowett-MSFT
Copy link
Contributor

I think you'll get a bit more of the transparent border behavior by keeping DwmExtend and passing {0}. I believe the firefox/chromium solution is ...Extend({0}) and then draw the top bar itself.

@beviu
Copy link
Contributor Author

beviu commented Feb 14, 2020

I just tried {0} and it doesn't seem to change anything unfortunately.

Please note that Chromium and Firefox don't have a solution.

Chromium doesn't do it correctly either. It just "ignores" the "Title bars and window borders" setting for the top border so it always paint the border with the accent color. And in that case, there is no transparency (I think) so that is correct but it shouldn't paint the border with the accent color in the first place:
image
There shouldn't be a blue line at the top.

Firefox is wrong too:
image

@beviu
Copy link
Contributor Author

beviu commented Feb 15, 2020

There appears to be a slight lag when unfocusing the window between the top border and others. It stays blue/gray for a little bit longer (a few ms).
EDIT: fixed

@beviu
Copy link
Contributor Author

beviu commented Feb 20, 2020

This won't work well when #3789 (custom color for terminal tabs = custom title bar color) is merged because my workaround that sets a top border color to an opaque color that look like the system's border might become too obvious with some colors.

Should I close this (wait until there is a better fix without weird workarounds) or not (because this is a temporary workaround to #3425 and having 4 white borders is probably worse than 1 border that looks slightly wrong?)?

@beviu beviu closed this Mar 5, 2020
@beviu
Copy link
Contributor Author

beviu commented Mar 5, 2020

see #4817

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants