From a7e642e473cd4132564a55ceb19c0cefb074bb82 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Fri, 12 Jan 2018 16:06:35 +0100 Subject: [PATCH 1/3] Destroy underlying element when resetting or destroying Workspace --- src/workspace-element.js | 4 ++++ src/workspace.js | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/workspace-element.js b/src/workspace-element.js index bd0e1b971..188795f9b 100644 --- a/src/workspace-element.js +++ b/src/workspace-element.js @@ -132,6 +132,10 @@ class WorkspaceElement extends HTMLElement { return this } + destroy () { + this.subscriptions.dispose() + } + getModel () { return this.model } handleDragStart (event) { diff --git a/src/workspace.js b/src/workspace.js index 127168748..66e7f8ba5 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -310,7 +310,10 @@ module.exports = class Workspace extends Model { this.originalFontSize = null this.openers = [] this.destroyedItemURIs = [] - this.element = null + if (this.element) { + this.element.destroy() + this.element = null + } this.consumeServices(this.packageManager) } @@ -1570,6 +1573,7 @@ module.exports = class Workspace extends Model { if (this.activeItemSubscriptions != null) { this.activeItemSubscriptions.dispose() } + if (this.element) this.element.destroy() } /* From 8077f46fdf4e43e4d30027bf6a6ccdaa25950d8d Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Fri, 12 Jan 2018 17:10:40 +0100 Subject: [PATCH 2/3] Programmatically detect when mouse approaches the edge of a dock Previously, we would assign dock elements a minimum width/height of 2 pixels so that we could detect when the mouse approached the edge of a hidden dock in order to show the chevron buttons. This, however, was causing confusion for users, who expected that extra space to be clickable in order to scroll editors located in the center dock. With this commit we will instead register a global `mousemove` event on the window right when attaching the workspace element to the DOM (the event handler is debounced, so this shouldn't have any performance consequence). Then, when mouse moves, we will programmatically detect when it is approaching to the edge of a dock and show the chevron button accordingly. This allows us to remove the `min-width` property from the dock container element, which eliminates the confusing behavior described above. --- spec/workspace-element-spec.js | 8 +++----- src/dock.js | 3 +++ src/workspace-element.js | 3 +-- static/docks.less | 6 ------ 4 files changed, 7 insertions(+), 13 deletions(-) diff --git a/spec/workspace-element-spec.js b/spec/workspace-element-spec.js index 552e95b6d..90d973773 100644 --- a/spec/workspace-element-spec.js +++ b/spec/workspace-element-spec.js @@ -561,8 +561,6 @@ describe('WorkspaceElement', () => { expectToggleButtonHidden(rightDock) expectToggleButtonHidden(bottomDock) - workspaceElement.paneContainer.dispatchEvent(new MouseEvent('mouseleave')) - // --- Right Dock --- // Mouse over where the toggle button would be if the dock were hovered @@ -591,7 +589,7 @@ describe('WorkspaceElement', () => { // Mouse to edge of the window moveMouse({clientX: 575, clientY: 150}) expectToggleButtonHidden(rightDock) - moveMouse({clientX: 600, clientY: 150}) + moveMouse({clientX: 598, clientY: 150}) expectToggleButtonVisible(rightDock, 'icon-chevron-left') // Click the toggle button again @@ -627,7 +625,7 @@ describe('WorkspaceElement', () => { // Mouse to edge of the window moveMouse({clientX: 25, clientY: 150}) expectToggleButtonHidden(leftDock) - moveMouse({clientX: 0, clientY: 150}) + moveMouse({clientX: 2, clientY: 150}) expectToggleButtonVisible(leftDock, 'icon-chevron-right') // Click the toggle button again @@ -663,7 +661,7 @@ describe('WorkspaceElement', () => { // Mouse to edge of the window moveMouse({clientX: 300, clientY: 290}) expectToggleButtonHidden(leftDock) - moveMouse({clientX: 300, clientY: 300}) + moveMouse({clientX: 300, clientY: 299}) expectToggleButtonVisible(bottomDock, 'icon-chevron-up') // Click the toggle button again diff --git a/src/dock.js b/src/dock.js index 7f2856800..1ee27d5c7 100644 --- a/src/dock.js +++ b/src/dock.js @@ -327,12 +327,15 @@ module.exports = class Dock { // Include all panels that are closer to the edge than the dock in our calculations. switch (this.location) { case 'right': + if (!this.isVisible()) bounds.left = bounds.right - 2 bounds.right = Number.POSITIVE_INFINITY break case 'bottom': + if (!this.isVisible()) bounds.top = bounds.bottom - 1 bounds.bottom = Number.POSITIVE_INFINITY break case 'left': + if (!this.isVisible()) bounds.right = bounds.left + 2 bounds.left = Number.NEGATIVE_INFINITY break } diff --git a/src/workspace-element.js b/src/workspace-element.js index 188795f9b..c9a30af85 100644 --- a/src/workspace-element.js +++ b/src/workspace-element.js @@ -104,6 +104,7 @@ class WorkspaceElement extends HTMLElement { this.addEventListener('mousewheel', this.handleMousewheel.bind(this), true) window.addEventListener('dragstart', this.handleDragStart) + window.addEventListener('mousemove', this.handleEdgesMouseMove) this.panelContainers = { top: this.model.panelContainers.top.getElement(), @@ -173,7 +174,6 @@ class WorkspaceElement extends HTMLElement { // being hovered. this.cursorInCenter = false this.updateHoveredDock({x: event.pageX, y: event.pageY}) - window.addEventListener('mousemove', this.handleEdgesMouseMove) window.addEventListener('dragend', this.handleDockDragEnd) } @@ -203,7 +203,6 @@ class WorkspaceElement extends HTMLElement { checkCleanupDockHoverEvents () { if (this.cursorInCenter && !this.hoveredDock) { - window.removeEventListener('mousemove', this.handleEdgesMouseMove) window.removeEventListener('dragend', this.handleDockDragEnd) } } diff --git a/static/docks.less b/static/docks.less index 301d7aee5..283402e09 100644 --- a/static/docks.less +++ b/static/docks.less @@ -16,12 +16,6 @@ atom-dock { .atom-dock-inner { display: flex; - // Keep the area at least 2 pixels wide so that you have something to hover - // over to trigger the toggle button affordance even when fullscreen. - // Needs to be 2 pixels to work on Windows when scaled to 150%. See atom/atom #15728 - &.left, &.right { min-width: 2px; } - &.bottom { min-height: 1px; } - &.bottom { width: 100%; } &.left, &.right { height: 100%; } From 176552fb7bf5d4982507c5b3814c7413bf160ded Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 15 Jan 2018 09:48:42 +0100 Subject: [PATCH 3/3] Change assertion to reflect the new programmatic dock revealing --- spec/dock-spec.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/spec/dock-spec.js b/spec/dock-spec.js index d4db460ae..6cdbc21f0 100644 --- a/spec/dock-spec.js +++ b/spec/dock-spec.js @@ -201,11 +201,7 @@ describe('Dock', () => { const dockElement = atom.workspace.getBottomDock().getElement() dockElement.querySelector('.atom-dock-resize-handle').dispatchEvent(new MouseEvent('mousedown', {detail: 2})) expect(dockElement.offsetHeight).toBe(0) - - // There should still be a hoverable, absolutely-positioned element so users can reveal the - // toggle affordance even when fullscreened. - expect(dockElement.querySelector('.atom-dock-inner').offsetHeight).toBe(1) - + expect(dockElement.querySelector('.atom-dock-inner').offsetHeight).toBe(0) // The content should be masked away. expect(dockElement.querySelector('.atom-dock-mask').offsetHeight).toBe(0) })