Cannot have different event handlers for different instances of ZeroClipboard() #90

Closed
cacieprins opened this Issue Jan 30, 2013 · 48 comments
@cacieprins

Using the following code:

/* MultiTest.js */
var copy = function(root) {
        var thisContext = this;
        this.message = root.data('message');
    this.text = root.data('clipboard-text');
        this.clip = new ZeroClipboard(root, {
                moviePath : 'ZeroClipboard.swf'
        });
        this.clip.on('complete', function() {
                thisContext.on_complete(arguments);
        });
};

copy.prototype.on_complete = function(client, args) {
        console.log(this.message);
        console.log(this.text);
};

$(document).ready(function() {
        var copyB = new copy($('#ButtonTwo'));
    var copyA = new copy($('#ButtonOne'));
});

And the following markup:

<!doctype html>
<html>
<head>
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.9.0/jquery.min.js"></script>
<script src="ZeroClipboard.js"></script>
<script src="MultiTest.js"></script>
</head>
<body>
<button id="ButtonOne" data-clipboard-text="Foo" data-message="Foo has been copied to your clipboard, maybe." />
<button id="ButtonTwo" data-clipboard-text="Bar" data-message="Bar has been copied to your clipboard, maybe." />
</body>
</html>

No matter which button you click, whichever was instantiated first is the object which is output to the console. Surprisingly, what is actually copied to the clipboard is what is correct.

Any suggestions/workarounds until a fix for this is issued? I may dive into your codebase in the next few days to fix it if not.

@cacieprins

This issue probably stems from the weird singleton pattern usage -- only one event handler can be attached for each event across the entire page (see: line 22 of event.js)!

Modifying line 62 of event.js to be

var element = args.currentElement = currentElement

gives the event listener the chance to find the originating element. But not being able to have multiple event handlers for different clip objects is kind of a gross issue that should be taken care of.

@JamesMGreene
Member

This was done intentionally (see here) but we fully acknowledge that it is a bug that needs to be addressed soon.

@cacieprins

Shiny. I'll see if I can take some time to fix it this weekend - I've got a couple ideas on how it could play out.

@bstrilziw

I really would love to see this fixed. :)

@joelzimmer

Seconded. ZeroClipboard used to allow for multiple instances in a page.

@JamesMGreene JamesMGreene was assigned May 7, 2013
@JamesMGreene
Member

Implementation plan:

  1. ZC "client" instances get assigned an internal ID (number).
  2. The ZC global object keeps a reference to all "client" instances.
    • NOTE: This may require that we add a destroy method (or something) to the "client" instances to enable a user to completely eliminate the references (e.g. for garbage collection, etc.).
  3. Any elements glued to that "client" have the ID added to an HTML "data" attribute, e.g. data-clipboard-groups. Conversely, any elements unglued from that "client" have the ID removed from the HTML "data" attribute.
  4. The ZC global object keeps an internal reference to the "client" that was last activated (i.e. when the mouseover occurs and repositions the Flash object
  5. When the Flash calls back to the ZC global object to dispatch an event, we look at the last activated "client", grab its "data" attribute, parse the value into a list of related IDs, and then dispatch the event to each associated "client" instance.
    • NOTE: Each "client" will now need to have its own small set of "dispatcher" functionality.
@jonrohan
Member

Any elements glued to that "client" have the ID added to an HTML "data" attribute, e.g. data-clipboard-groups. Conversely, any elements unglued from that "client" have the ID removed from the HTML "data" attribute.

Can multiple clients be glued to a single html element?

@JamesMGreene
Member

Yes, the additional "client" ID will just be added to the list, i.e. data-clipboard-groups="1,2,3". Therefore, all clients that are glued to that element will have their corresponding event handlers triggered.

@jonrohan
Member

Sounds like a plan to me

@JamesMGreene
Member

Any preference on the "data" attribute's name? e.g.

  • data-clipboard-groups
  • data-clipboard-clients
  • data-clipboard-ids (I'm a little weary of this one as it may be confused with data-clipboard-target)
  • etc.
@jonrohan
Member

clients sounds right

@JamesMGreene
Member

Cool, thanks for the feedback.

@JamesMGreene JamesMGreene was assigned May 17, 2013
@JamesMGreene
Member

Looked into it a bit more, seems like we can simplify my plan as Points 1 and 3 shouldn't actually be necessary. We can instead just ensure that the existing gluedElements array becomes a client-specific array rather than a global. Point 4 is also already implemented as currentElement. Then for Point 5, we just adapt the logic to loop through the client instances (tracked via Point 2) and then loop through their individual gluedElements arrays to see if they contain currentElement.

@bergmark

In my case I only had one instance of ZC active at a time, and the issue was that the new callbacks didn't register or trigger properly (copying worked). I temporarily fixed this by doing

if (ZeroClipboard.prototype._singleton) ZeroClipboard.destroy();
var clip = ZeroClipboard(...);
@JamesMGreene
Member

@bergmark That sounds odd. Would you mind filing a new bug with more info? e.g. repro script/gist/fiddle/repo plus info from http://aboutmybrowser.com

@bergmark

As far as i can tell this is the same issue. But I'll make a new issue if you'd like!

The behavior is same in both chrome https://aboutmybrowser.com/vaBrvgax and firefox https://aboutmybrowser.com/qCs2V1ai

With the if line commented out, the correct text is copied for both elements (A -> "A", B -> "B"), but the complete event triggers for A each time. With the if line uncommented the A instance no longer works (expected) and B's event triggers properly.

<!doctype html>
<html>
  <head>
    <style> a { display: block; width: 100px; height: 100px; background-color: gray; margin: 10px; }</style>
    <script type="text/javascript" src="http://code.jquery.com/jquery-1.10.1.js"></script>
    <script type="text/javascript" src="ZeroClipboard.js"></script>
    <script>
$(document).ready(function () {
  function makeButton (text)
  {
    var b = $("<a class='copy-button' href='#'/>");
    b.text(text);
    b.attr("data-clipboard-text", text);
    b.appendTo(document.body);

    // Uncomment this line to make B's complete event trigger properly
    // (it destroys the ZC instance for A.)
    // if (ZeroClipboard.prototype._singleton) ZeroClipboard.destroy();
    var clip = new ZeroClipboard(b[0], { moviePath : "ZeroClipboard.swf" });
    clip.on("complete", function () {
      b.text("Copied " + text);
      console.log("copied", text);
    });
  }
  makeButton("A");
  makeButton("B");
});
    </script>
  </head>
  <body>
  </body>
</html>

@JamesMGreene
Member

CCing @a-frolovsky-parc for watching.

@terinjokes

I've noticed this today attempting to port forward a codebase from the Google Code version of this library. We can have multiple clients on a page, and now we can only support one.

Not supporting this leaves a lot of users with no upgrade path from the old XSS-vulnerable versions.

@JamesMGreene
Member

Wholeheartedly agreed, @terinjokes. Unfortunately supporting this involves a lot of changes... so, although I actually have it working locally, I've had to backtrack to make my changes in smaller increments for iterative progress or else the diff is so big it's incomprehensible.

Rest assured, it is on the short list for an upcoming release. 👍

@terinjokes

I actually got the legacy library working against 1.1.7 today—I'll publish it on GitHub when I get a moment tonight—but having separate instances would still be much nicer.

@jakedouglas

@JamesMGreene any idea when this is coming? I've been considering taking a swing at it, but I don't want to duplicate the work if you've already done it.

@terinjokes

I ended up working this out at a layer above: https://github.com/terinjokes/zClip

@JamesMGreene
Member

Sorry, guys. Life's just super busy lately.

I finished most of the work a while ago but it had snowballed into what would've been a PR that was too hard to review, so I took a step back and started breaking it down into smaller pieces. Haven't been able to pick it up again for a few weeks now, though.

I also started working on a prototype of the new APIs to address Issues #143 & #152 as of Friday, so I'm a bit scattered.

@trodrigues

Hi there. I just started using ZeroClipboard and came across the issue that ZC is effectively a singleton because I have 2 different buttons and want to trigger a tooltip when you hover on each.

After reading the docs a second time I realized this is the current affected element, so that helped me and solved my problem.

However, it still took me a while to find out what was really happening (the singleton issue). I think one of two things should be done:

  • expose ZC properly as a singleton, as in, having just one object with a create method or something to create the clipboards (and of course, allow for a way to identify instances inside event handlers)
  • warn people at the start of the docs that it is effectively a singleton they're dealing with

For any JS developer, when you see something using new ZeroClipboard(...) you assume you're gonna get different instances, so the way this works is completely confusing.

@alexstrat
Contributor

What is the status of this bug? Is it still present? What are the plans to fix it? Thanks.

@JamesMGreene
Member

I was working on it back in September but I ran into issues feeling safe about the changes since our unit tests were all in nodeunit instead of browser-based testing framework. I've been busy with life and a side project since then but was recently able to set some time aside to convert all the tests over to QUnit, thus removing the only major roadblock.

That said, I am still busy with life and that same side project but I expect to be able to steal enough time to fix this before the end of the year... it requires a sizeable overhaul of the internals.

@alexstrat
Contributor

Ok, is the fix simple? Can we help? Can you give us a preview ^^

If it can help, I recently transformed a nodeunit test suite into a nodeJS and cross-browser test suite CI moment/moment#1301

@alexstrat alexstrat added a commit to alexstrat/zeroclipboard that referenced this issue Dec 2, 2013
@alexstrat alexstrat Add a known issues section with #90 3f1224c
@BeOleg
BeOleg commented Dec 10, 2013

+1 willing to contribute code to solve this issue, can write Jasmine \ Mocha tests if required.

@JamesMGreene
Member

Thanks for the offers, guys. This is already in progress, just need to find some time to wrap it up. I can guarantee this will be fixed by the end of 2013, if not sooner.

@terinjokes

@JamesMGreene When you have something on a branch, can you let me know so I can update my zclip fork?

@JamesMGreene
Member

@terinjokes: Yeah, I'll be sure to update this issue thread.

@reywright

+1 for this.

@paltman
paltman commented Dec 26, 2013

Hitting this issue in a bad way and am looking forward to the code push to remove the singleton issue.

@mcohen9
mcohen9 commented Dec 31, 2013

+1 need this

@JamesMGreene
Member

It's in progress... hopefully pushing tonight.

@JamesMGreene
Member

Sorry, all, this one is a bit of a nightmare to untangle. Lots of behavioral changes and API changes, leading to test changes, etc.

@JamesMGreene
Member

Minor update: I believe all the necessary code changes are done [locally], now just working through the unit test failures now... about 50% of the tests fail but that's about what I expected.

@terinjokes

@JamesMGreene that tends to happen in a semver major bump :)

@JamesMGreene
Member

@terinjokes: Yes but unfortunately this will actually be a minor bump with a ton of deprecations, followed by a major bump. The two massive changes in behavior that will be deemed acceptable within a minor bump are:

  1. All configuration is now globally shared rather than per-client
    • Because there has only been a singleton client for quite a while now, changing this to be globally shared won't really change much. Additionally, this will be the correct/desired behavior going forward into v2.0.0.
  2. Having separate client instances rather than a shared singleton.
    • This was considered a bug from the time that design choice was made but we have since figured out how to accommodate both that design goal (only having 1 Flash object) and still allowing multiple clients.
@JamesMGreene
Member

@terinjokes: Anyway, thanks for the vote of confidence and kind words. 👍

Hoping to push tonight... 177 out of 313 assertions passing at the moment.

@JamesMGreene
Member

Down to 1 very annoying failure. Should be able to push today. 🤘

@JamesMGreene JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this issue Jan 9, 2014
@JamesMGreene JamesMGreene Removed client singleton pattern.
Deprecated lots of functions related to the v2.x API; details in Issue #289.

Fixes #90.
2619430
@JamesMGreene
Member

BOOM! 💥 PR #311

@terinjokes

Thanks James. Mind if I have a couple days to try it out and provide some feedback?

@JamesMGreene
Member

@terinjokes: This is a substantial change in ZeroClipboard (as evidenced by the massive number of changes in the PR):

Showing 26 changed files with 2,094 additions and 926 deletions

So, yes, please take your time (though I'd prefer to get it merged within, say, 7 days) to help test it thoroughly. We'd appreciate it! 👍

@terinjokes

I meant that as "I'm going to spend a couple of days working against this so don't merge it until I give a thumbs up" but in a way that makes it look like you giving me permission. 😛

@JamesMGreene
Member

Oh, gotcha. 👍 I thought you meant you couldn't get around to testing it for a few days. :)

@bebraw bebraw referenced this issue in asafdav/ng-clip Jan 10, 2014
Closed

Fails with ng-repeat #7

@bebraw bebraw added a commit to MaxCDN/osscdn that referenced this issue Jan 12, 2014
@bebraw bebraw frontend - Replace file links with copy buttons
Note that copy is still bugged due to an issue in ZeroClipboard. Need to
wait for that to get fixed.

More info: zeroclipboard/zeroclipboard#90 .
d085692
@JamesMGreene JamesMGreene added a commit that closed this issue Jan 14, 2014
@JamesMGreene JamesMGreene Removed client singleton pattern.
Deprecated lots of functions related to the v2.x API; details in Issue #289.

Fixes #90.
b2ecc11
@JamesMGreene
Member

Patch has been released as v1.3.0-beta.1.

@terinjokes

Looked good here

@bebraw bebraw added a commit to MaxCDN/osscdn that referenced this issue Jan 18, 2014
@bebraw bebraw frontend - Replace file links with copy buttons
Note that copy is still bugged due to an issue in ZeroClipboard. Need to
wait for that to get fixed.

More info: zeroclipboard/zeroclipboard#90 .
59c3c28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment