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

Fix frameless windows with vibrancy #11886

Merged
merged 1 commit into from Feb 12, 2018

Conversation

Projects
None yet
@MarshallOfSound
Copy link
Member

MarshallOfSound commented Feb 11, 2018

Fixes #10521

Tested with setVibrancy as well so this supports dynamically switching vibrancy still as well.

@@ -149,6 +149,8 @@ class NativeWindowMac : public NativeWindow,

bool simple_fullscreen() const { return always_simple_fullscreen_; }

void SetRenderWidgetHostOpaque(bool opaque);

This comment has been minimized.

@ckerr

ckerr Feb 11, 2018

Member

This is only called from within the class. Seems like it could be moved to inside the protected block?

impl->SetBackgroundOpaque(opaque);
}
}
}

This comment has been minimized.

@ckerr

ckerr Feb 11, 2018

Member

This is a pretty egregious demeter violation but I'm not sure what the Right Thing is here since some of these conditionals are for upstream classes out of our control 🤔

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 11, 2018

Author Member

Yeah, I tried to collapse it as much as I could but everything is just consecutive potentially nullptr values 🤔

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 11, 2018

Author Member

I inverted the logic to collapse it a bit 👍

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 11, 2018

Author Member

Still like 4 steps of separation though so idk 🤔

@@ -213,6 +215,9 @@ class NativeWindowMac : public NativeWindow,
NSRect original_frame_;
NSUInteger simple_fullscreen_mask_;

NSColor* background_color_before_vibrancy_ = nil;

This comment has been minimized.

@ckerr

ckerr Feb 11, 2018

Member

I'm not positive but should this be a base::scoped_nsobject<NSColor>?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 11, 2018

Author Member

You right 👍 😄

@MarshallOfSound MarshallOfSound force-pushed the fix-vibrant-frameless-windows branch from cede986 to cd31f8f Feb 11, 2018

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 11, 2018

@ckerr Updated 👍

@MarshallOfSound MarshallOfSound force-pushed the fix-vibrant-frameless-windows branch from cd31f8f to 636f0dd Feb 11, 2018

@codebytere
Copy link
Member

codebytere left a comment

Looks 💯 to me now!

@codebytere codebytere merged commit ae65938 into master Feb 12, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the fix-vibrant-frameless-windows branch Feb 12, 2018

@sallar

This comment has been minimized.

Copy link

sallar commented Feb 12, 2018

@codebytere when do you think this will be released?

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Feb 12, 2018

@sallar it's on the table for 2.0, i believe, so fairly soon

@HuChundong

This comment has been minimized.

Copy link

HuChundong commented Feb 24, 2018

not include in 2.0 beta1?

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 24, 2018

@HuChundong It should be, do you have a case where it's not working?

@HuChundong

This comment has been minimized.

Copy link

HuChundong commented Feb 24, 2018

@MarshallOfSound
yes,there is something wrong with me, at first i'm sorry for my poor english,thanks for your quick reply
mainWindow = new BrowserWindow({ height: 540, minHeight: 540, width: 830, minWidth: 830, useContentSize: true, webPreferences: {webSecurity: false}, resizable: true, alwaysOnTop: false, transparent: true, show: false, frame: false, titleBarStyle: 'hiddenInset', vibrancy: 'ultra-dark' })

this main window still lose round corner in high sierra 10.13.3
i need to set fullscreen and then exit full screen to get round corner back

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 24, 2018

This PR was not intended to fix the round / square corner issue. There is a separate open issue for that

@tcaer

This comment has been minimized.

Copy link

tcaer commented Feb 26, 2018

Hi, I don't know what I am doing wrong but this is still not working for me... running MacOS 10.13.3. I still get the same error of the transparent bar showing up.
Here is my window code:

function createWindow() {
	var bounds = electron.screen.getPrimaryDisplay().workAreaSize;

	/*
	* Fix the issue where the window bounds are not correct in windows 10
	*/
	var shouldMaximize;
	if (process.platform == 'win32') {
		if (bounds.x === 0 || bounds.y === 0 || bounds.x === -8 || bounds.y === -8) {
        	var screenSize = electron.screen.getPrimaryDisplay().workAreaSize
        	if ((screenSize.width === bounds.width || bounds.width - screenSize.width === 16) && (screenSize.height === bounds.height || bounds.height - screenSize.height === 16)) {
          		shouldMaximize = true
        	}
      	}
	}

	mainWindow = new electron.BrowserWindow({
		width: bounds.width,
		minWidth: 300,
		height: bounds.height,
		minHeight: 430,
		x: 0,
		y: 0,
		titleBarStyle: 'hidden-inset',
		show: false,
		frame: false,
		vibrancy: 'light'
	});

	if (shouldMaximize) {
		mainWindow.maximize();
	}

	if (process.platform == 'win32') {
		mainWindow.setMenu(null);
	}

	mainWindow.loadURL(path.join('file://', __dirname, '/public/app/index.html'));

	mainWindow.once('ready-to-show', function() {
		mainWindow.show();
	});

	mainWindow.on('closed', function() {
		mainWindow = null;
	});
}
@sallar

This comment has been minimized.

Copy link

sallar commented Feb 26, 2018

@huangruichang @tcaer Remove frame: false from your configs to see if it makes any difference.

@huangruichang

This comment has been minimized.

Copy link
Contributor

huangruichang commented Feb 26, 2018

@sallar Maybe I am not the person you wanna notice 🤣

@tcaer

This comment has been minimized.

Copy link

tcaer commented Feb 26, 2018

@sallar No luck... I get the square corners and the invisible tab :/

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 26, 2018

@tcaer Can you share repro?

@Igor10k

This comment has been minimized.

Copy link

Igor10k commented Feb 26, 2018

@tcaer add transparent: true. The invisible tab should disappear but that was working even before this PR. There's no fix for the rounded corners though.

Judging by your comments this PR fixes nothing.

@sallar

This comment has been minimized.

Copy link

sallar commented Feb 26, 2018

I just tested beta1. This fix changes nothing. Vibrancy+Square Corners was also possible before (using transparent: true and frame: false). This issue should be re-opened?

image

But judging by @MarshallOfSound's comment, there is a separate issue for the rounded corners... #10886

@tommoor

This comment has been minimized.

Copy link
Member

tommoor commented Feb 27, 2018

Also just tested and unfortunately I'm also seeing much worse behavior than before the fix.

Previously there was just a transparent area where the titlebar would normally be – now it's showing corrupted artifacts and the titlebar has reverted to the standard thickness / position even with titleBarStyle: "hidden-inset" window option.

image

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 27, 2018

@tommoor Super weird, let me test this real quick 🤔

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 27, 2018

@tommoor Two quick things

  • hidden-inset shouldn't be an option in 2.0.0-beta.1 it has been replaced with hiddenInset (the other one was deprecated for a while)
  • Can you share your exact browser window constructor options object, I can't repro what you have a screenshot of
@tommoor

This comment has been minimized.

Copy link
Member

tommoor commented Feb 27, 2018

Oh, interesting I did check the release notes for breaking changes and there wasn't a mention of that property being deprecated but that would explain the first part of the issue 😄. I wonder if having the deprecated option there caused it. Was the deprecation removed in 2.0? If so I guess we should add that to the notes.

The screenshot doesn't capture it too well, basically the entire titlebar is showing artifacts depending on what's behind - It mostly looks black / transparent. Options are:

{
      titleBarStyle: "hidden-inset",
      acceptFirstMouse: true,
      file: "index",
      webPreferences: { scrollBounce: true, textAreasAreResizable: false },
      x,
      y,
      width,
      height
}
@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 27, 2018

screen shot 2018-02-27 at 3 18 46 pm
screen shot 2018-02-27 at 3 21 04 pm

These are two screenshots off current master using

{
  vibrancy: 'dark',
  titleBarStyle: 'hiddenInset',
  transparent: true,
  frame: false,
}
@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 27, 2018

And just to prove no bamboozle, here is a screenshot from electron-quick-start using the above config using electron@2.0.0.-beta.1

screen shot 2018-02-27 at 3 24 09 pm

@tommoor

This comment has been minimized.

Copy link
Member

tommoor commented Feb 27, 2018

Thanks @MarshallOfSound – I'm out for the day now, but I'll try with 2.0 again tomorrow and the correct hiddenInset option, hopefully that does the trick 👍

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 27, 2018

@tommoor It might also be the frame and transparent options helping out, let me know if you can't make it look like mine 😆

@tommoor

This comment has been minimized.

Copy link
Member

tommoor commented Feb 27, 2018

Maybe, I can't really use those options as the square corners are unacceptable on macOS unfortunately 😞

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 27, 2018

Hm, fair enough, we had an issue somewhere for the square corner thing. Will try take a look at that later this week if I get a chance.

@tcaer

This comment has been minimized.

Copy link

tcaer commented Feb 27, 2018

Thanks so much for all the help!
There is also another bug I get when closing an app (I am using the current beta). MacOS reports that the app crashed even though it didn't.

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Feb 28, 2018

@tcaer Known bug and I think it's already fixed 😆

@stevenfabre

This comment has been minimized.

Copy link

stevenfabre commented Mar 15, 2018

Hey guys, thanks so much for fixing this!

Does this fix the corner radius bug too? In my build, the top two corners aren't rounded when I use the following settings with a webview in it #10522:

titleBarStyle: 'default',
transparent: true,
vibrancy: 'dark',

@chadluo chadluo referenced this pull request Apr 29, 2018

Open

Issues in High Sierra #13

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

@mlabod

This comment has been minimized.

Copy link

mlabod commented May 23, 2018

Rounded corners are still missing at the top edges, when vibrancy is activated.
Tested in Electron 2.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.