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

stopPropagation is not respected #467

Closed
michaelmalonenz opened this Issue Jul 28, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@michaelmalonenz
Contributor

michaelmalonenz commented Jul 28, 2016

I'm submitting a bug report

  • Library Version:
    1.0.0

Please tell us about your environment:

  • Operating System:
    Windows [10]
  • Node Version:
    5.11.1
  • NPM Version:
    3.9.5
  • JSPM OR Webpack AND Version
    JSPM 0.16.39
  • Browser:
    all
  • Language:
    all

Current behavior:
I have some nested div elements, which each have click delegates. If I call event.stopPropagation when in the inner-most element, the event delegate on the parent is called.

Expected/desired behavior:
I would expect the event to not bubble to the parent if stopPropagation was called on the event. The behaviour was changed in this commit to start bubbling events like a browser would.

Please find an example gist here When clicking the inner element, I would expect that only it changes colour, not both.

  • What is the expected behavior?
    Essentially, that the event manager respects the stopPropagation behaviour like any browser would.

After looking at the problem, I feel there is an obvious, if slightly awkward solution, and I wanted to get feedback prior to submitting a pull request:

function handleDelegatedEvent(event) {
  let originalStopPropagationFn = event.stopPropagation;
  let propagationStopped = false;
  event.stopPropagation = () => { 
    propagationStopped = true; 
    originalStopPropagationFn.call(event);
  }
  let target = findOriginalEventTarget(event);

  while (target && !propagationStopped) {
    if (target.delegatedCallbacks) {
      let callback = target.delegatedCallbacks[event.type];
      if (callback) {
        callback(event);
      }
    }

    target = target.parentNode;
  }
}
  • What is the motivation / use case for changing the behavior?

So I can have delegates instead of triggers (the problem goes away if I use a trigger on both) except that I have a large number of "inner" elements (hundreds / thousands), so wanted to keep the improved performance promised by delegation (which was possible prior to the bubbling change mentioned earlier).

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Jul 28, 2016

Member

I think this is a good change. We actually discussed this when we were researching the event manager fix.

Couple performance suggestions:

What if we lazily overrode stopPropagation, deferring the override work until we knew we were about to invoke a callback?

What do you think about refactoring the override to eliminate closures?

Here's some examples:

function interceptStopPropagation(event) {
  event.standardStopPropagation = event.stopPropagation;
  event.stopPropagation = function() {
    this.propagationStopped  = true;
    this.standardStopPropagation();
  };
}

function handleDelegatedEvent(event) {
  let interceptInstalled = false;
  event.propagationStopped = false;
  while (target && !event.propagationStopped) {
    if (target.delegatedCallbacks) {
      let callback = target.delegatedCallbacks[event.type];
      if (callback) {
        if (!interceptInstalled) {
           interceptStopPropagation(event);
        }
        callback(event);
      }
    }

    target = target.parentNode;
  }
}
Member

jdanyow commented Jul 28, 2016

I think this is a good change. We actually discussed this when we were researching the event manager fix.

Couple performance suggestions:

What if we lazily overrode stopPropagation, deferring the override work until we knew we were about to invoke a callback?

What do you think about refactoring the override to eliminate closures?

Here's some examples:

function interceptStopPropagation(event) {
  event.standardStopPropagation = event.stopPropagation;
  event.stopPropagation = function() {
    this.propagationStopped  = true;
    this.standardStopPropagation();
  };
}

function handleDelegatedEvent(event) {
  let interceptInstalled = false;
  event.propagationStopped = false;
  while (target && !event.propagationStopped) {
    if (target.delegatedCallbacks) {
      let callback = target.delegatedCallbacks[event.type];
      if (callback) {
        if (!interceptInstalled) {
           interceptStopPropagation(event);
        }
        callback(event);
      }
    }

    target = target.parentNode;
  }
}
@michaelmalonenz

This comment has been minimized.

Show comment
Hide comment
@michaelmalonenz

michaelmalonenz Jul 28, 2016

Contributor

Sounds good - I've amended the Pull Request with the updated code

Contributor

michaelmalonenz commented Jul 28, 2016

Sounds good - I've amended the Pull Request with the updated code

@jdanyow jdanyow closed this in #468 Jul 28, 2016

jdanyow added a commit that referenced this issue Jul 28, 2016

feat(event-manager): enable stopping propagation of delegated events
nested elements with delegate handlers should still benefit from delegate overhead reductions, whilst being able to handle the event on just the inner element

closes #467
@CasiOo

This comment has been minimized.

Show comment
Hide comment
@CasiOo

CasiOo Jul 28, 2016

Contributor

Just a quick note about the implementation: Instead of polluting the event object with more methods, how about keeping the stopPropagation local to interceptStopPropagation?

Also, any reasoning behind adding the interceptor inside the loop and not before the loop?

Contributor

CasiOo commented Jul 28, 2016

Just a quick note about the implementation: Instead of polluting the event object with more methods, how about keeping the stopPropagation local to interceptStopPropagation?

Also, any reasoning behind adding the interceptor inside the loop and not before the loop?

@michaelmalonenz

This comment has been minimized.

Show comment
Hide comment
@michaelmalonenz

michaelmalonenz Jul 28, 2016

Contributor

@CasiOo the reasoning for inside the loop was to avoid adding extra methods to the event object that we aren't using.

As for adding methods to event... if the Creator had wanted us to not do that, then they would have provided a propagationStopped property to us. It's data that belongs to the event, not to us - encapsulating it there makes sense to me.

Contributor

michaelmalonenz commented Jul 28, 2016

@CasiOo the reasoning for inside the loop was to avoid adding extra methods to the event object that we aren't using.

As for adding methods to event... if the Creator had wanted us to not do that, then they would have provided a propagationStopped property to us. It's data that belongs to the event, not to us - encapsulating it there makes sense to me.

@CasiOo

This comment has been minimized.

Show comment
Hide comment
@CasiOo

CasiOo Jul 28, 2016

Contributor

@michaelmalonenz I see what you mean with only adding the interceptor if it's needed. That way we don't interfere with DOM events outside Aurelia 👍 .

For good measure, this is what I originally had in mind.

function handleDelegatedEvent(event) {
  let propagationStopped = false;
  let target = findOriginalEventTarget(event);
  interceptStopPropagation(event);

  while (target && !propagationStopped) {
    if (target.delegatedCallbacks) {
      let callback = target.delegatedCallbacks[event.type];
      if (callback) {
        callback(event);
      }
    }

    target = target.parentNode;
  }

  function interceptStopPropagation(event) {
    let standardStopPropagation = event.stopPropagation;

    event.stopPropagation = function() {
      propagationStopped = true;
      standardStopPropagation.call(this);
    };
  }
}
Contributor

CasiOo commented Jul 28, 2016

@michaelmalonenz I see what you mean with only adding the interceptor if it's needed. That way we don't interfere with DOM events outside Aurelia 👍 .

For good measure, this is what I originally had in mind.

function handleDelegatedEvent(event) {
  let propagationStopped = false;
  let target = findOriginalEventTarget(event);
  interceptStopPropagation(event);

  while (target && !propagationStopped) {
    if (target.delegatedCallbacks) {
      let callback = target.delegatedCallbacks[event.type];
      if (callback) {
        callback(event);
      }
    }

    target = target.parentNode;
  }

  function interceptStopPropagation(event) {
    let standardStopPropagation = event.stopPropagation;

    event.stopPropagation = function() {
      propagationStopped = true;
      standardStopPropagation.call(this);
    };
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment