$.clone doesn't clone delegated events #73

AVGP · 2015-12-10T07:58:57Z

This test fails but should not:

    var element = $.create("div", { class: "parent",  contents: "This is the parent" });
    element._.delegate("click", "p", function() {
      done();
    });

    var child = $.create("p");
    element.appendChild(child);

    var clone = $.clone(element);

    var ev = document.createEvent("MouseEvent");
        ev.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
    clone.childNodes[1].dispatchEvent(ev);

guoguo12 · 2015-12-10T14:13:07Z

I believe it works if you do $.create("p", {}). $.create("p") creates a text node, so it won't match the selector "p" in the delegate call.

guoguo12 · 2015-12-10T14:17:18Z

Oh, I see there was more discussion about this in #70. @LeaVerou, can you verify what I just wrote?

AVGP · 2015-12-10T14:24:22Z

@guoguo12 You're right about the glitch with the text node, but the event still doesn't fire.

        var element = $.create("div", { class: "parent",  contents: "This is the parent" });
    element._.delegate("click", "p", function() {
      done();
    });

    var child = $.create("p", {});
    element.appendChild(child);

    var clone = $.clone(element);

    var ev = document.createEvent("MouseEvent");
        ev.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
    clone.childNodes[1].dispatchEvent(ev);

And either I'm really blind today, or it won't even work using addEventListener on the clone when bliss is used:

var parent = document.createElement("div");
var child = document.createElement("span");

parent.appendChild(child);

var clone = parent.cloneNode(true);

clone.addEventListener("click", function() {
  done();
});

var ev = document.createEvent("MouseEvent");
ev.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
clone.querySelector("span").dispatchEvent(ev);

Nota bene: This JSFiddle demonstrates that this code should be okay.

guoguo12 · 2015-12-10T14:33:21Z

@AVGP: Hmm, interesting. Are you saying it only fails during the Karma test?

AVGP · 2015-12-10T14:41:37Z

Yes. It's weird, isn't it?

Here's yet another sample of what works:

<!doctype html>
<html>
  <body>
    <div id="test">
      <p>Click me</p>
    </div>
    <script src="bliss.min.js"></script>
    <script>
      var original = document.getElementById("test");

      original._.delegate("click", "p", function() {
        alert("OK");
      })

      var clone = $.clone(original);

      var ev = document.createEvent("MouseEvent");
      ev.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
      clone.querySelector("p").dispatchEvent(ev);

    </script>
  </body>
</html>

zdfs · 2015-12-10T16:15:09Z

Are you sure that's the only <p> tag on the page when you run that code?

AVGP · 2015-12-10T16:17:54Z

@zdfs I don't see how that matters here?
The only <p> elements are the one in the original element and in the clone.
clone.querySelector only matches on the descendants of clone anyway... do I misunderstand how delegate works?

zdfs · 2015-12-10T16:22:00Z

I see it now. Sorry. I was worried for a second that maybe the delegated event wasn't actually being called. I need more coffee.

AVGP · 2015-12-10T16:23:12Z

No worries, I just wanted to make sure I'm not missing the elephant in the room ;-)

zdfs · 2015-12-10T16:25:11Z

What about adding actual contents to the p tag. In my local testing, this seems to work.

zdfs · 2015-12-10T16:26:00Z

This:

var element = $.create("div", { class: "parent",  contents: "This is the parent" });
element._.delegate("click", "p", function() {
  done();
});

var child = $.create("p", { contents: 'test' });
element.appendChild(child);

var clone = $.clone(element);

var ev = document.createEvent("MouseEvent");
    ev.initMouseEvent("click", true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, null);
clone.childNodes[1].dispatchEvent(ev);

AVGP · 2015-12-10T16:31:21Z

@zdfs Yes, that, too, works in a normal page but not in Karma... what the heck?

zdfs · 2015-12-10T16:50:32Z

Weird. It's working in Karma for me.

zdfs · 2015-12-10T16:52:09Z

You're passing the done parameter, right?

dperrymorrow · 2015-12-10T17:49:48Z

Probably unrelated, but might make your life easier when dealing with DOM items.
Have you tried out using HTML fixtures? That way you can just load up whatever HTML you want and not have to create it all manually with JS for each test.

https://github.com/LeaVerou/bliss/blob/gh-pages/tests/CoreSpec.js#L4-L14
https://github.com/LeaVerou/bliss/blob/gh-pages/tests/fixtures/core.html

zdfs · 2015-12-10T21:28:03Z

@AVGP - Is this still relevant? I just merged your PR.

AVGP · 2015-12-10T22:11:25Z

@zdfs unfortunately yes.

I'll try later if HTML fixtures work or if it's just my local Karma + Chrome Beta combination that behaves weird...

LeaVerou · 2015-12-11T20:30:01Z

Regarding the $.create() issue:

This is exactly why we shouldn't be using Bliss methods in tests, apart from the one being currently tested.
I wonder if the mistake hints that the API is confusing. Especially since, I didn't even notice it when I first read the code! Treating plain strings as text nodes is certainly useful inside $.contents(), but not sure about $.create(). Think about it: If there's a second parameter, it creates an element. If there's only one, but it's an object, it creates an element. If there's NO parameter, it still creates an element. And if there's one, which is a string, it creates a text node?! I think it's internally inconsistent and we need to change it and move the text node logic to $.contents() instead. Thoughts?

guoguo12 · 2015-12-11T20:34:56Z

Agreed. It's currently way too confusing.
On Dec 11, 2015 12:30 PM, "Lea Verou" [email protected] wrote:

Regarding the $.create() issue:

This is exactly why we shouldn't be using Bliss methods in tests,
apart from the one being currently tested.

I wonder if the mistake hints that the API is confusing. Especially
since, I didn't even notice it when I first read the code! Treating plain
strings as text nodes is certainly useful inside $.contents(), but not
sure about $.create(). Think about it: If there's a second parameter,
it creates an element. If there's only one, but it's an object, it creates
an element. If there's NO parameter, it still creates an element. And if
there's one, which is a string, it creates a text node?! I think it's
internally inconsistent and we need to change it and move the text node
logic to $.contents() instead. Thoughts?

—
Reply to this email directly or view it on GitHub
#73 (comment).

zdfs · 2015-12-11T21:16:40Z

Agreed.

LeaVerou · 2015-12-11T22:25:55Z

Done. :)

LeaVerou · 2015-12-24T14:54:36Z

Apart from the $.create() issue, is the actual bug reported here still valid? If not, we should close this.

zdfs added the tests label Dec 11, 2015

LeaVerou mentioned this issue Dec 11, 2015

$.create() should always create elements #85

Closed

LeaVerou closed this as completed Dec 24, 2015

LeaVerou reopened this Dec 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

$.clone doesn't clone delegated events #73

$.clone doesn't clone delegated events #73

AVGP commented Dec 10, 2015

guoguo12 commented Dec 10, 2015

guoguo12 commented Dec 10, 2015

AVGP commented Dec 10, 2015

guoguo12 commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

zdfs commented Dec 10, 2015

dperrymorrow commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

LeaVerou commented Dec 11, 2015

guoguo12 commented Dec 11, 2015

zdfs commented Dec 11, 2015

LeaVerou commented Dec 11, 2015

LeaVerou commented Dec 24, 2015

$.clone doesn't clone delegated events #73

$.clone doesn't clone delegated events #73

Comments

AVGP commented Dec 10, 2015

guoguo12 commented Dec 10, 2015

guoguo12 commented Dec 10, 2015

AVGP commented Dec 10, 2015

guoguo12 commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

zdfs commented Dec 10, 2015

zdfs commented Dec 10, 2015

dperrymorrow commented Dec 10, 2015

zdfs commented Dec 10, 2015

AVGP commented Dec 10, 2015

LeaVerou commented Dec 11, 2015

guoguo12 commented Dec 11, 2015

zdfs commented Dec 11, 2015

LeaVerou commented Dec 11, 2015

LeaVerou commented Dec 24, 2015