Skip to content

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

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
simonihmig:fix-contains-ie11
Dec 6, 2018
Merged

[BUGFIX beta] fix mouseEnter/Leave event delegation w/o jQuery for SVG & IE11#17227
rwjblue merged 1 commit intoemberjs:masterfrom
simonihmig:fix-contains-ie11

Conversation

@simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Nov 24, 2018

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

Fixes #17225

@simonihmig simonihmig mentioned this pull request Nov 24, 2018
Closed
@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

simonihmig commented Nov 26, 2018

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...

rwjblue
rwjblue reviewed Nov 26, 2018
View reviewed changes
packages/@ember/-internals/views/lib/system/event_dispatcher.js Outdated
(!related || (related !== target && !target.contains(related)))
(!related ||
(related !== target &&
!(target.contains ? target.contains(related) : contains(target, related))))
Copy link
Member

@rwjblue rwjblue Nov 26, 2018

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

@simonihmig simonihmig Nov 26, 2018

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

@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.

Done!

packages/@ember/-internals/views/lib/system/event_dispatcher.js Outdated
target &&
target.nodeType === 1 &&
(!related || (related !== target && !target.contains(related)))
(!related ||
Copy link
Member

@rwjblue rwjblue Nov 26, 2018

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 •
edited
Loading

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

simonihmig commented Nov 30, 2018

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
@simonihmig
Update old jQuery versions for tests
e0c16f6 
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 simonihmig mentioned this pull request Dec 6, 2018
Merged
@simonihmig
Copy link
Contributor Author

simonihmig commented Dec 6, 2018

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.

@simonihmig
[BUGFIX beta] fix mouseEnter/Leave event delegation w/o jQuery for SV…
62d6096 
…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

simonihmig commented Dec 6, 2018

@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
@simonihmig @kategengler
Update old jQuery versions for tests
4f0223e 
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

Reviewers

@rwjblue rwjblue rwjblue left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@simonihmig @rwjblue