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

hasfocus won't set focus if element is hidden #355

Open
mbest opened this issue Mar 2, 2012 · 19 comments
Open

hasfocus won't set focus if element is hidden #355

mbest opened this issue Mar 2, 2012 · 19 comments

Comments

@mbest
Copy link
Member

mbest commented Mar 2, 2012

Demonstration: http://jsfiddle.net/mbest/NTKcU/

Input 1 is in a template that is hidden while it's parsed (visible after if).

Input 2 is in a template that is visible while it's parsed (visible before if).

The button switches between them. The input will be focused for input 2 but not for input 1.

Scenario for input 1 is useful to avoid flashing, especially in Firefox, when the template includes control-flow.

@SteveSanderson
Copy link
Contributor

Good point - I'll update the hasfocus binding docs to clarify this. In general, hasfocus is only going to work when it is applied to visible elements (invisible elements can't have focus).

@nayato
Copy link

nayato commented May 3, 2012

in IE6 setting focus to invisible element actually causes error. Would it be possible to skip an attempt to set focus to an invisible element?

@mbest
Copy link
Member Author

mbest commented May 3, 2012

Would it be possible to skip an attempt to set focus to an invisible element?

My customized version of hasfocus does this:

update: function(element, valueAccessor) {
    var value = ko.utils.unwrapObservable(valueAccessor());
    setTimeout(function() {
        if (value
            && element.offsetWidth && element.offsetHeight
            && document.activeElement && document.activeElement != element)
        {
            element.focus();
            ko.utils.triggerEvent(element, "focusin"); // For IE, which doesn't reliably fire "focus" or "blur" events synchronously
        }
    });
}

It also will only set the focus and not clear it, and it checks if the element is already in focus first.

@nayato
Copy link

nayato commented May 15, 2012

mbest,
I've tried your version and it still throws an error for me even in IE8.

@mbest
Copy link
Member Author

mbest commented May 15, 2012

Here's the example using my revised update function: http://jsfiddle.net/mbest/tAGmp/

Try it out.

@nayato
Copy link

nayato commented May 16, 2012

Your example works fine - no question about that. I'll try to shorten mine into a fiddle soon-ish

@SteveSanderson
Copy link
Contributor

nayato - are you saying there is some other situation we should handle?

I guess we do want to avoid throwing actual errors while trying to focus elements. Perhaps we could look at what jQuery does to handle this situation (maybe just try-catch?).

@nayato
Copy link

nayato commented May 18, 2012

jQuery docs say:
"Attempting to set focus to a hidden element causes an error in Internet Explorer. Take care to only use .focus() on elements that are visible."
Ironic :)

mbest, I've found the culprit in my case: adding " && !element.disabled" to condition in your version of hasfocus does the trick.

Here's my markup (it's text field with watermark):

<div class="wrap">
    <div class="watermark" data-bind="click: focusAddress, visible: !(addressFocused() || address()), text: watermark">&nbsp;</div>
    <input data-bind="value: address, valueUpdate: 'afterkeydown', hasfocus: addressFocused, disable: $root.busy" />
</div>

@mbest
Copy link
Member Author

mbest commented May 18, 2012

maybe just try-catch?

IE gives an error even if you use try/catch.

@nayato
Copy link

nayato commented May 18, 2012

It didn't throw in my case with this:

try {
    value ? element.focus() : element.blur();
    ko.utils.triggerEvent(element, value ? "focusin" : "focusout");
} catch (ex) { }

@mbest
Copy link
Member Author

mbest commented May 18, 2012

It didn't throw in my case with this

Okay. I'll check it out again.

@mbest
Copy link
Member Author

mbest commented May 19, 2012

So try/catch does work to prevent the error. So that could be a useful change as well. Of course that doesn't solve the problem I've reported in this issue, that "hasfocus won't set focus if element is hidden." Solving that requires using setTimeout to set the focus if the element is hidden.

@nayato
Copy link

nayato commented May 19, 2012

mbest, I'd prefer your approach with no try-catch as it a) works just as well with IE b) does a bit more c) I just feel uncomfortable having try-catch for no good reason

@SteveSanderson
Copy link
Contributor

Hmm, if jQuery is unable to avoid throwing errors in the case of attempting to focus a hidden element, perhaps that suggests it's more trouble than it's worth.

I'd be especially keen to avoid any use of setTimeout, because as soon as you make things asynchronous they stop being deterministic. We can't currently even write specs for async things without using a different testing framework. Quite a lot of KO design decisions were taken specifically to keep changes synchronous, because that makes application code a lot easier to reason about and much less prone to unexpected interactions.

I would give this a low priority unless an extremely neat solution occurs to anyone.

@mbest
Copy link
Member Author

mbest commented May 22, 2012

There's a big difference between jQuery and Knockout. jQuery is generally used imperatively, whereas Knockout is mostly declaratively. Also, I think Knockout is at a higher level (less low level) than jQuery in terms of DOM manipulation. When doing DOM stuff with jQuery, you could just have your code check if the element is visible first. With Knockout, the binding should do all the DOM stuff for you, and all you should have to think about is your bindings and model.

@mbest
Copy link
Member Author

mbest commented Jul 25, 2012

I was taking a look at the jQuery code (ver 1.7) and found the following in trigger

// IE<9 dies on focus/blur to hidden element (#1486)
if ( ... ((type !== "focus" && type !== "blur") || event.target.offsetWidth !== 0) ... ) {

@lcorneliussen
Copy link
Contributor

A simple fix could be to make it configurable, if the binding also should blur the element on update - maybe the intersection use case is close to zero...?

(might be a bit out of context here - but i also read #352 before...)

@lcorneliussen
Copy link
Contributor

By just doing the focus() in a setTimeout whenever the element is hidden, fixes the problem (I'm not sure if this could have race condition problems, though..)

Have a look here:
http://jsfiddle.net/mhrf2/

@lcorneliussen
Copy link
Contributor

Just upgraded my fork and found out, that this is still current. #671 works ok for me - but it uses a setTimeout...

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

No branches or pull requests

5 participants