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

Bug 13393: .focus() results in "Unspecified Error" in IE9 #1181

Closed
wants to merge 5 commits into from

Conversation

dgalvez
Copy link
Contributor

@dgalvez dgalvez commented Feb 23, 2013

I couldn't find a better way to avoid IE9 crashing when evaluating document.activeElement. This patch sets documet.body as the activeElement in case elem.focus() is called before document.readyState !== "complete" and then redefines jQuery.event.special.focus.trigger to avoid re-running the extra check and body.focus() statements.

A test case has been added.

focus: {
            // Fire native event if possible so blur/focus sequence is correct
            trigger: function() {
                var focusTrigger = function() {
                    if ( this !== document.activeElement && this.focus ) {
                        this.focus();
                        return false;
                    }
                };

                if ( document.readyState !== "complete" ) {
                    document.body.focus();
                }

                jQuery.event.special.focus.trigger = focusTrigger;

                return focusTrigger.call( this );
            },
            delegateType: "focusin"
        }

The bug url: http://bugs.jquery.com/ticket/13393

@dgalvez dgalvez closed this Feb 23, 2013
@dgalvez dgalvez reopened this Feb 23, 2013
@dgalvez
Copy link
Contributor Author

dgalvez commented Feb 23, 2013

I didn't add a check to run this for IE9 only or a check to make sure document.body.focus is a method. What do you think?

@dmethvin
Copy link
Member

It's good to know that the problem only occurs before the document is ready. My Spidey senses tingle when it comes to actively forcing focus to anything though. If we're nested a couple of iframes deep for example, is this going to cause an issue if the outer iframe monkeys with the focus as well?

I wonder if we might be able to skip the document.activeElement check if the doc isn't ready and still get the focus to work.

I think the test should run on all browsers to make sure we didn't break them. On XML documents document.body.focus wouldn't be there but people should not be triggering events named "focus" on XML documents.

@dgalvez
Copy link
Contributor Author

dgalvez commented Feb 24, 2013

"is this going to cause an issue if the outer iframe monkeys with the focus as well?"
That's a good point Dave.

"I wonder if we might be able to skip the document.activeElement check if the doc isn't ready and still get the focus to work."
Good idea, let me try this and see if we get it working without breaking any existing test cases.

In case it's not possible to avoid the document.activeElement check I'll try to discard any outer iframe interaction with the current approach ( perhaps using this.ownerDocument.body.focus() instead and checking edge cases ).

@dmethvin
Copy link
Member

Thanks for looking at this. The focus/blur stuff tends to be really fidgety in both IE and Firefox.

Another thought, maybe we could use the flag that @gibson042 is proposing in gh-1183 if focusing the body isn't reliable. We could avoid focusing the element if the document isn't ready but still run the handlers. Your solution is better but this could be a fallback.

@dmethvin
Copy link
Member

@dgalvez, @timmywil said he'd take a look at this. It looks like we may be able to use document.documentElement.activeElement. @timmywil note that this only appears to happen before the doc is ready, in case that helps or is needed for the test case.

@timmywil
Copy link
Member

check

@dgalvez
Copy link
Contributor Author

dgalvez commented Mar 26, 2013

Great! Sorry guys, busy week :(

@dmethvin
Copy link
Member

dmethvin commented Apr 4, 2013

ping @timmywil on this ... should I close the PR or do you plan to riff off this?

@dgalvez
Copy link
Contributor Author

dgalvez commented Apr 4, 2013

Yep, I'll poke @timmywil PR closed :)

@dgalvez dgalvez closed this Apr 4, 2013
@dgalvez dgalvez reopened this Apr 6, 2013
@dgalvez
Copy link
Contributor Author

dgalvez commented Apr 6, 2013

@timmywil what about having document.documentElement.activeElement as a fallback for document.activeElement? Please take a look at my last commit. Perhaps we could redefine the focus method after the first call when doc is ready to avoid any overhead, although I don't think is needed.

Using document.documentElement.activeElement caused "focusin bubbles" tests to not pass and that's why I thought of the fallback approach.

@dmethvin reopening the PR was the way I found to ping @timmywil :)

@timmywil
Copy link
Member

timmywil commented Apr 8, 2013

@dgalvez : Thanks! This is already in the works.

@timmywil timmywil closed this Apr 8, 2013
@dgalvez
Copy link
Contributor Author

dgalvez commented Apr 8, 2013

@timmywil cool!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants