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.

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

Adds tap-to-click in viewport drag mode #600

Merged
merged 1 commit into from
Apr 28, 2016
Merged

Conversation

jp-bennett
Copy link
Contributor

We wait until the mouse button is released to determine whether a mouse event is a tap or drag, and make that determination by tracking whether the viewport has actually moved.

This makes for a better experience on mobile, I think. So much so that for my use, I'm starting mobile users in viewport dragging mode by default.

--Jonathan Bennett

@samhed
Copy link
Member

samhed commented Apr 18, 2016

Thanks for working on this! It is a very neat feature to have!

Quick tests show that this works on Chrome/Firefox on a Linux desktop, and on Firefox on Android. However, it doesn't seem to work on Chrome 49 on Android?

Lastly, it would be nice if we could respect the read-only mode for this code as well.

@jp-bennett
Copy link
Contributor Author

Odd that there is an issue for you in Chrome. I am running chrome 49 on a shield tablet, and it works here. Could your tap have been interpreted as a tiny drag?

I will tweak the patch to respect read-only mode, should be trivial.

@jp-bennett
Copy link
Contributor Author

@samhed I just updated the PR, should respect read-only now.

--Jonathan Bennett

@samhed
Copy link
Member

samhed commented Apr 18, 2016

Thank you!

Some more tests on Chrome 49 on Android show that it does work very rarely.. perhaps one in 100 taps while in viewport drag mode goes through on my Samsung S6. Testing on a Nexus 4 phone goes better but still not perfect, roughly half my taps go through tough. I found that it works perfectly on a Nexus 7 tablet however.

This seems to be a general weird behavior in Chrome on some devices (perhaps related to dpi) and not an effect of your patch. However since it directly effects the end result of this feature;

Could modify _handleMouseMove in your commit to use your new variable to add a threshold like this:

    _handleMouseMove: function (x, y) {

        if (this._viewportDragging) {
            var deltaX = this._viewportDragPos.x - x;
            var deltaY = this._viewportDragPos.y - y;

            // 10 pixel threshold for panning
            if (this._viewportHasMoved || Math.abs(deltaX) > 10 || Math.abs(deltaY) > 10) {
                this._viewportHasMoved = true;

                this._viewportDragPos = {'x': x, 'y': y};
                this._display.viewportChangePos(deltaX, deltaY);
            }

            // Skip sending mouse events
            return;
        }

        if (this._view_only) { return; } // View only, skip mouse events

        if (this._rfb_state !== "normal") { return; }
        RFB.messages.pointerEvent(this._sock, this._display.absX(x), this._display.absY(y), this._mouse_buttonMask);
    },

I have verified that your code (with my modification above) works on Firefox and Chrome on Android as well as Safari on iOS.

Also, please be clear in the commit message that you are adding a 10px threshold for viewport dragging.

@jp-bennett
Copy link
Contributor Author

That should work. I'll test this myself later, and amend the PR.

@samhed samhed self-assigned this Apr 18, 2016
@samhed
Copy link
Member

samhed commented Apr 26, 2016

Did you get a chance to test this with the threshold modification? Personally I think it works very well.

@jp-bennett
Copy link
Contributor Author

@samhed on my todo list, hope to take a look tonight.

@jp-bennett
Copy link
Contributor Author

@samhed That works really well. You want me to edit the existing PR to include your change?

@samhed samhed merged commit 27e77d4 into novnc:master Apr 28, 2016
samhed added a commit that referenced this pull request Apr 28, 2016
@samhed
Copy link
Member

samhed commented Apr 28, 2016

I committed it, np. Thanks!

samhed added a commit that referenced this pull request Apr 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants