GUACAMOLE-124: Add full-screen action

[madmath03]

GUACAMOLE-124: Add full-screen action

Signed-off-by: mathieu.brunot [email protected]

[necouchman]

Overall this looks pretty solid to me. My only question is how much testing you've done with scaling/resizing with this? Presumably when it goes to full-screen the screen resolution changes, which, depending on the protocol in use for the connection, will result in either an on-the-fly resize (RDP, maybe SSH/Telnet), a disconnect/reconnect (RDP), or some extra black space around the screen area. What sort of behavior have you seen?

[madmath03]

Overall this looks pretty solid to me. My only question is how much testing you've done with scaling/resizing with this? Presumably when it goes to full-screen the screen resolution changes, which, depending on the protocol in use for the connection, will result in either an on-the-fly resize (RDP, maybe SSH/Telnet), a disconnect/reconnect (RDP), or some extra black space around the screen area. What sort of behavior have you seen?

I did some more tests on different devices / browsers and connections. Here are the results:

Device / Browser	🏁 Windows RDP	🐧 Linux SSH	🐧 Linux VNC (TigerVNC)
🏁 Windows / Chromium	⚠️	✅	✅
🏁 Windows / Firefox	⚠️	✅	✅
🏁 Windows / Edge	⚠️	✅	✅
🏁 Windows / IE 11	🚫	🚫	🚫
🤖 Android Tablet / Chromium	⚠️	✅	✅
🤖 Android Tablet / Firefox	⚠️	✅	✅
🍏 iPhone / Safari	❌	❌	❌

• ✅ : Fullscreen works properly and connection is immediately resized
• ⚠️ : Fullscreen button works but Windows RDP connection displays empty space and needs to be restarted to be resized (disconnect / reconnect, no need to go back to home)
• 🚫 : Fullscreen button works but only displays a black screen during connection, whatever the connection type
• ❌ : Fullscreen button does not trigger fullscreen

So basically:

• IE 11 and Safari do not work
• RDP connection needs reconnect (seems to be the normal behavior of RDP)
• everything else is good

@necouchman & @mike-jumper : I do not have a Mac to debug behavior on Safari, and I do not believe IE 11 is relevant anymore, so I think that's good enough to be used, or at least to be reviewed.

[necouchman]

@madmath03: Please note that all your commits need to have the "GAUCAMOLE-124" tag on them. Also, might want to consider squashing the commits into a smaller number - five is a lot of commits for the small amount of code that is being changed.

[madmath03]

@necouchman done

[necouchman]

I'm happy with it, but I'll let @mike-jumper give the final word.

[madmath03]

Hi @mike-jumper ,

Do you think this could be part of the 1.2.0 release ?

[madmath03]

hello @mike-jumper

Let me know if the corrections applied are valid for you.

[madmath03]

hello @mike-jumper

I've applied the modifications you requested (not a 100% sure about the last one though).
Let me know if there is anything else.

[madmath03]

@mike-jumper I've tested this on the same devices I mentioned initially and it all seems to work as expected.

Let me know if you need more changes.

[echu2013]

@madmath03 : I've tested it on Version 80.0.3987.149 (Official Build) (64-bit) under Windows 10 Pro 64-bit and it's working OK!
IMO there should be some sort of toggle switch or visual change when full-screen toggled on, like changing "Full Screen" value to "Turn off Full Screen".

[madmath03]

@madmath03 : I've tested it on Version 80.0.3987.149 (Official Build) (64-bit) under Windows 10 Pro 64-bit and it's working OK!

Glad to hear that 😃

IMO there should be some sort of toggle switch or visual change when full-screen toggled on, like changing "Full Screen" value to "Turn off Full Screen".

That might be nice indeed, but since the action name and CSS classes are constants, I'm not sure if there is a way to do that easily though...
@necouchman & @mike-jumper is that something possible and if so how ?

[madmath03]

@mike-jumper anything blocking this to be merged ?

[madmath03]

@mike-jumper & @necouchman anything blocking this to be merged ?

[echu2013]

@madmath03 I can say that for me just worked fine! Just a minor addition I've made to your code, don't show toggle is running Internet explorer as it renders a black screen (known bug, but it's IE..)

--- a/guacamole/src/main/webapp/app/navigation/directives/guacUserMenu.js
+++ b/guacamole/src/main/webapp/app/navigation/directives/guacUserMenu.js
@@ -191,6 +191,17 @@ angular.module('navigation').directive('guacUserMenu', [function guacUserMenu()
              */
             $scope.actions = [ LOGOUT_ACTION ];
 
+
+            // Check if Internet Explorer UserAgent
+            var isIE = false;
+            var ua = window.navigator.userAgent;
+            var old_ie = ua.indexOf('MSIE ');
+            var new_ie = ua.indexOf('Trident/');
+
+            if ((old_ie > -1) || (new_ie > -1)) {
+                isIE = true;
+}
+
             // Initialize fullscreen functions
             var docElem = document.documentElement;
             var requestFullscreen = docElem.requestFullscreen
@@ -201,8 +212,8 @@ angular.module('navigation').directive('guacUserMenu', [function guacUserMenu()
                 || document.mozCancelFullScreen
                 || document.webkitExitFullscreen
                 || document.msExitFullscreen;
-
-            if (!!requestFullscreen && !!exitFullscreen) {
+            // If running Internet Explorer DONT include FullScreen toggle (Black Screen is produced when toggled)
+            if (!!requestFullscreen && !!exitFullscreen && !isIE ) {
                 // Bind browser-specific fullscreen functions
                 if (!docElem.requestFullscreen) {
                     docElem.requestFullscreen = (requestFullscreen).bind(docElem);

[madmath03]

Just a minor addition I've made to your code, don't show toggle is running Internet explorer as it renders a black screen (known bug, but it's IE..)

Hi @echu2013

I added your suggestion to this PR.

@necouchman & @mike-jumper let me know what you guys think about this.

[echu2013]

@madmath03 great job! Thanks let's hope they approve it

[madmath03]

@mike-jumper can you review this PR ?

[mike-jumper]

I've checked out the code locally to test the full screen functionality, and I think there are some rough edges that are worth resolving before merge:

1. The placement of the "Fullscreen" menu item is a bit awkward. Normally, actions local to the current page are displayed at the top of the user menu, such as the client-specific "Disconnect" option. Currently, the "Fullscreen" item is between "Settings" and "Logout", which does not make logical sense to me:

   

   There is already functionality built into the controller of the client page to add client-specific user menu options. That may be a good thing to leverage here:

   guacamole-client/guacamole/src/main/webapp/app/client/controllers/clientController.js

   Lines 899 to 900 in 0091bb1

   	// Set client-specific menu actions
   	$scope.clientMenuActions = [ DISCONNECT_MENU_ACTION ];

   Alternatively, the Guacamole menu has a dedicated section for controlling the display. That might be a good place for a button that enters/exits fullscreen.

2. After clicking "Fullscreen", the Guacamole menu remains open, requiring the user to manually close the menu by pressing Ctrl+Alt+Shift. The current standard behavior of the client interface is that taking action within the menu should automatically close the menu, and I think that should be done here. The behavior of the "Disconnect" menu item may be a good example here, as well.

3. When Chrome enters fullscreen mode, it displays the key that the user may press to exit fullscreen. In my case, this is Esc:

   

   This is unfortunately true. Despite Guacamole otherwise having full control of the Esc key, that control is lost while in full screen. This is a bit annoying when using an application which leverages Esc, though I'm also not sure anything can be done about this.

[necouchman]

Despite Guacamole otherwise having full control of the Esc key, that control is lost while in full screen. This is a bit annoying when using an application which leverages Esc, though I'm also not sure anything can be done about this.

We could try to roll https://issues.apache.org/jira/browse/GUACAMOLE-989 into this??

[madmath03]

@mike-jumper

Regarding point 3, as you've mentioned, I'm not sure there is anything that can be done about this here.

For the point 1 though, I do not agree with you. It's actually not related to the current page since it switches the whole browser to fullscreen mode.
Also, if we did make it the way you say, we could not enter fullscreen before opening a connection. From my experience, some connections, like RDP, work better if you're already in fullscreen when opening it. Switching on/off fullscreen during connection will sometimes "mess up" the resolution and resize.
So I would think it's better to keep as it is since it's not "client-specific".

what do you think @necouchman ?

I can take a look at point 2 though and see how it's done.

[madmath03]

@necouchman & @mike-jumper just pinging to get some news on this old PR

[depawlur]

I hope this is not dead. I did test this before 1.2.0 release. It's 1.3.0 already.
This also sounds amazing https://issues.apache.org/jira/browse/GUACAMOLE-989

[necouchman]

@depawlur: I don't think it's dead, just didn't get finished up in time to make the 1.3.0 release. Hopefully we can add it to the next one...

[necouchman]

@madmath03:

For the point 1 though, I do not agree with you. It's actually not related to the current page since it switches the whole browser to fullscreen mode.

I agree with @mike-jumper on the placement of this item - while it is true it puts the entire browser into fullscreen mode, the context in which the user executes the action is a specific connection, and so I think it makes more sense to have it grouped with connection-specific options.

Also, if we did make it the way you say, we could not enter fullscreen before opening a connection.

Ah, I think I see what you're getting at - you've added it to the Guacamole menu where it displayed either here in the hidden menu or in the menu on the home screen, I'm guessing? So that someone can go to the menu on the home screen and put the entire screen in fullscreen mode, and then start a connection? I guess I'm not opposed to having this functionality available on the Home screen, assuming we can agree on the correct place for it. However, a couple of points, here:

• It is still possible to put the browser into fullscreen mode without this option in the menu on the Home screen - most browsers have both a menu item and a shortcut key (F11 or F12, I think) that trigger fullscreen mode. A menu item on the home screen is not required to enable this - indeed, you can do it today with no modifications to the Guacamole interface.
• Most modern versions of the protocols supported by Guacamole deal with the screen resizing properly when it happens after connection time. RDP has had the functionality for several years to do a dynamic resize without terminating the connection. I cannot remember what version of Windows added this support - maybe 8? - but it does work. Other RDP servers may not support it (doubt xrdp does), but at least modern Windows versions do. SSH and Kubernetes should also be fine - VNC currently does not support any resolution negotiation between client and server, let alone dynamic, and I'm kind of doubt most Telnet servers deal with resolution changes all that seamlessly.

[madmath03]

@necouchman thanks for the feedback. I was really expecting this before doing any further modifications to this PR 😃

you've added it to the Guacamole menu where it displayed either here in the hidden menu or in the menu on the home screen, I'm guessing? So that someone can go to the menu on the home screen and put the entire screen in fullscreen mode, and then start a connection?

Yes, that's exactly what I had in mind and the reason why I'm not too much of a fan of making it connection-specific.

I guess I'm not opposed to having this functionality available on the Home screen, assuming we can agree on the correct place for it.

In that case, how and where should it be placed ?

• It is still possible to put the browser into fullscreen mode without this option in the menu on the Home screen - most browsers have both a menu item and a shortcut key (F11 or F12, I think) that trigger fullscreen mode.

That's only true for desktop browsers. I developped this PR when I was testing Guacamole on an Android tablet: it worked fine, but there is no way on tablets or smartphones to switch to Fullscreen making it not really practical...
Since the fullscreen API still works on mobile, it's just that there is no menu or shortcut for it, this PR would be really nice.

BTW: another solution but much more complicated would be to make Guacamole into a PWA (basically requires HTTPS, manifest.json and a Service Worker) so that we could install Guacamole and run it in fullscreen by default.

[necouchman]

@mike-jumper Any further thoughts or guidance on placement of this item?

@madmath03 Fair point about the non-desktop browsers, this makes sense. Let's see if we can reach consensus on where such an option would be placed.

[necouchman]

@mike-jumper This one has languished a bit - any further feedback, here?

@madmath03 You still around and willing to continue to work on this if we can get you some guidance?

[mike-jumper]

@necouchman I think this may be effectively superseded by #695, which was merged for GUACAMOLE-1525, a duplicate of GUACAMOLE-124.

[necouchman]

Thanks @mike-jumper - didn't even think to check that. I'll verify and close this and the Jira issue out.

[necouchman]

Closed by #695.