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

Update dateinput.js #999

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

diegolamonica
Copy link

The test against "msie" UA is weak because most of the browsers can change their UA string so, if a browser is identified as msie but it supports focus method it will fail.
Maybe in the future versions of IE the focus method would work. In this case we are ready too (however I hope that one of the next IE will support date input natively :-) ).

The test against "msie" UA is weak because most of the browsers can change their UA string so, if a browser is identified as msie but it supports focus method it will fail. 
Maybe in the future versions of IE the focus method would work. In this case we are ready too (however I hope that one of the next IE will support date input natively :-) ).
@alibby251
Copy link
Contributor

Hi diegolamonica,

Thanks for this - I've been giving this some more thought:

It is true that IE is the only one that doesn't support .focus() properly, although I am hearing rumours it was fixed for IE9? I have done some more reading, which suggests that using .triggerHandler("focus"), instead of .focus() might be a better option, at least until we can get rid of support for older IE browsers! ;-) Have a look at http://api.jquery.com/triggerHandler/ and see what you think - it should just be a direct replacement that is required...?

P.S. Could I please ask if you could keep comments to a minimum when submitting patches? It should just be sufficient to put a single comment pointing to the URL of the issue log, otherwise code will become very bloated! Thanks.

@diegolamonica
Copy link
Author

Maybe I'm misunderstanding something... But reading the triggerHandler documentation and deeping into the example it seems that would be better to use .trigger("focus") than .triggerHandler("focus").
Just reporting what documentation says: The .triggerHandler() method does not cause the default behavior of an event to occur (such as a form submission).. In our case the default behavior is the element focus and considering the previous source code you did focus if other browsers else do nothing.
However i think that delegating native focus function (when it's possible) is better, so I suggest this change further:

if (e.type == "click" && input.focus) { // element has focus method? 
    try{ // IE<9 dies focus on hidden elements
        input.focus();
    }catch(e){
        // Intercepting the exception if UA does not support 
        // the focus and firing other related events.
        input.trigger("focus");
    }
}

what do you think about?

About comments, you're right, as you can state from this message I'm too much verbose. I will try to reduce noises in the next commit! 🔨

@alibby251
Copy link
Contributor

Good point - I suspect I wasn't too keen on simply not applying any action at all, when the browser being used is less than IE9; it feels better that we've at least got two options, rather than 1 option, and nothing...

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

Successfully merging this pull request may close these issues.

None yet

2 participants