From b582b0a7c30bb4db1d663ee82b1b21174d6fc037 Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Mon, 12 Aug 2019 12:17:24 -0700 Subject: [PATCH 1/6] fix: normalize behavior of `win.setOpacity()` for invalid number values across operating systems --- atom/browser/native_window_mac.mm | 4 ++- atom/browser/native_window_views.cc | 8 +++-- spec/api-browser-window-spec.js | 54 +++++++++++++++++++++-------- 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/atom/browser/native_window_mac.mm b/atom/browser/native_window_mac.mm index 8ac4746a63fba..46570e307ddef 100644 --- a/atom/browser/native_window_mac.mm +++ b/atom/browser/native_window_mac.mm @@ -22,6 +22,7 @@ #include "atom/common/options_switches.h" #include "base/mac/mac_util.h" #include "base/mac/scoped_cftyperef.h" +#include "base/numerics/ranges.h" #include "base/strings/sys_string_conversions.h" #include "components/remote_cocoa/app_shim/bridged_native_widget_impl.h" #include "content/public/browser/browser_accessibility_state.h" @@ -1047,7 +1048,8 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) { } void NativeWindowMac::SetOpacity(const double opacity) { - [window_ setAlphaValue:opacity]; + const double boundedOpacity = base::ClampToRange(opacity, 0.0, 1.0); + [window_ setAlphaValue:boundedOpacity]; } double NativeWindowMac::GetOpacity() { diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc index dac82de74b14c..ccc4c3b51af5a 100644 --- a/atom/browser/native_window_views.cc +++ b/atom/browser/native_window_views.cc @@ -25,6 +25,7 @@ #include "atom/common/draggable_region.h" #include "atom/common/native_mate_converters/image_converter.h" #include "atom/common/options_switches.h" +#include "base/numerics/ranges.h" #include "base/strings/utf_string_conversions.h" #include "content/public/browser/browser_thread.h" #include "native_mate/dictionary.h" @@ -866,6 +867,7 @@ bool NativeWindowViews::HasShadow() { void NativeWindowViews::SetOpacity(const double opacity) { #if defined(OS_WIN) + const double boundedOpacity = base::ClampToRange(opacity, 0.0, 1.0); HWND hwnd = GetAcceleratedWidget(); if (!layered_) { LONG ex_style = ::GetWindowLong(hwnd, GWL_EXSTYLE); @@ -873,9 +875,11 @@ void NativeWindowViews::SetOpacity(const double opacity) { ::SetWindowLong(hwnd, GWL_EXSTYLE, ex_style); layered_ = true; } - ::SetLayeredWindowAttributes(hwnd, 0, opacity * 255, LWA_ALPHA); + ::SetLayeredWindowAttributes(hwnd, 0, boundedOpacity * 255, LWA_ALPHA); + opacity_ = boundedOpacity; +#else + opacity_ = 1.0; // setOpacity unsupported on Linux #endif - opacity_ = opacity; } double NativeWindowViews::GetOpacity() { diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index e262afd09750a..476954f334ac8 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1214,24 +1214,48 @@ describe('BrowserWindow module', () => { }) describe('BrowserWindow.setOpacity(opacity)', () => { - it('make window with initial opacity', () => { - w.destroy() - w = new BrowserWindow({ - show: false, - width: 400, - height: 400, - opacity: 0.5 + describe('Windows and Mac', function () { + if (process.platform === 'linux') { + this.skip() + } + + it('make window with initial opacity', () => { + const w = new BrowserWindow({ show: false, opacity: 0.5 }) + expect(w.getOpacity()).to.equal(0.5) + }) + + it('allows setting the opacity', () => { + const w = new BrowserWindow({ show: false }) + expect(() => { + w.setOpacity(0.0) + expect(w.getOpacity()).to.equal(0.0) + w.setOpacity(0.5) + expect(w.getOpacity()).to.equal(0.5) + w.setOpacity(1.0) + expect(w.getOpacity()).to.equal(1.0) + }).to.not.throw() + }) + + it('clamps opacity to [0.0...1.0]', () => { + const w = new BrowserWindow({ show: false, opacity: 0.5 }) + w.setOpacity(100) + expect(w.getOpacity()).to.equal(1.0) + w.setOpacity(-100) + expect(w.getOpacity()).to.equal(0.0) }) - assert.strictEqual(w.getOpacity(), 0.5) }) - it('allows setting the opacity', () => { - assert.doesNotThrow(() => { - w.setOpacity(0.0) - assert.strictEqual(w.getOpacity(), 0.0) + + describe('Linux', function () { + if (process.platform !== 'linux') { + this.skip() + } + + it('sets 1 regardless of parameter', () => { + const w = new BrowserWindow({ show: false }) + w.setOpacity(0) + expect(w.getOpacity()).to.equal(1.0) w.setOpacity(0.5) - assert.strictEqual(w.getOpacity(), 0.5) - w.setOpacity(1.0) - assert.strictEqual(w.getOpacity(), 1.0) + expect(w.getOpacity()).to.equal(1.0) }) }) }) From aeb3e96789ade9827ce655d269310c53f3a8ef1d Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Tue, 13 Aug 2019 10:27:00 -0700 Subject: [PATCH 2/6] expect -> assert --- spec/api-browser-window-spec.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 476954f334ac8..4f34e882d67b2 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1221,27 +1221,27 @@ describe('BrowserWindow module', () => { it('make window with initial opacity', () => { const w = new BrowserWindow({ show: false, opacity: 0.5 }) - expect(w.getOpacity()).to.equal(0.5) + assert.strictEqual(w.getOpacity(), 0.5) }) it('allows setting the opacity', () => { const w = new BrowserWindow({ show: false }) expect(() => { w.setOpacity(0.0) - expect(w.getOpacity()).to.equal(0.0) + assert.strictEqual(w.getOpacity(), 0.0) w.setOpacity(0.5) - expect(w.getOpacity()).to.equal(0.5) + assert.strictEqual(w.getOpacity(), 0.5) w.setOpacity(1.0) - expect(w.getOpacity()).to.equal(1.0) + assert.strictEqual(w.getOpacity(), 1.0) }).to.not.throw() }) it('clamps opacity to [0.0...1.0]', () => { const w = new BrowserWindow({ show: false, opacity: 0.5 }) w.setOpacity(100) - expect(w.getOpacity()).to.equal(1.0) + assert.strictEqual(w.getOpacity(), 1.0) w.setOpacity(-100) - expect(w.getOpacity()).to.equal(0.0) + assert.strictEqual(w.getOpacity(), 0.0) }) }) @@ -1253,9 +1253,9 @@ describe('BrowserWindow module', () => { it('sets 1 regardless of parameter', () => { const w = new BrowserWindow({ show: false }) w.setOpacity(0) - expect(w.getOpacity()).to.equal(1.0) + assert.strictEqual(w.getOpacity(), 1.0) w.setOpacity(0.5) - expect(w.getOpacity()).to.equal(1.0) + assert.strictEqual(w.getOpacity(), 1.0) }) }) }) From b0c200a9cd85681eec440124b15137079002fc1c Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Tue, 13 Aug 2019 12:36:15 -0700 Subject: [PATCH 3/6] assert 2 --- spec/api-browser-window-spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 4f34e882d67b2..3fa188d9c3342 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1226,14 +1226,14 @@ describe('BrowserWindow module', () => { it('allows setting the opacity', () => { const w = new BrowserWindow({ show: false }) - expect(() => { + assert.doesNotThrow(() => { w.setOpacity(0.0) assert.strictEqual(w.getOpacity(), 0.0) w.setOpacity(0.5) assert.strictEqual(w.getOpacity(), 0.5) w.setOpacity(1.0) assert.strictEqual(w.getOpacity(), 1.0) - }).to.not.throw() + }) }) it('clamps opacity to [0.0...1.0]', () => { From ee0bad0e53e61aaf582f4eac3d216e8034232755 Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Tue, 13 Aug 2019 15:40:16 -0700 Subject: [PATCH 4/6] fix `this` scoping --- spec/api-browser-window-spec.js | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 3fa188d9c3342..7ecfacbae8a56 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1214,10 +1214,12 @@ describe('BrowserWindow module', () => { }) describe('BrowserWindow.setOpacity(opacity)', () => { - describe('Windows and Mac', function () { - if (process.platform === 'linux') { - this.skip() - } + describe('Windows and Mac', () => { + before(function () { + if (process.platform !== 'linux') { + this.skip() + } + }) it('make window with initial opacity', () => { const w = new BrowserWindow({ show: false, opacity: 0.5 }) @@ -1245,10 +1247,12 @@ describe('BrowserWindow module', () => { }) }) - describe('Linux', function () { - if (process.platform !== 'linux') { - this.skip() - } + describe('Linux', () => { + before(function () { + if (process.platform !== 'linux') { + this.skip() + } + }) it('sets 1 regardless of parameter', () => { const w = new BrowserWindow({ show: false }) From c9388ab46cc58c6fce113975ffa902bf7fbcde00 Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Tue, 13 Aug 2019 16:29:28 -0700 Subject: [PATCH 5/6] fix equality --- spec/api-browser-window-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 7ecfacbae8a56..19925bcf4158e 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1216,7 +1216,7 @@ describe('BrowserWindow module', () => { describe('BrowserWindow.setOpacity(opacity)', () => { describe('Windows and Mac', () => { before(function () { - if (process.platform !== 'linux') { + if (process.platform === 'linux') { this.skip() } }) From 724ac9473ffbe058eb620b2e1fa339d43c483590 Mon Sep 17 00:00:00 2001 From: Erick Zhao Date: Wed, 14 Aug 2019 11:05:07 -0700 Subject: [PATCH 6/6] tests --- spec/api-browser-window-spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 19925bcf4158e..2d6ca85e4f4ce 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1221,13 +1221,13 @@ describe('BrowserWindow module', () => { } }) - it('make window with initial opacity', () => { - const w = new BrowserWindow({ show: false, opacity: 0.5 }) + it('makes a window with initial opacity', () => { + w.destroy() + w = new BrowserWindow({ show: false, opacity: 0.5 }) assert.strictEqual(w.getOpacity(), 0.5) }) it('allows setting the opacity', () => { - const w = new BrowserWindow({ show: false }) assert.doesNotThrow(() => { w.setOpacity(0.0) assert.strictEqual(w.getOpacity(), 0.0) @@ -1239,7 +1239,6 @@ describe('BrowserWindow module', () => { }) it('clamps opacity to [0.0...1.0]', () => { - const w = new BrowserWindow({ show: false, opacity: 0.5 }) w.setOpacity(100) assert.strictEqual(w.getOpacity(), 1.0) w.setOpacity(-100) @@ -1255,7 +1254,8 @@ describe('BrowserWindow module', () => { }) it('sets 1 regardless of parameter', () => { - const w = new BrowserWindow({ show: false }) + w.destroy() + w = new BrowserWindow({ show: false, opacity: 0.5 }) w.setOpacity(0) assert.strictEqual(w.getOpacity(), 1.0) w.setOpacity(0.5)