Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8331 closed bug (notabug)

Datepicker: do not double focus input

Reported by: gabriel.sobrinho Owned by: gabriel.sobrinho
Priority: minor Milestone: 1.9.0
Component: ui.datepicker Version: 1.8.20
Keywords: Cc:
Blocked by: Blocking:

Description

Hi,

These lines https://github.com/jquery/jquery-ui/blob/653673ed64176645128782038e0ee99c05514b92/ui/jquery.ui.datepicker.js#L706-707 are causing an issue on phantom.js and/or poltergeist, take a look here: https://github.com/jonleighton/poltergeist/issues/60

I removed these lines on my project and didn't see any side effect since the date picker opens on focus event.

Any specific reason to these lines be there?

Note that line 685 currently checks if the date picker is not inline: https://github.com/jquery/jquery-ui/blob/653673ed64176645128782038e0ee99c05514b92/ui/jquery.ui.datepicker.js#L685

Change History (28)

comment:1 Changed 7 years ago by Scott González

Version: git1.8.20

That moves focus to the input field in case the datepicker is shown programmatically.

Please stop filing bugs with a version of git; that is for code that has not been released.

comment:2 Changed 7 years ago by Scott González

Owner: set to gabriel.sobrinho
Status: newpending

Why is this causing problems for you?

comment:3 Changed 7 years ago by gabriel.sobrinho

Status: pendingnew

Scott,

I'm using the currently git version, that's the reason why I'm using the git version to report the bugs.

If it's not intended to report bugs, you should remove that from tracker and tell users about that.

I this warning already exists, I didn't see, sorry.

comment:4 Changed 7 years ago by Scott González

Status: newpending

It's there for users to report bugs against code that is broken in git, but not in a release. This is not the case for the stuff you're reporting.

comment:5 Changed 7 years ago by gabriel.sobrinho

Status: pendingnew

I see

comment:6 Changed 7 years ago by Scott González

Status: newpending

Can you please answer the question of why this is causing problems for you?

comment:7 Changed 7 years ago by gabriel.sobrinho

Status: pendingnew

Scott,

This is causing another issue related to poltergeist and phantom.js

I don't know why this causes the issue because I'm not a C++ developer to debug that inside the phantom.js but reason is call to focus inside intercepted focus event.

Browsers may deal with that (instead of being a infinite loop) but I guess we should not call the intercepted event itself again.

What do you think?

comment:8 Changed 7 years ago by Scott González

Resolution: invalid
Status: newclosed

Sounds like a bug in a UA that we don't support.

comment:9 Changed 7 years ago by gabriel.sobrinho

Scott,

I would to suggest something, take a look: https://gist.github.com/2721846

This will not break anything and solves this issue.

What you think?

comment:10 Changed 7 years ago by Scott González

Yes, that will solve this one specific case, but the input may still get many focus events per "focus".

comment:11 Changed 7 years ago by gabriel.sobrinho

Yep, but currently who's buggy is jQuery Datepicker.

Currently, user callbacks is called twice when date picker input receives focus.

Using jsFiddle like suggested: http://jsfiddle.net/aCgJh/

That could be considered a date picker issue?

comment:12 Changed 7 years ago by Scott González

A long time ago I would've said yes, but as I've said over and over, there's nothing we can do about that. You'll get focus events for anything you do that removes focus when we want to maintain focus. I'll accept a patch that prevents this, but I can't consider native events being triggered when they're not expected or not being triggered when they are expected as bugs because it's just impossible to build this type of advanced functionality without changing the native behavior.

comment:13 Changed 7 years ago by gabriel.sobrinho

Scott,

Are you talking about #8330?

If so, these problems are not related to each other.

comment:14 Changed 7 years ago by Scott González

No, I'm talking about focus. You will get many focus events per interaction. Just try clicking anywhere on the calendar that won't cause a date selection.

comment:15 Changed 7 years ago by gabriel.sobrinho

Scott,

Nothing happens, take a look on fiddle: http://jsfiddle.net/aCgJh/1/

The focus event is only triggered when user clicks on month navigation that not makes any sense, by the way.

Anyway, this is an another issue on date picker, not related to this one.

Would you accept a pull request covering my proposal on gist?

comment:16 Changed 7 years ago by Scott González

I don't know how you can say "nothing happens" and follow it up with "the focus event is only triggered when user clicks on month navigation."

Yes, I will accept a check against document.activeElement, I already said that.

comment:17 Changed 7 years ago by gabriel.sobrinho

Scott,

Talking about focus on month navigation, what's the reason for that?

comment:18 Changed 7 years ago by Scott González

This is the last time I'll explain this: Clicking on the navigation controls moves focus, therefore we move focus back to the text field. We can prevent this by calling event.preventDefault() in mousedown, but that won't work in IE.

comment:19 Changed 7 years ago by gabriel.sobrinho

I already understand it, isn't necessary to repeat.

The question is why return the focus to the input since it already didn't happen if user click on date picker except for month navigation or date, that's what I don't understand the reason.

The only case that makes sense to return the focus is clicking in a specific date, not in month navigation.

Maybe I'm not being clear in my questions.

comment:20 Changed 7 years ago by Scott González

It should always go back to the text field, it's the element that manages all key handling.

comment:21 Changed 7 years ago by gabriel.sobrinho

Scott,

Could you take a look?

https://github.com/sobrinho/jquery-ui/commit/b30c863fb16a4e4a146ea5868cc02e0508368d8d

I guess it's fine but I want to be sure before submitting a pull request :)

comment:22 Changed 7 years ago by Scott González

No, that's not ok. You should not be defining a focused selector; the implementation is not even correct as it doesn't handle cross-window references. Just do the check inline.

comment:23 Changed 7 years ago by gabriel.sobrinho

Scott,

What you mean with cross-window references? frames?

Take a look about activeElement here: https://developer.mozilla.org/en/DOM/document.activeElement

Seems like it's present in all supported browsers by jQuery UI.

comment:24 Changed 7 years ago by Scott González

Frames, iframes, tabs, whatever. There are plenty of ways to create multiple windows. This has nothing to do with cross-browser support.

comment:25 Changed 7 years ago by gabriel.sobrinho

Scott,

Sorry, I do not get yet.

What window.activeElement (the :focused selector) has to do with tabs and frames?

window.activeElement "Returns the currently focused element, that is, the element that will get keystroke events if the user types any." (reference: https://developer.mozilla.org/en/DOM/document.activeElement)

It's not a big deal to check that inline on date picker but this could be useful for others widgets and others users of jQuery UI if it get documented.

comment:26 Changed 7 years ago by Scott González

1) It's document.activeElement not window.activeElement

2) elem === document.activeElement breaks cross-window references between elem may not exist in document, so the check may not be valid.

3) A proper focus selector already exists in Sizzle.

4) There's already documentation: http://api.jquery.com/focus-selector/

comment:27 Changed 7 years ago by gabriel.sobrinho

Scott,

1) Sorry, typo.

2) If element no longer exists in document, it can't receive focus. Am I wrong?

3 and 4) Take a look: https://github.com/sobrinho/jquery-ui/commit/b7ba15784256d4b5977012c7a7c918ddb3a296d2

I don't know the reason but using is(:focus) didn't worked.

comment:28 Changed 7 years ago by gabriel.sobrinho

Scott,

I'm removing the proposal from github because seems like you aren't interested in this fix.

Thanks!

Note: See TracTickets for help on using tickets.