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.

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

Event target is different for tap vs press when used in Polymer/shadow-dom #21

Closed
mpilone opened this issue Oct 28, 2019 · 6 comments · Fixed by #22
Closed

Event target is different for tap vs press when used in Polymer/shadow-dom #21

mpilone opened this issue Oct 28, 2019 · 6 comments · Fixed by #22

Comments

@mpilone
Copy link

mpilone commented Oct 28, 2019

I originally opened issue #18 regarding the behavior I was seeing but realized the issue was with how I was debugging. Now that I have that straightened out, I think I see the real issue that is tripping up VisJs Timeline.

In the simple test case I attached, I get different targets in the event for press vs tap. For press, the event target appears to be properly set to the inner element that is actually pressed. For tap, the event target appears to be set to the outer container element that Hammer is associated with.

 2019-10-28 10-44-15

I'm not sure which behavior is "correct", but it seems odd that they are different. From a simple example using the original Hammer it looks like the inner element should always be the target. This is what is tripping up VisJs Timeline. When pressing it can determine the actual child item pressed. When tapping, it doesn't get the actual child element so selection functionality doesn't work.

I assume this is related to being inside a Polymer element/shadow-dom because the simple example (linked above) seems to work fine. However I haven't found the smoking gun of how Polymer would be interfering with element selection or why it would only affect tap and not press.

Hopefully this is a real issue unlike the last one I opened :)

start-polymer3.zip

@mpilone
Copy link
Author

mpilone commented Oct 28, 2019

I found that with a tap, Hammer generates the event after getting the combination of pointerdown/pointerup. The pointerdown event seems to be properly targeting the clickArea while pointerup is targeting the polymer element, maybe due to event retargeting.

For a press, Hammer generates the event based on just the pointerdown event so the target is the clickArea and then the pointerup events are ignored.

Here's the output after adding some debugging logging in computeInputData:

var target = manager.element;

  // MIKE
  console.log("input.srcEvent.type: " + input.srcEvent.type);
  console.log("input.srcEvent.target.id: " + input.srcEvent.target.id);
  console.log("input.srcEvent.target.tagName: " + input.srcEvent.target.tagName);
  console.log("target.id: " + target.id);
  console.log("target.tagName: " + target.tagName);

  if (hasParent(input.srcEvent.target, target)) {
    target = input.srcEvent.target;
  }

  input.target = target;

 2019-10-28 11-23-47

@mpilone
Copy link
Author

mpilone commented Oct 28, 2019

Looking through the code, in input/mouse.js there are two groups of events, element and window:

const MOUSE_ELEMENT_EVENTS = 'mousedown';
const MOUSE_WINDOW_EVENTS = 'mousemove mouseup';

The mousedown event is an element event which may explain why the target element is the actual, clicked element. However the mouseup event is a window event which may explain why it would see the target as the polymer/web component element because of event retargeting. That is, when capturing events at the window level the source event target will be retargeted to the component element, not the actual target in the shadow dom. It may be necessary to use event.composedPath() when looking for the original target rather than the source event target directly if Hammer needs to capture events at the Window level.

@WoodNeck
Copy link
Member

Hello @mpilone, good to see you again :)
You made really nice description explaining the issue. That was super helpful.
I made a PR(#22) fixing this issue, it will be nice if you review it.
I'm just wondering which one of event.composedPath() and event.path in srcEvent is better.

@mpilone
Copy link
Author

mpilone commented Nov 1, 2019

Thanks for the quick fix.
It looks like event.path is non-standard and event.composedPath() is standard but not implemented everywhere. The recommended approach, similar to what you have, is to check both for compatibility:
var path = event.path || (event.composedPath && event.composedPath());

I'm wondering if you should be doing the same "hasParent" check in either the normal or shadow-dom case. For example:

  let target = manager.element;
  let srcEventTarget;
  if (srcEvent.composedPath && srcEvent.composedPath()) {
    // Use the composedPath if available so the specific element can be targeted in
    // the case of open shadow-dom/web components.
    srcEventTarget = srcEvent.composedPath()[0];
  }
  else if (srcEvent.path) {
    // Backward compatibility for browsers that haven't moved to composedPath yet.
    srcEventTarget = path[0];
  }
  else {
    // Default to the event target which may be retargeted in the case of closed shadow-dom.
    srcEventTarget = srcEvent.target;
  }

  if (hasParent(srcEventTarget, target)) {
    target = srcEventTarget;
  } 

  input.target = target;
}

That would treat normal dom element and shadow-dom element clicks the same regarding the hasParent check.

@WoodNeck
Copy link
Member

WoodNeck commented Nov 11, 2019

Hello, @mpilone! Sorry for the late release.
We were busy these days, so it took a while to merge the PR including your review, sorry for that.
I've just released a new version(2.0.16) fixing this issue, so please go ahead and check it!
Thanks for filing the issue!

@WoodNeck WoodNeck reopened this Nov 11, 2019
@mpilone
Copy link
Author

mpilone commented Nov 11, 2019

Fix looks good. I confirmed that after updating to 2.0.16 I now get the proper event targets even in shadow-dom. Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants