Skip to content

Show local cursor while dragging the viewport #1992

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

Open
redneck-f25 wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Show local cursor while dragging the viewport #1992

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6eb28b8
Make local cursor accessible/configurable
redneck-f25 Sep 15, 2025
355dd27
Make RFB::dragViewport a private Variable with getter and setter
redneck-f25 Sep 15, 2025
0f48f31
Show local (grab/grabbing) cursor while dragging the viewport
redneck-f25 Sep 15, 2025
5782915
Do not compare to 'mousedown' on every 'mousemove'
redneck-f25 Sep 15, 2025
97c0ad4
Add view_drag parameter to enable dragging when connected
redneck-f25 Sep 15, 2025
abc7d63
Fix id of hidden checkbox noVNC_setting_view_drag
redneck-f25 Sep 17, 2025
254fff9
Revert "Show local (grab/grabbing) cursor while dragging the viewport"
redneck-f25 Sep 17, 2025
59e40b4
Revert "Make local cursor accessible/configurable"
redneck-f25 Sep 17, 2025
b1745d5
Drop a note about isTouchDevice and useFallback
redneck-f25 Sep 17, 2025
5a9a81c
Add __FORCE_NO_TOUCH_DEVICE__ to not use fallback
redneck-f25 Sep 17, 2025
4dd16ad
Always initalize canvas.style in case of showing local cursors
redneck-f25 Sep 17, 2025
b5b7ebe
Extend Cursor-API to show local cursors
redneck-f25 Sep 17, 2025
3b79d45
Add members for local cursors
redneck-f25 Sep 17, 2025
68a2db4
Refresh, attach, detach cursor on API-calls and mouse events
redneck-f25 Sep 17, 2025
4fc3fc9
Implement local cursors
redneck-f25 Sep 17, 2025
702302f
Add default local cursor set to UI
redneck-f25 Sep 17, 2025
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
2 changes: 2 additions & 0 deletions app/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ const UI = {
UI.initSetting('password');
UI.initSetting('autoconnect', false);
UI.initSetting('view_clip', false);
UI.initSetting('view_drag', false);
Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Sep 16, 2025

Choose a reason for hiding this comment

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

We already have the toolbar button for this. What's the purpose of this change?

Copy link
Copy Markdown
Author

@redneck-f25 redneck-f25 Sep 17, 2025

Choose a reason for hiding this comment

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

There is a button, but no URL parameter to save this setting as bookmark or enable it automatically when embedding.

https://example.org/vnc.html?autoconnect=1&view_clip=1&view_drag=1

Nevertheless, it wasn't working as intended. Fixed with abc7d63

UI.initSetting('resize', 'off');
UI.initSetting('quality', 6);
UI.initSetting('compression', 2);
Expand Down Expand Up @@ -1096,6 +1097,7 @@ const UI = {
UI.rfb.addEventListener("bell", UI.bell);
UI.rfb.addEventListener("desktopname", UI.updateDesktopName);
UI.rfb.clipViewport = UI.getSetting('view_clip');
UI.rfb.dragViewport = UI.getSetting('view_drag');
UI.rfb.scaleViewport = UI.getSetting('resize') === 'scale';
UI.rfb.resizeSession = UI.getSetting('resize') === 'remote';
UI.rfb.qualityLevel = parseInt(UI.getSetting('quality'));
Expand Down
32 changes: 23 additions & 9 deletions core/rfb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,12 +290,12 @@ export default class RFB extends EventTargetMixin {

// ===== PROPERTIES =====

this.dragViewport = false;
this.focusOnClick = true;

this._viewOnly = false;
this._clipViewport = false;
this._clippingViewport = false;
this._dragViewport = false;
this._scaleViewport = false;
this._resizeSession = false;

Expand Down Expand Up @@ -342,6 +342,12 @@ export default class RFB extends EventTargetMixin {
this._updateClip();
}

get dragViewport() { return this._dragViewport; }
set dragViewport(dragViewport) {
this._dragViewport = dragViewport;
this._refreshCursor();
}

get scaleViewport() { return this._scaleViewport; }
set scaleViewport(scale) {
this._scaleViewport = scale;
Expand Down Expand Up @@ -1111,11 +1117,14 @@ export default class RFB extends EventTargetMixin {

let bmask = RFB._convertButtonMask(ev.buttons);

let down = ev.type == 'mousedown';
let down = false;
Comment on lines 1114 to +1155
Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Sep 16, 2025

Choose a reason for hiding this comment

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

What does this improve?

Copy link
Copy Markdown
Author

@redneck-f25 redneck-f25 Sep 17, 2025

Choose a reason for hiding this comment

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

This is a minor non-functional optimization.

First, the event type is compared against the word mousedown and the result is stored in the local variable down.
Second, in case of the text value of the event type we switch to the responsible code block for'mousedown, mouseup and mousemove.

As the commit message says: Do not compare to 'mousedown' on every 'mousemove'.

I guess mousemove is the most frequently invoked block, so it should be the first case block.

--- 0f48f31e08a70045e4fe5deebcac7d7b04fd816d/core/rfb.js
+++ 57829159839e7a2358f5c22a53414565f5704087/core/rfb.js
@@ -1117,11 +1117,9 @@ export default class RFB extends EventTargetMixin {

         let bmask = RFB._convertButtonMask(ev.buttons);

+        let down = false;
-        let down = ev.type == 'mousedown';
         switch (ev.type) {
             case 'mousedown':
+                down = true;
+            // eslint-disable-next-line no-fallthrough
             case 'mouseup':
                 if (this._dragViewport) {
                     this._cursor.setLocalCursor(down ? 'grabbing' : 'grab');
@@ -1157 +1159 @@
                 break;
             case 'mousemove':

switch (ev.type) {
case 'mousedown':
down = true;
// eslint-disable-next-line no-fallthrough
case 'mouseup':
if (this.dragViewport) {
if (this._dragViewport) {
this._cursor.setLocalCursor(down ? 'grabbing' : 'grab');
if (down && !this._viewportDragging) {
this._viewportDragging = true;
this._viewportDragPos = {'x': pos.x, 'y': pos.y};
Expand Down Expand Up @@ -1334,7 +1343,7 @@ export default class RFB extends EventTargetMixin {
this._handleTapEvent(ev, 0x2);
break;
case 'drag':
if (this.dragViewport) {
if (this._dragViewport) {
this._viewportHasMoved = false;
this._viewportDragging = true;
this._viewportDragPos = {'x': pos.x, 'y': pos.y};
Expand All @@ -1344,7 +1353,7 @@ export default class RFB extends EventTargetMixin {
}
break;
case 'longpress':
if (this.dragViewport) {
if (this._dragViewport) {
// If dragViewport is true, we need to wait to see
// if we have dragged outside the threshold before
// sending any events to the server.
Expand Down Expand Up @@ -1376,7 +1385,7 @@ export default class RFB extends EventTargetMixin {
break;
case 'drag':
case 'longpress':
if (this.dragViewport) {
if (this._dragViewport) {
this._viewportDragging = true;
const deltaX = this._viewportDragPos.x - pos.x;
const deltaY = this._viewportDragPos.y - pos.y;
Expand Down Expand Up @@ -1451,7 +1460,7 @@ export default class RFB extends EventTargetMixin {
case 'twodrag':
break;
case 'drag':
if (this.dragViewport) {
if (this._dragViewport) {
this._viewportDragging = false;
} else {
this._fakeMouseMove(ev, pos.x, pos.y);
Expand All @@ -1465,7 +1474,7 @@ export default class RFB extends EventTargetMixin {
break;
}

if (this.dragViewport && !this._viewportHasMoved) {
if (this._dragViewport && !this._viewportHasMoved) {
this._fakeMouseMove(ev, pos.x, pos.y);
// If dragViewport is true, we need to wait to see
// if we have dragged outside the threshold before
Expand Down Expand Up @@ -3032,7 +3041,7 @@ export default class RFB extends EventTargetMixin {

_shouldShowDotCursor() {
// Called when this._cursorImage is updated
if (!this._showDotCursor) {
if (!this._showDotCursor || this._dragViewport) {
// User does not want to see the dot, so...
return false;
}
Expand Down Expand Up @@ -3062,6 +3071,11 @@ export default class RFB extends EventTargetMixin {
image.hotx, image.hoty,
image.w, image.h
);
if (this._dragViewport) {
this._cursor.setLocalCursor(this._viewportDragging ? 'grabbing' : 'grab');
} else {
this._cursor.setLocalCursor('none');
}
Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Sep 16, 2025

Choose a reason for hiding this comment

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

I was thinking that we only show the drag cursor whilst actively dragging. Otherwise, we want to keep showing the actual cursor to give proper feedback.

Copy link
Copy Markdown
Author

@redneck-f25 redneck-f25 Sep 17, 2025

Choose a reason for hiding this comment

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

It doesn't make that many sense to only show a local cursor whilst dragging the viewport. There might be other reasons for a local cursor in different situations. See below.

Reverted with 254fff9

}

static genDES(password, challenge) {
Expand Down
6 changes: 5 additions & 1 deletion core/util/cursor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export default class Cursor {
}

clear() {
this._target.style.cursor = 'none';
this.setLocalCursor('none');
this._canvas.width = 0;
this._canvas.height = 0;
this._position.x = this._position.x + this._hotSpot.x;
Expand All @@ -115,6 +115,10 @@ export default class Cursor {
this._hotSpot.y = 0;
}

setLocalCursor(cursor) {
Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Sep 16, 2025

Choose a reason for hiding this comment

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

I don't think this fits well with how this class normally operates. I think you'll need to extend the change() API. And probably provide a fallback image for whe we cannot rely on a browser rendered cursor.

Copy link
Copy Markdown
Author

@redneck-f25 redneck-f25 Sep 17, 2025

Choose a reason for hiding this comment

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

Thanks for the hint.

Reverted with 59e40b4
Proposal b5b7ebe

this._target.style.cursor = cursor;
}

// Mouse events might be emulated, this allows
// moving the cursor in such cases
move(clientX, clientY) {
Expand Down
3 changes: 3 additions & 0 deletions docs/EMBEDDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ Currently, the following options are available:
* `view_clip` - If the remote session should be clipped or use scrollbars if
it cannot fit in the browser.

* `view_drag` - If the remote session is clipped enable dragging the viewport
when connected.

* `resize` - How to resize the remote session if it is not the same size as
the browser window. Can be one of `off`, `scale` and `remote`.

Expand Down
4 changes: 4 additions & 0 deletions vnc.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,10 @@ <h1 class="noVNC_logo" translate="no"><span>no</span><br>VNC</h1>
Clip to window
</label>
</li>
<li style="display: none">
<!-- hidden checkbox to enable 'Move/Drag viewport' with url parameter -->
<input id="noVNC_setting_view_clip" type="checkbox">
</li>
<li>
<label for="noVNC_setting_resize">Scaling mode:</label>
<select id="noVNC_setting_resize" name="vncResize">
Expand Down