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

fix(event-manager): Firefox shadow dom event handling #766

Merged
merged 3 commits into from
Nov 23, 2019

Conversation

michaelw85
Copy link
Contributor

Return correct original event target for Firefox

Closes #765

Return correct original event target for Firefox

Closes aurelia#765
@michaelw85
Copy link
Contributor Author

michaelw85 commented Nov 12, 2019

@bigopon @fkleuver Followup for #755 #758

@fkleuver
Copy link
Member

@bigopon @michaelw85 Do we have any particular tests for this? In v2 I did this a little bit differently, starting with the more specific APIs first and falling back to the more generic ones. I seem to recall discussion that there was something wrong about v2's event handling but not sure if it applied to this.
Should this code be changed to be in sync with v2? https://github.com/aurelia/aurelia/blob/master/packages/runtime-html/src/observation/event-manager.ts#L16
Or should it be the other way around you think? @bigopon
Either way I'd prefer if we kept v1 and v2 the same in this particular regard.

@michaelw85
Copy link
Contributor Author

michaelw85 commented Nov 13, 2019

@fkleuver I tested the fix locally in my project (created a branch with updated dist). The method is private and I did not think any tests would run in Firefox so I did not write a unit test. I could of course quite easily provide a simple component that could be used to verify the fix (if we run tests in Firefox).

On the approach, I would prefer the v2 setup. This seems correct to me, I thought the composedPath is the "new" standardized api for getting the target info. I was thinking about this approach with my change but decided to minimize impact. If @bigopon agrees I can update the PR to match v2

@bigopon
Copy link
Member

bigopon commented Nov 13, 2019

If path and composedPath are not the same in chrome, then we cannot move composedPath in front of path, unless we have a way to configure it in the event manager

@michaelw85
Copy link
Contributor Author

path should be the same, apparently this was the initial proposal by Google and this became composedPath(). composedPath is the standard now.

A quick test in my app shows they are identical in this scenario:
image

@bigopon
Copy link
Member

bigopon commented Nov 13, 2019

@michaelw85 thanks for checking, If so let's go with @fkleuver 's suggestion and make it behave like v2

@michaelw85
Copy link
Contributor Author

michaelw85 commented Nov 13, 2019

ok will update.

@fkleuver I just spotted a mistake in the v2 code. event.deepPath()[0] deepPath is an array not a method.
Documentation: https://developer.mozilla.org/en-US/docs/Web/API/Event

Event.deepPath 
An Array of DOM Nodes through which the event has bubbled.

n.v.m. it was wrong in v1 and the docs I quoted. Based on the original change it's a method: https://codereview.chromium.org/2014173002/#ps100001

@michaelw85
Copy link
Contributor Author

@bigopon @fkleuver Can this PR be reviewed or are you waiting for me to do any additional changes?

@fkleuver
Copy link
Member

I think we're good. Thank you!

@EisenbergEffect Unless you see any issue, as far as I'm concerned we can merge/release this. It's now in sync with v2.

@EisenbergEffect EisenbergEffect merged commit 9bd53fc into aurelia:master Nov 23, 2019
@EisenbergEffect
Copy link
Contributor

Thanks everyone! We'll get this out next week.

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 this pull request may close these issues.

Delegate does not support shadow dom for Firefox
4 participants