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

Emscripten: fix duplicate mousebuttonup/mousebuttondown events when touch events are disabled #64

Merged
merged 1 commit into from Dec 8, 2018

Conversation

Projects
None yet
2 participants
@Beuc
Contributor

Beuc commented Nov 18, 2018

Both SDL2 and the browser attempt to simulate mouse events from touch events.
As a result we may get duplicates.

By default, on touch event on the canvas we generates fake mouse events with mouse identifier which=SDL_TOUCH_MOUSEID.

However if we disable touch events:

  SDL_EventState(SDL_FINGERDOWN, 0);
  SDL_EventState(SDL_FINGERUP, 0);

then we still generate the fake mouse events (from browser's touchstart/touchend, because the touch handler is still called even if the events are not passed to the application), but we also generate duplicate events from mouse identifier which=0 (from browser's simulated mousedown/mouseup - which were not triggered before as the touch handler currently issues a preventDefault when touch events are enabled).

This is not consistent with e.g. the Android SDL port, in which we simply always get the mouse events from which=SDL_TOUCH_MOUSEID (plus the touch events, if enabled).
(There's also an Android SDL_HINT_ANDROID_SEPARATE_MOUSE_AND_TOUCH mode but that's separate and used to stop generating fake mouse events entirely.)

According to https://wiki.libsdl.org/SDL_MouseButtonEvent#Remarks, the right events we need to propagate are the SDL_TOUCH_MOUSEID ones.

With this patch the behavior is consistent with the Android port with both enabled and disabled touch events.

What do you think?

@Beuc

This comment has been minimized.

Contributor

Beuc commented Nov 23, 2018

Ping?

@Daft-Freak

This comment has been minimized.

Member

Daft-Freak commented Nov 24, 2018

Haven't had a chance to test this, but I think one of the reasons for the original checks was so you could scroll with touch events disabled.

preventDefault = 1;
}
// no browser's simulated mousemove if we block mousedown
preventDefault = 0;

This comment has been minimized.

@Daft-Freak

Daft-Freak Nov 24, 2018

Member

Shouldn't need this (it's already 0)

This comment has been minimized.

@Beuc

Beuc Nov 24, 2018

Contributor

Hi!

As far as I can test, "wheel" events are handled independently (HandleWheel), and the browser does not simulate them on touch devices. We should be safe on that front :)

I added the "preventDefault = 0" line for clarity / symmetry.
I believe it is removed by trivial compiler optimization.
Do you want me to drop/comment it?

@Beuc

This comment has been minimized.

Contributor

Beuc commented Nov 30, 2018

Ping?

@Daft-Freak

This comment has been minimized.

Member

Daft-Freak commented Dec 1, 2018

Hmm.. I just tested this in Chrome and FIrefox on Android and with this example:

#include <emscripten/emscripten.h>
#include "SDL.h"

void loop()
{
    SDL_Event event;
    while (SDL_PollEvent(&event)) {
        switch(event.type) {
            case SDL_MOUSEMOTION:
                if(event.motion.which != SDL_TOUCH_MOUSEID)
                    printf("mousemotion %i\n", event.motion.which);
                break;
            case SDL_MOUSEBUTTONDOWN:
                if(event.button.which != SDL_TOUCH_MOUSEID)
                    printf("mousebuttondown %i\n", event.button.which);
                break;
            case SDL_MOUSEBUTTONUP:
                if(event.button.which != SDL_TOUCH_MOUSEID)
                    printf("mousebuttonup %i\n", event.button.which);
                break;

            case SDL_FINGERMOTION:
                printf("fingermotion %lli\n", event.tfinger.touchId);
                break;
            case SDL_FINGERDOWN:
                printf("fingerdown %lli\n", event.tfinger.touchId);
                break;
            case SDL_FINGERUP:
                printf("fingerup %lli\n", event.tfinger.touchId);
                break;
        }
    }
}

int main(int argc, char *argv[])
{
    SDL_Window *window;

    if (SDL_Init(SDL_INIT_VIDEO) < 0) {
        return (1);
    }

    window = SDL_CreateWindow("Test", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 640, 480, 0);

    SDL_EventState(SDL_FINGERDOWN, SDL_IGNORE);
    SDL_EventState(SDL_FINGERUP, SDL_IGNORE);
    SDL_EventState(SDL_FINGERMOTION, SDL_IGNORE);

    emscripten_set_main_loop(loop, 0, 1);

    return (0);
}

No unexpeceted mouse events are generated even without this patch.

@Beuc

This comment has been minimized.

Contributor

Beuc commented Dec 1, 2018

AFAICS you're filtering out SDL_TOUCH_MOUSEID - this is not meant to be.
Did you compare your test's behavior with a native Android SDL2 app?

@Daft-Freak

This comment has been minimized.

Member

Daft-Freak commented Dec 1, 2018

I though the issue was getting duplicate mouse events generated by the browser? I was filtering SDL_TOUCH_MOUSEID because those events are expected.

@Beuc

This comment has been minimized.

Contributor

Beuc commented Dec 1, 2018

Test:

Native Android:

mousemotion -1
mousebuttondown -1
mousebuttonup -1

emscripten SDL2 + Firefox Android:

mousemotion -1
mousebuttondown -1
mousebuttonup -1
mousemotion 0
mousebuttondown 0
mousebuttonup 0

Conclusion: duplicate event with Emscripten SDL2.

Clear enough? :)

@Daft-Freak

This comment has been minimized.

Member

Daft-Freak commented Dec 1, 2018

Okay, I see what I was doing wrong (I was moving my finger). With this change though, you can't scroll the page with the touch events disabled. It looks like with only FINGERUP always setting preventDefault to 1 the duplicate events are prevented and scrolling still works.

@Beuc Beuc force-pushed the Beuc:patch-2 branch from 048eac6 to d6093f0 Dec 1, 2018

@Beuc

This comment has been minimized.

Contributor

Beuc commented Dec 1, 2018

How about this new commit?

preventDefault = 1;
}
// setting preventDefault=1 breaks scrolling/pinch-to-zoom with finger events SDL_IGNOREd
//preventDefault = 0;

This comment has been minimized.

@Daft-Freak

Daft-Freak Dec 8, 2018

Member

This should be left alone, we still want to prevent default touch actions if the app has enabled touch events.

@Beuc Beuc force-pushed the Beuc:patch-2 branch from d6093f0 to cb9d6d9 Dec 8, 2018

@Beuc

This comment has been minimized.

Contributor

Beuc commented Dec 8, 2018

There you go!

@Daft-Freak

This comment has been minimized.

Member

Daft-Freak commented Dec 8, 2018

Okay, one last change: /* comments */.

@Beuc Beuc force-pushed the Beuc:patch-2 branch from cb9d6d9 to 091a002 Dec 8, 2018

@Beuc

This comment has been minimized.

Contributor

Beuc commented Dec 8, 2018

The SDL2 source code (including Emscripten-specific parts) is full of '//' comments.
Besides unhealthily testing my patience, what is the point?
Done anyway.

@Daft-Freak Daft-Freak merged commit 54f268a into emscripten-ports:master Dec 8, 2018

@Daft-Freak

This comment has been minimized.

Member

Daft-Freak commented Dec 8, 2018

All the ones in the Emscripten backend are actually in inline JS. Upstream seems to want even the Emscripten backend to compile with a C89 compiler. Sorry about how long this took.

@Beuc

This comment has been minimized.

Contributor

Beuc commented Dec 8, 2018

Thanks for the on-going feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment