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.

Sign up for GitHub

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

GUACAMOLE-1292: prompt when closing the window/tab to avoid accidents with ctrl-w #592

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

GUACAMOLE-1292: prompt when closing the window/tab to avoid accidents with ctrl-w #592

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2826861
prompt when closing the window/tab to avoid accidents with ctrl-w
phreakocious Feb 18, 2021
91b195e
GUACAMOLE-1292: rearrange to a check in the event handler, fix indent…
phreakocious Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion guacamole/src/main/webapp/app/client/controllers/clientController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams

// Required services
var $location = $injector.get('$location');
var $window = $injector.get('$window');
var authenticationService = $injector.get('authenticationService');
var connectionGroupService = $injector.get('connectionGroupService');
var clipboardService = $injector.get('clipboardService');
Expand Down Expand Up @@ -219,6 +220,26 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams
remaining: 15
};

/**
* Catch window or tab closing (ctrl-w) and prompt the user if there is an active connection
*
* @param {Event} e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add documentation for parameters accepted by functions.

* @returns {undefined}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this function returns nothing, there's no need to include an @returns.

*/
var windowCloseListener = function onBeforeUnload(e) {
var managedClient = $scope.client;
if (managedClient) {

// Get current connection state
var connectionState = managedClient.clientState.connectionState;

// If connected, prompt to close
if (connectionState === ManagedClientState.ConnectionState.CONNECTED)
e.preventDefault();
e.returnValue = '';
necouchman marked this conversation as resolved.
Show resolved Hide resolved
}
};

/**
* Menu-specific properties.
*/
Expand Down Expand Up @@ -776,6 +797,8 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams

// Client error
else if (connectionState === ManagedClientState.ConnectionState.CLIENT_ERROR) {
// Stop intercepting window close
$window.removeEventListener('beforeunload', ctrlwlistener, false);
phreakocious marked this conversation as resolved.
Show resolved Hide resolved

// Determine translation name of error
var errorName = (status in CLIENT_ERRORS) ? status.toString(16).toUpperCase() : "DEFAULT";
Expand All @@ -798,7 +821,6 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams

// Tunnel error
else if (connectionState === ManagedClientState.ConnectionState.TUNNEL_ERROR) {

// Determine translation name of error
var errorName = (status in TUNNEL_ERRORS) ? status.toString(16).toUpperCase() : "DEFAULT";

Expand Down Expand Up @@ -831,6 +853,8 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams

// Hide status and sync local clipboard once connected
else if (connectionState === ManagedClientState.ConnectionState.CONNECTED) {
// Start intercepting ctrl-w / window close
$window.addEventListener('beforeunload', windowCloseListener, false);
Comment on lines +856 to +857
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be problematic. Since the event listener is being added conditionally depending on whether the current client has connected (and removing it upon error), there will be issues if:

  • The user navigates away from the client view (the client will remain active and connecting in the background, ready for the user to resume)
  • The user has multiple connections open within the same tab (they will fight over handling of beforeunload). Depending on whether the event handler is recognized as the same function by the event stack, this would result either in the connections removing each other's handlers or the connections adding multiple copies of the event handler.

I think this instead needs to be at the root level, outside the handling of a connection. If there were a single beforeunload event handler that is never removed, that handler could:

  1. Check whether any clients are present in the overall set of managed clients (this is exposed by the guacClientManager service).
  2. If there are clients, prompt. If there aren't, don't.

In fact, guacClientManager already handles unload:

guacamole-client/guacamole/src/main/webapp/app/client/services/guacClientManager.js

Lines 164 to 165 in 0091bb1

// Disconnect all clients when window is unloaded
$window.addEventListener('unload', service.clear);

Perhaps it would make sense for it to handle beforeunload as well, and for that to be the sole instance of this event handler?


// Sync with local clipboard
clipboardService.getLocalClipboard().then(function clipboardRead(data) {
Expand Down