-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Core: Add methods to work around IE active element bugs #1478
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
Conversation
|
||
// Support: IE9 - 10 only | ||
// If the <body> is blurred, IE will switch windows, see #9420 | ||
if ( element && element.nodeName.toLowerCase() !== "body" ) { |
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.
In mobile we also make sure that we avoid blurring the window. In fact, the code for retrieving the window and the document from the element is copied from the widget factory :)
https://github.com/jquery/jquery-mobile/pull/7927/files#diff-103715cf0e3db43f218c7235fd2a18fbR213
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 saw that, it seemed really whacky. When are you ever passing a window?
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 believe the reason for that is that if you ever pass event.target
or the context of an event handler you may end up passing in the window. I don't believe document.activeElement
can ever be set to the window. I guess the basic assumption was that $( window ).blur()
is unsafe in the same way as $( "body" ).blur()
in that it results in the window losing focus. However, I've now tested $( window ).blur()
as far back as IE6 and it does not seem to do anything, so I guess attempting to blur the window is safe.
I used the following test:
<!doctype html>
<html>
<head>
<script src="http://code.jquery.com/jquery-1.11.1.js"></script>
<script>
setTimeout( function() {
$( "#console" ).text( "blurring window\n" );
$( window ).blur();
}, 5000 );
</script>
</head>
<body>
<pre><code id="console"></code></pre>
<input>
</body>
</html>
and I tried both with focus initially set to default and with focus immediately set on the input by my clicking on it.
You always pass
Let's add source comments for that. |
I wanted these methods to be as close to the intended APIs as possible. In the real APIs, we're always using DOM elements.
This will be used outside of widgets in jQuery Mobile. I should note that there are references to |
Done. |
I just looked at the jQuery Mobile PR, but I didn't see any calls to the equivalent helpers outside of widgets. Both |
I'm not sure, I was going by what @arschmitz said. I can say, though, that if we had already split UI Core into multiple files, I would have used this in Effects Core. |
Sorry the page container call used to be in navigation it was moved I forgot. The effects use seems valid though. |
We should've done this a long time ago. 👍 |
Actually, it might be simpler to override core's |
Sorry, I just wanted to throw this in there, because I don't have the time right now to actually look in more detail, but I noticed that focus and blur are implemented in core as .trigger( "focus" ) and .trigger( "blur" ) and those events, in turn, are handled in $.event.special, so they are exposed to being overridden. Also, core already has a private method safeActiveElement() which does the try {} catch(){} thing, so we need not duplicate that if we go the $.event.special route. |
We could override https://github.com/jquery/jquery/blob/master/src/event.js#L558-L576 and add the body-check (facepalm) into blur. |
Overriding Overriding |
@scottgonzalez That makes sense. |
Discussed with @tjvantoll and @arschmitz in IRC. These methods will not be documented and will not be supported; they're only for internal use.
jquery-archive/jquery-mobile#7927 should be updated if/when this lands.