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

[BUGFIX beta] fix mouseEnter/Leave event delegation w/o jQuery for SVG & IE11 #17227

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

simonihmig
Copy link
Contributor

IE11 does not support Node#contains() on SVG elements, so in that case a custom fallback implementation is used.

Fixes #17225

@rwjblue
Copy link
Member

rwjblue commented Nov 26, 2018

Had one failure under the /tests/?jquery=1.8.3 test run...

Test failed: EventDispatcher:  delegated event listeners work for mouseEnter on SVG elements", source:  (49)
[1124/013147.014267:INFO:CONSOLE(52)] "    Failed assertion: mouseenter event was triggered, expected: 1, but was: 0
    at Object.@test delegated event listeners work for mouseEnter on SVG elements (http://localhost:13141/ember-tests.js:19171:14)
    at Object.<anonymous> (http://localhost:13141/ember-tests.js:76563:37)
    at runTest (http://localhost:13141/qunit/qunit.js:1478:30)
    at Test.run (http://localhost:13141/qunit/qunit.js:1464:6)
    at http://localhost:13141/qunit/qunit.js:1670:12
    at advance (http://localhost:13141/qunit/qunit.js:1116:26)", source:  (52)
[1124/013147.014915:INFO:CONSOLE(52)] "    Failed assertion: Died on test #2     at generateTest (http://localhost:13141/ember-tests.js:76562:15)
    at Set.forEach (<anonymous>)
    at moduleFor (http://localhost:13141/ember-tests.js:76546:16)
    at http://localhost:13141/ember-tests.js:18961:27
    at internalRequire (http://localhost:13141/ember.debug.js:62:14)
    at Object.requireModule [as require] (http://localhost:13141/ember.debug.js:99:14)
    at http://localhost:13141/tests/?jquery=1.8.3&nolint=true:242:61: Cannot read property 'target' of undefined
TypeError: Cannot read property 'target' of undefined
    at Object.@test delegated event listeners work for mouseEnter on SVG elements (http://localhost:13141/ember-tests.js:19172:49)
    at Object.<anonymous> (http://localhost:13141/ember-tests.js:76563:37)
    at runTest (http://localhost:13141/qunit/qunit.js:1478:30)
    at Test.run (http://localhost:13141/qunit/qunit.js:1464:6)
    at http://localhost:13141/qunit/qunit.js:1670:12
    at advance (http://localhost:13141/qunit/qunit.js:1116:26)", source:  (52)

@simonihmig
Copy link
Contributor Author

Hm, ok, I'll check this. Although I did not touch the jQuery-based implementation, so the added test seems to have uncovered this!?

@rwjblue
Copy link
Member

rwjblue commented Nov 26, 2018

@simonihmig - Yeah, looks like it...

(!related || (related !== target && !target.contains(related)))
(!related ||
(related !== target &&
!(target.contains ? target.contains(related) : contains(target, related))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vaguely prefer to always use contains(target, related) and tweak the contains util to deal with the ternary, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@@ -335,7 +336,9 @@ export default EmberObject.extend({
while (
target &&
target.nodeType === 1 &&
(!related || (related !== target && !target.contains(related)))
(!related ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this isn't new, but I'm curious why are we checking !related here? Can we tweak this to avoid boolean coercion? What do we expect target.relatedTarget to be? I was thinking it could only be either undefined or null, if thats correct we could (possibly in a different PR?) tweak to related === undefined && related === null...

Copy link
Contributor Author

@simonihmig simonihmig Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the specs the uninitialized value must be null, so I changed this to a strict null check.

@simonihmig
Copy link
Contributor Author

Had one failure under the /tests/?jquery=1.8.3 test run...

Not sure why this is happening, but the root cause seems to be with jQuery. With /tests/?jquery=1.10.0 the test is passing again!

How should we deal with that? Skip the test when using this older version of jquery? Or can we update the test suite's old jQuery version, like using the latest 1.x?

@rwjblue
Copy link
Member

rwjblue commented Dec 6, 2018

How should we deal with that?

Lets change our oldest tested jQuery to 1.10.0. Mind updating for that?

simonihmig added a commit to simonihmig/ember.js that referenced this pull request Dec 6, 2018
Updates the suite of tests for old jQuery versions to run with latest of
1.10.x, 1.12.x and 2.2.x.

Unlocks emberjs#17227 which is failing due to jQuery 1.8 presumably not handling
events on SVGs correctly.
@simonihmig
Copy link
Contributor Author

Lets change our oldest tested jQuery to 1.10.0. Mind updating for that?

@rwjblue done, in this separate PR: #17268

I will rebase this and fix the linting error once the other PR has been merged.

@rwjblue
Copy link
Member

rwjblue commented Dec 6, 2018

Perfect, #17268 is landed. Ready for rebase / linting fix now.

…G elements in IE11

IE11 does not support `Node#contains()` on SVG elements, so in that case a custom fallback implementation is used

Fixes emberjs#17225
@simonihmig
Copy link
Contributor Author

@rwjblue done, green! :)

@rwjblue rwjblue merged commit bbf003a into emberjs:master Dec 6, 2018
@rwjblue
Copy link
Member

rwjblue commented Dec 6, 2018

Thank you very much @simonihmig!

kategengler pushed a commit that referenced this pull request Dec 6, 2018
Updates the suite of tests for old jQuery versions to run with latest of
1.10.x, 1.12.x and 2.2.x.

Unlocks #17227 which is failing due to jQuery 1.8 presumably not handling
events on SVGs correctly.

(cherry picked from commit e0c16f6)
@simonihmig simonihmig deleted the fix-contains-ie11 branch December 26, 2018 22:57
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.

2 participants