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

[JW8-2595] Return in getElementWindow() if element is null #3280

Merged
merged 3 commits into from Feb 8, 2019

Conversation

Projects
None yet
5 participants
@waxidiotic
Copy link
Member

waxidiotic commented Feb 6, 2019

JW8-2595

This PR will...

Return window if getElementWindow(element) (in ui.js) if element is falsy.

Why is this Pull Request needed?

  • In IE 11 and Edge, getElementWindow(element) was being called when element === null.
  • This appears to be caused by when IE11 and Edge fire pointer events compared to other browsers. In IE 11/Edge, this.destroy() in dismissButton.js is setting the element to null before interactEndHandler is bring run.
  • This caused a JavaScript exception in those environments.
  • Functionality was not affected but we should avoid JavaScript exceptions when possible.
  • The stack trace went back to minified jQuery code so I couldn't find the exact cause.

Are there any points in the code the reviewer needs to double check?

n/a

Are there any Pull Requests open in other repos which need to be merged with this?

n/a

Addresses Issue(s):

JW8-2595

@waxidiotic waxidiotic added this to the 8.8.0 milestone Feb 6, 2019

@waxidiotic waxidiotic requested review from robwalch and karimMourra Feb 6, 2019

@johnBartos

This comment has been minimized.

Copy link
Member

johnBartos commented Feb 6, 2019

Warnings
⚠️

🛠 There are modified src files, but no test changes. Add tests if you're able to.

Generated by 🚫 dangerJS

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Feb 6, 2019

MULTI Build for commit 59daed3 passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@waxidiotic waxidiotic requested review from pajong and vseventer and removed request for karimMourra Feb 6, 2019

@robwalch
Copy link
Member

robwalch left a comment

getElementWindow should still return window when the element is undefined.

You should really be figuring out what is calling it with undefined - probably core-shim originalContainer... or a destroyed UI instance.

@@ -348,6 +348,10 @@ const eventRegisters = {
};

export function getElementWindow(element) {
if (!element) {
return;

This comment has been minimized.

@robwalch

robwalch Feb 7, 2019

Member

If this does not return anything then there will be another exception with the code that uses the undefined result. Please see what where we are in the debug code after this point, and don't use the harness - use the events page (no jQuery).

Once you've finished investigating, return window here.

This comment has been minimized.

@waxidiotic

waxidiotic Feb 7, 2019

Author Member

@robwalch Thanks for the suggestion on using the event page. I was able to pinpoint the issue to being when IE11 and Edge fire pointer events. In those browsers, the destroy() method in dismissButton.js (in commercial) is setting the element to null before the interactEndHandler can run. So by the time it does run, the element is null.

I updated to make sure ui.js returns window.

This comment has been minimized.

@robwalch

robwalch Feb 7, 2019

Member

@waxidiotic I moved it to interactEndHandler so it's a little less code and execution.

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Feb 7, 2019

MULTI Build for commit f20fafe passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Feb 7, 2019

MULTI Build for commit 50d0a92 passed.
🏗 jwplayer build SUCCESS
🏗 jwplayer browserstack tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@robwalch robwalch merged commit eafe0c3 into master Feb 8, 2019

4 checks passed

Danger ⚠️ Danger found some issues. Don't worry, everything is fixable.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jw7-pr-multi-opensource Build finished.
Details

@robwalch robwalch removed the needs-review label Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment