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.

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

Jump to bottom

.click event fired twice on mobile devices [$20 awarded] #1976

Closed
dandersson01 opened this issue Feb 17, 2015 · 10 comments
Closed

.click event fired twice on mobile devices [$20 awarded] #1976

dandersson01 opened this issue Feb 17, 2015 · 10 comments

Comments

@dandersson01
Copy link

When fabric is included on mobile devices (tested on iPhone 6 and Chrome Developer Tools (with device mode)), any .click event is triggered twice.

The following code will output once click count on desktop but two on mobile devices.

<html>
    <head>
        <script src="jquery-2.1.3.js"></script>
        <script src="jquery.form.min-v20140218.js"></script>
        <script src="event-1.1.5.js"></script>
        <script src="fabric-1.4.13.js"></script>
        <script>
            $(document).ready(function () {
                count = 1;

                $('#menuupload').unbind('click');
                $('#menuupload').click(function(event)
                {
                    console.log('click count: ' + count);
                    count = count + 1;
                });
            });
        </script>
    </head>
    <body>
        <a id="menuupload" href="#">Test</a>
    </body>
</html>

The $20 bounty on this issue has been claimed at Bountysource.
@kangax kangax changed the title .click event fired twice on mobile devices .click event fired twice on mobile devices [$20] Feb 17, 2015
@kangax kangax added the bounty label Feb 17, 2015
@grimor
Copy link

grimor commented Feb 18, 2015

Can you provide assets ? I didn't found event-1.1.5.js nowhere. When I tested that code it worked fine (Chrome Dev Tools with touch emulation):

<html>
    <head>
        <script src="jquery-2.1.3.js"></script>
        <script src="jquery.form.min.js"></script>
        <!-- <script src="event-1.1.5.js"></script> -->
        <script src="fabric.js"></script>
        <script>
            $(document).ready(function () {
                count = 1;

                $('#menuupload').unbind('click');
                $('#menuupload').click(function(event)
                {
                    console.log('click count: ' + count);
                    count = count + 1;
                });
            });
        </script>
    </head>
    <body>
        <a id="menuupload" href="#">Test</a>
    </body>
</html>

EDIT: Ok, i found that in /lib folder. Tested with event-1.1.5.js script on Android (Chrome and stock 4.1.2 browsers) and that doesn't occur. Doesn't have iPhone to test it out.

@dandersson01
Copy link
Author

Running Chrome 40.0.2214.111 (64-bit) on Mac and .click is fired twice (when in device mode with iPhone 6/any mobile phone selected). It does not happen in device mode with "Generic notebook" selected.

In device mode, "Emulate touch screen" and "Emulate mobile" are ticked (among others).

Same results on Windows 8.1 and Chrome 40.0.2214.111 m.

@grimor
Copy link

grimor commented Feb 18, 2015

Weird, I have that setting enabled and don't have that issue. I've uploaded my test files here http://grimor.hmhost.pl/bountysource/fabricjs-test/ can you check if issue happens here ?

@dandersson01
Copy link
Author

Does not happen with the version/build of Fabric you're using.

Yours say:

/* build: `node build.js modules=ALL exclude=gestures,cufon,json minifier=uglifyjs` */

Ah, I think it's because gestures are excluded (in the version you're testing with).

The one I'm using is:

/* build: `node build.js modules=text,cufon,gestures,easing,parser,freedrawing,interaction,serialization,image_filters,gradient,pattern,shadow,node minifier=uglifyjs` */

@grimor
Copy link

grimor commented Feb 18, 2015

Ok, I know what's the problem. It replace 'click' event to two events 'touchstart' and 'mousedown' and mobiles support both so it fires it both. The fastest way to 'fix' that is to comment (in fabric.js build):

eventManager(this, type, listener, configure, trigger, true);

and the uncomment line below

// handler.call(this, type, listener, useCapture);

But I don't know if it won't make new problems (hopefuly not).

I've got an idea how to fix it, but I'll do it later.

PS. You don't have to include events-1.1.5.js it's bundled inside fabric.js

@asturur
Copy link
Member

asturur commented Feb 18, 2015 

      if (e.type === 'touchstart') {
        // Unbind mousedown to prevent double triggers from touch devices
        removeListener(this.upperCanvasEl, 'mousedown', this._onMouseDown);
      }

There is also this in fabric.js i do know if you added this or not. It can be a timing of events.

We have this

    _initEventListeners: function () {

      this._bindEvents();

      addListener(fabric.window, 'resize', this._onResize);

      // mouse events
      addListener(this.upperCanvasEl, 'mousedown', this._onMouseDown);
      addListener(this.upperCanvasEl, 'mousemove', this._onMouseMove);
      addListener(this.upperCanvasEl, 'mousewheel', this._onMouseWheel);

      // touch events
      addListener(this.upperCanvasEl, 'touchstart', this._onMouseDown);
      addListener(this.upperCanvasEl, 'touchmove', this._onMouseMove);

      if (typeof Event !== 'undefined' && 'add' in Event) {
        Event.add(this.upperCanvasEl, 'gesture', this._onGesture);
        Event.add(this.upperCanvasEl, 'drag', this._onDrag);
        Event.add(this.upperCanvasEl, 'orientation', this._onOrientationChange);
        Event.add(this.upperCanvasEl, 'shake', this._onShake);
        Event.add(this.upperCanvasEl, 'longpress', this._onLongPress);
      }
    },

And maybe using this

fabric.isTouchSupported = "ontouchstart" in fabric.document.documentElement;

    _initEventListeners: function () {

      this._bindEvents();

      addListener(fabric.window, 'resize', this._onResize);

      // mouse events
      if (!fabric.isTouchSupported) {
        addListener(this.upperCanvasEl, 'mousedown', this._onMouseDown);
        addListener(this.upperCanvasEl, 'mousemove', this._onMouseMove);
      }
      addListener(this.upperCanvasEl, 'mousewheel', this._onMouseWheel);

      // touch events
      addListener(this.upperCanvasEl, 'touchstart', this._onMouseDown);
      addListener(this.upperCanvasEl, 'touchmove', this._onMouseMove);

      if (typeof Event !== 'undefined' && 'add' in Event) {
        Event.add(this.upperCanvasEl, 'gesture', this._onGesture);
        Event.add(this.upperCanvasEl, 'drag', this._onDrag);
        Event.add(this.upperCanvasEl, 'orientation', this._onOrientationChange);
        Event.add(this.upperCanvasEl, 'shake', this._onShake);
        Event.add(this.upperCanvasEl, 'longpress', this._onLongPress);
      }
    },

Maybe in this way is fixed. A full testing is required on mobile device.
I'm just speculating i did test nothing.

@grimor
Copy link

grimor commented Feb 18, 2015

Ok, I solved it. I will add PR soon.

UPDATE: I've builded fabric.js from source using build script and it doesn't have that issue, even without my fix. Maybe that was fixed earlier. Although, I have that problem with custom build generated from the fabric.js website

@dandersson01
Copy link
Author

Yes, the bug seems to be fixed in the current source.

Will use that from now on, thanks. Will award bounty.

@dandersson01
Copy link
Author

As soon as I realize how one awards bounties..

Issue can be closed, as it can't be found in the latest source.

@asturur
Copy link
Member

asturur commented Feb 18, 2015

@grimor will claim the bounty, you will accept.

i close the issue

@asturur asturur closed this as completed Feb 18, 2015
@kangax kangax added the bounty label Feb 18, 2015
@kangax kangax changed the title .click event fired twice on mobile devices [$20] .click event fired twice on mobile devices Feb 18, 2015
@kangax kangax changed the title .click event fired twice on mobile devices .click event fired twice on mobile devices [$20 awarded] Feb 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kangax @grimor @asturur @dandersson01