-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Avoid “Unspecified Error” in MS Edge when accessing `document.activeE… #4393
Conversation
@@ -58,17 +58,15 @@ export function contains(parent, child) { | |||
} while (child = child.parentNode) | |||
} | |||
|
|||
export let activeElt = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the function forking because it seemed like overkill.
// Older versions of IE and sometimes Edge throws unspecified error when touching | ||
// document.activeElement in some cases (during loading, in iframe) | ||
try { | ||
let activeElement = document.activeElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it still work when you only wrap the first access to document.activeElement
in a try
? Non-trivial code wrapped in error-swallowing try/catch scares me, since when someone introduces a bug there, we probably won't notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check it out. What's interesting is the old fallback for IE9 didn't have the loop logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marijnh
Do you know if any of the documents are created with DOMParser
or the result of an XHR?
I'm triaging the bug from the MS Edge side and trying to narrow down the Unspecified Error
which is a fallback error for when things go totally wonky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marijnh
Sorry for more q's, but when is there a root
property on an element?
Is this something CodeMirror adds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop is to 'break out' of shadow DOMs (which old IE's don't support, hence it was missing from the other implementation)
Ok I found the issue. By whatwg spec However, in Edge in an iframe, if Update: |
…lement` from inside an iframe.
Thanks! Merged as 69669e4.
Could it be that these are actually the same case? It was observed 'during loading' before, but most likely that was just because no body was available. |
They could be similar. |
This PR avoids a tricky “Unspecified Error” in MS Edge 14 when accessing
document.activeElement
from inside an iframe.