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

IE Modal error #750

Closed
mclarentgp opened this issue Oct 25, 2016 · 16 comments · Fixed by #823
Closed

IE Modal error #750

mclarentgp opened this issue Oct 25, 2016 · 16 comments · Fixed by #823

Comments

@mclarentgp
Copy link
Contributor

I'm receiving the following error when I close the modal window through a button click event in IE.

"Unable to get property 'contains' of undefined or null reference"

I do not receive any error if i close the modal by clicking outside of it though.
I'm using the latest version of semantic-ui-react and my version of IE is Version 11.0.9600.18500

@levithomason
Copy link
Member

We use Node.contains() in a couple places. We may need to replace this with a util. Can you paste the full error output including any stack trace?

@mclarentgp
Copy link
Contributor Author

I'm not seeing any stack trace on my end and the most i can see if that were its erroring out in IE is the specific line,
// If event happened in the portal, ignore it

  if (_this.portal.contains(e.target)) return;

in portal.js

@mclarentgp
Copy link
Contributor Author

I turned the debugger on in the console for the modal and see that the error occurs after this.

semanticUIReact:modal handlePortalUnmount()

@levithomason
Copy link
Member

levithomason commented Oct 26, 2016

Thanks much, this is enough for us to fix it 👍.

Whoever picks this up, we need to remove use of Node.contains(). Instead, let's add this util:

/**
 * Returns true if a root node contains another node.
 * @param {DOMElement} root The root that is expected to contain the node
 * @param {DOMElement} node The node to look for in root
 * @returns {boolean} Whether or not root contains the node
 */
export const rootContainsNode(root, node) {
  // traverse node parents, if we reach the root then the node was in root
  while (node) {
    if (node === root) return true
    node = node.parentNode
  }

  return false
}

@jeffcarbs
Copy link
Member

jeffcarbs commented Oct 27, 2016

"Unable to get property 'contains' of undefined or null reference"

@levithomason - I think the issue raised here is that this.portal is undefined, not that contains isn't a function.

The issue may be with firstElementChild although it seems to be supported on all modern browsers. It's used to set the portal here: https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/addons/Portal/Portal.js#L333

FWIW Node.contains has been supported in IE dating back to v5 😮 : https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

@levithomason
Copy link
Member

Thanks, I looked right over this in the OP: ...property 'contains' of undefined....

From a quick scan around the web, it looks like the fix may be as simple as:

-this.portal = this.node.firstElementChild
+this.portal = this.node.children[0]

@layershifter
Copy link
Member

I'll test it on windows PC today, if last comment solves problem, I'll make PR.

@layershifter
Copy link
Member

Docs has problems with running in IE 11, I'm reviewing them, but main problem is not in firstElementChild - it's supported from IE9. Looks like problem with unmounting, but I haven't enough time today to find problem.

image

@levithomason
Copy link
Member

Looks like problem with unmounting

The OP noted this error Unable to get property 'contains' of undefined or null reference. Which problem are you referring to as I do not see any errors on your console in the screenshot provided?

@layershifter
Copy link
Member

It's output of console.log(this.portal) after line this.portal = this.node.firstElementChild.


I've added some debug to Portal.handleDocumentClick.

+ debug('handleDocumentClick(): start')
// If event happened in the portal, ignore it
if (this.portal.contains(e.target)) return

And recieved this in IE:
image

While in Chrome:
image


If we look on IE debug, it becomes clear that problem is in event calling, not in firstElementChild. There is this.portal = null in unmountPortal, so fail on contains() is resonable, this.portal is null.

So, main question there is why IE calls handleDocumentClick when it was removed in unmountPortal 😕

@levithomason
Copy link
Member

Ah, I see. I may have to drag out the old windows box at some point to debug. It would also be good to get some tests going in multiple browsers / platforms. I'll add these to my bucket list, won't be soon though!

@layershifter
Copy link
Member

@levithomason As temporary fix we can make this change:

+ // Temporary fix for IE
+ if(!this.portal) return

// If event happened in the portal, ignore it
if (this.portal.contains(e.target)) return

@levithomason
Copy link
Member

I've made some updates and added your suggestion, see #823.

@layershifter
Copy link
Member

I'll pull branch with fix and test it IE today.

@EwanValentine
Copy link

Modals aren't working for me in I.E. 11, it shows the dimmed background, then no actual modal.

@levithomason
Copy link
Member

Please open a new issue and include as much information as possible. There is a template shown when you create an issue.

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

Successfully merging a pull request may close this issue.

5 participants