From a78a7bf8aa0d8d321a8d17fe547c0b01f28197bf Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Mon, 13 Dec 2021 10:03:28 +0100 Subject: [PATCH 1/6] Update comment for scrollbar workaround This is no longer an issue on Google Chrome, tested on Chrome 96 on Fedora 34, Windows 10, macOS 12 and Android 12. It is however an issue on Safari on macOS 12. Without this workaround we get scrollbars when making the browser window smaller, despite remote resize being enabled. --- core/rfb.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index 15831033..fe9939f0 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -703,12 +703,13 @@ export default class RFB extends EventTargetMixin { } _fixScrollbars() { - // This is a hack because Chrome screws up the calculation - // for when scrollbars are needed. So to fix it we temporarily - // toggle them off and on. + // This is a hack because Safari on macOS screws up the calculation + // for when scrollbars are needed. We get scrollbars when making the + // browser smaller, despite remote resize being enabled. So to fix it + // we temporarily toggle them off and on. const orig = this._screen.style.overflow; this._screen.style.overflow = 'hidden'; - // Force Chrome to recalculate the layout by asking for + // Force Safari to recalculate the layout by asking for // an element's dimensions this._screen.getBoundingClientRect(); this._screen.style.overflow = orig; From a7b96087d764c86407ec629035ed39154c34d7c7 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Fri, 10 Dec 2021 00:40:59 +0100 Subject: [PATCH 2/6] Add some explanatory comments to test.rfb.js --- tests/test.rfb.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 98d5629d..59b9cdf5 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -74,6 +74,9 @@ describe('Remote Frame Buffer Protocol Client', function () { let fakeResizeObserver = null; const realObserver = window.ResizeObserver; + // Since we are using fake timers we don't actually want + // to wait for the browser to observe the size change, + // that's why we use a fake ResizeObserver class FakeResizeObserver { constructor(handler) { this.fire = handler; @@ -783,14 +786,18 @@ describe('Remote Frame Buffer Protocol Client', function () { 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00 ]; - // First message should trigger a resize - + // This property is indirectly used as a marker for the first update client._supportsSetDesktopSize = false; + // First message should trigger a resize + client._sock._websocket._receiveData(new Uint8Array(incoming)); + // It should match the current size of the container, + // not the reported size from the server expect(RFB.messages.setDesktopSize).to.have.been.calledOnce; - expect(RFB.messages.setDesktopSize).to.have.been.calledWith(sinon.match.object, 70, 80, 0, 0); + expect(RFB.messages.setDesktopSize).to.have.been.calledWith( + sinon.match.object, 70, 80, 0, 0); RFB.messages.setDesktopSize.resetHistory(); From c0d4dc8eb3654d4fd6e7fb6a955a48f56b6d1370 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Fri, 10 Dec 2021 00:58:38 +0100 Subject: [PATCH 3/6] Breakdown of ExtendedDesktopSize message in tests Saves time by not requiring the developer to look up the RFB protocol each time viewing these tests. --- tests/test.rfb.js | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 59b9cdf5..3bde5300 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -780,11 +780,26 @@ describe('Remote Frame Buffer Protocol Client', function () { it('should request a resize when initially connecting', function () { // Simple ExtendedDesktopSize FBU message - const incoming = [ 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x04, 0x00, 0x04, 0xff, 0xff, 0xfe, 0xcc, - 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x04, - 0x00, 0x00, 0x00, 0x00 ]; + const incoming = [ 0x00, // msg-type=FBU + 0x00, // padding + 0x00, 0x01, // number of rects = 1 + 0x00, 0x00, // reason = server initialized + 0x00, 0x00, // status = no error + 0x00, 0x04, // new width = 4 + 0x00, 0x04, // new height = 4 + 0xff, 0xff, + 0xfe, 0xcc, // enc = (-308) ExtendedDesktopSize + 0x01, // number of screens = 1 + 0x00, 0x00, + 0x00, // padding + 0x00, 0x00, + 0x00, 0x00, // screen id = 0 + 0x00, 0x00, // screen x = 0 + 0x00, 0x00, // screen y = 0 + 0x00, 0x04, // screen width = 4 + 0x00, 0x04, // screen height = 4 + 0x00, 0x00, + 0x00, 0x00]; // screen flags // This property is indirectly used as a marker for the first update client._supportsSetDesktopSize = false; From acc30093ada1f69fdf9e98b1615c8139e6a3e42a Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Fri, 10 Dec 2021 17:53:47 +0100 Subject: [PATCH 4/6] Replace resize events with observations in tests This was missed in commit 375f36c57544dd89c042a6beceff93a2430f2358, probably because these unit tests still passed (due to the expectancy was for the code to not act on the resize events). --- tests/test.rfb.js | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 3bde5300..1685f81d 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -553,8 +553,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; - const event = new UIEvent('resize'); - window.dispatchEvent(event); + fakeResizeObserver.fire(); clock.tick(); expect(client._display.viewportChangeSize).to.not.have.been.called; @@ -566,8 +565,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; - const event = new UIEvent('resize'); - window.dispatchEvent(event); + fakeResizeObserver.fire(); clock.tick(); expect(client._display.viewportChangeSize).to.not.have.been.called; @@ -748,8 +746,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; - const event = new UIEvent('resize'); - window.dispatchEvent(event); + fakeResizeObserver.fire(); clock.tick(); expect(client._display.autoscale).to.not.have.been.called; @@ -859,8 +856,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; - const event = new UIEvent('resize'); - window.dispatchEvent(event); + fakeResizeObserver.fire(); clock.tick(1000); expect(RFB.messages.setDesktopSize).to.not.have.been.called; @@ -871,8 +867,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; - const event = new UIEvent('resize'); - window.dispatchEvent(event); + fakeResizeObserver.fire(); clock.tick(1000); expect(RFB.messages.setDesktopSize).to.not.have.been.called; @@ -883,8 +878,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; - const event = new UIEvent('resize'); - window.dispatchEvent(event); + fakeResizeObserver.fire(); clock.tick(1000); expect(RFB.messages.setDesktopSize).to.not.have.been.called; From 6cd69705d6c9a16337d6f653c14168d2e194684f Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Fri, 10 Dec 2021 19:39:49 +0100 Subject: [PATCH 5/6] Make sure we wait for the resizeTimeout in tests Not waiting for the full timeout can obscure future bugs. --- tests/test.rfb.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 1685f81d..1d806e72 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -523,7 +523,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; fakeResizeObserver.fire(); - clock.tick(); + clock.tick(1000); expect(client._display.viewportChangeSize).to.have.been.calledOnce; expect(client._display.viewportChangeSize).to.have.been.calledWith(40, 50); @@ -554,7 +554,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; fakeResizeObserver.fire(); - clock.tick(); + clock.tick(1000); expect(client._display.viewportChangeSize).to.not.have.been.called; }); @@ -566,7 +566,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; fakeResizeObserver.fire(); - clock.tick(); + clock.tick(1000); expect(client._display.viewportChangeSize).to.not.have.been.called; }); @@ -717,7 +717,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; fakeResizeObserver.fire(); - clock.tick(); + clock.tick(1000); expect(client._display.autoscale).to.have.been.calledOnce; expect(client._display.autoscale).to.have.been.calledWith(40, 50); @@ -747,7 +747,7 @@ describe('Remote Frame Buffer Protocol Client', function () { container.style.width = '40px'; container.style.height = '50px'; fakeResizeObserver.fire(); - clock.tick(); + clock.tick(1000); expect(client._display.autoscale).to.not.have.been.called; }); From 0ff0844a142c086128cce8771a3dad21a499f02b Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Fri, 10 Dec 2021 20:11:05 +0100 Subject: [PATCH 6/6] Ignore resize observation caused by server resizes If we increase the remote screen size from the server in such a way that it no longer fits the browser window, the browser will probably want to show scrollbars. The same happens if you enable 'clipping' while the remote is larger than the browser window. These scrollbars do, in turn, decrease the available space in the browser window. This causes our ResizeObserver to trigger. If the resize observation triggers a requestRemoteResize() we will overwrite the size and request a new one just because scrollbars have appeared. We don't want that. We can save the expected client size after resizing, and then compare the current client size with the expected one. If there is no change compared to the expected size, we shouldn't send the request. Fixes issue #1616. --- core/rfb.js | 31 +++++++++++++++++ tests/test.rfb.js | 84 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index fe9939f0..970e03ff 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -244,6 +244,8 @@ export default class RFB extends EventTargetMixin { this._sock.on('message', this._handleMessage.bind(this)); this._sock.on('error', this._socketError.bind(this)); + this._expectedClientWidth = null; + this._expectedClientHeight = null; this._resizeObserver = new ResizeObserver(this._eventHandlers.handleResize); // All prepared, kick off the connection @@ -623,7 +625,26 @@ export default class RFB extends EventTargetMixin { { detail: { name: this._fbName } })); } + _saveExpectedClientSize() { + this._expectedClientWidth = this._screen.clientWidth; + this._expectedClientHeight = this._screen.clientHeight; + } + + _currentClientSize() { + return [this._screen.clientWidth, this._screen.clientHeight]; + } + + _clientHasExpectedSize() { + const [currentWidth, currentHeight] = this._currentClientSize(); + return currentWidth == this._expectedClientWidth && + currentHeight == this._expectedClientHeight; + } + _handleResize() { + // Don't change anything if the client size is already as expected + if (this._clientHasExpectedSize()) { + return; + } // If the window resized then our screen element might have // as well. Update the viewport dimensions. window.requestAnimationFrame(() => { @@ -664,6 +685,12 @@ export default class RFB extends EventTargetMixin { this._display.viewportChangeSize(size.w, size.h); this._fixScrollbars(); } + + // When changing clipping we might show or hide scrollbars. + // This causes the expected client dimensions to change. + if (curClip !== newClip) { + this._saveExpectedClientSize(); + } } _updateScale() { @@ -688,6 +715,7 @@ export default class RFB extends EventTargetMixin { } const size = this._screenSize(); + RFB.messages.setDesktopSize(this._sock, Math.floor(size.w), Math.floor(size.h), this._screenID, this._screenFlags); @@ -2507,6 +2535,9 @@ export default class RFB extends EventTargetMixin { this._updateScale(); this._updateContinuousUpdates(); + + // Keep this size until browser client size changes + this._saveExpectedClientSize(); } _xvpOp(ver, op) { diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 1d806e72..c3aa74e2 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -540,6 +540,10 @@ describe('Remote Frame Buffer Protocol Client', function () { sinon.spy(client._display, "viewportChangeSize"); client._sock._websocket._receiveData(new Uint8Array(incoming)); + // The resize will cause scrollbars on the container, this causes a + // resize observation in the browsers + fakeResizeObserver.fire(); + clock.tick(1000); // FIXME: Display implicitly calls viewportChangeSize() when // resizing the framebuffer, hence calledTwice. @@ -571,6 +575,32 @@ describe('Remote Frame Buffer Protocol Client', function () { expect(client._display.viewportChangeSize).to.not.have.been.called; }); + describe('Clipping and remote resize', function () { + beforeEach(function () { + // Given a remote (100, 100) larger than the container (70x80), + client._resize(100, 100); + client._supportsSetDesktopSize = true; + client.resizeSession = true; + sinon.spy(RFB.messages, "setDesktopSize"); + }); + afterEach(function () { + RFB.messages.setDesktopSize.restore(); + }); + it('should not change remote size when changing clipping', function () { + // When changing clipping the scrollbars of the container + // will appear and disappear and thus trigger resize observations + client.clipViewport = false; + fakeResizeObserver.fire(); + clock.tick(1000); + client.clipViewport = true; + fakeResizeObserver.fire(); + clock.tick(1000); + + // Then no resize requests should be sent + expect(RFB.messages.setDesktopSize).to.not.have.been.called; + }); + }); + describe('Dragging', function () { beforeEach(function () { client.dragViewport = true; @@ -734,6 +764,10 @@ describe('Remote Frame Buffer Protocol Client', function () { sinon.spy(client._display, "autoscale"); client._sock._websocket._receiveData(new Uint8Array(incoming)); + // The resize will cause scrollbars on the container, this causes a + // resize observation in the browsers + fakeResizeObserver.fire(); + clock.tick(1000); expect(client._display.autoscale).to.have.been.calledOnce; expect(client._display.autoscale).to.have.been.calledWith(70, 80); @@ -830,6 +864,35 @@ describe('Remote Frame Buffer Protocol Client', function () { expect(RFB.messages.setDesktopSize).to.have.been.calledWith(sinon.match.object, 40, 50, 0, 0); }); + it('should not request the same size twice', function () { + container.style.width = '40px'; + container.style.height = '50px'; + fakeResizeObserver.fire(); + clock.tick(1000); + + expect(RFB.messages.setDesktopSize).to.have.been.calledOnce; + expect(RFB.messages.setDesktopSize).to.have.been.calledWith( + sinon.match.object, 40, 50, 0, 0); + + // Server responds with the requested size 40x50 + const incoming = [ 0x00, 0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, + 0x00, 0x28, 0x00, 0x32, 0xff, 0xff, 0xfe, 0xcc, + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x28, 0x00, 0x32, + 0x00, 0x00, 0x00, 0x00]; + + client._sock._websocket._receiveData(new Uint8Array(incoming)); + clock.tick(1000); + + RFB.messages.setDesktopSize.resetHistory(); + + // size is still 40x50 + fakeResizeObserver.fire(); + clock.tick(1000); + + expect(RFB.messages.setDesktopSize).to.not.have.been.called; + }); + it('should not resize until the container size is stable', function () { container.style.width = '20px'; container.style.height = '30px'; @@ -885,16 +948,33 @@ describe('Remote Frame Buffer Protocol Client', function () { }); it('should not try to override a server resize', function () { - // Simple ExtendedDesktopSize FBU message + // Simple ExtendedDesktopSize FBU message, new size: 100x100 const incoming = [ 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x04, 0x00, 0x04, 0xff, 0xff, 0xfe, 0xcc, + 0x00, 0x64, 0x00, 0x64, 0xff, 0xff, 0xfe, 0xcc, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00 ]; + // Note that this will cause the browser to display scrollbars + // since the framebuffer is 100x100 and the container is 70x80. + // The usable space (clientWidth/clientHeight) will be even smaller + // due to the scrollbars taking up space. client._sock._websocket._receiveData(new Uint8Array(incoming)); + // The scrollbars cause the ResizeObserver to fire + fakeResizeObserver.fire(); + clock.tick(1000); expect(RFB.messages.setDesktopSize).to.not.have.been.called; + + // An actual size change must not be ignored afterwards + container.style.width = '120px'; + container.style.height = '130px'; + fakeResizeObserver.fire(); + clock.tick(1000); + + expect(RFB.messages.setDesktopSize).to.have.been.calledOnce; + expect(RFB.messages.setDesktopSize.firstCall.args[1]).to.equal(120); + expect(RFB.messages.setDesktopSize.firstCall.args[2]).to.equal(130); }); });