Skip to main content

Search and Top Navigation

#8331 closed bug (notabug)

Opened May 17, 2012 08:39PM UTC

Closed May 17, 2012 09:18PM UTC

Last modified May 22, 2012 04:12PM UTC

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

Attachments (0)
Change History (28)

Changed May 17, 2012 08:47PM UTC by scottgonzalez comment:1

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.

Changed May 17, 2012 08:49PM UTC by scottgonzalez comment:2

owner: → gabriel.sobrinho
status: newpending

Why is this causing problems for you?

Changed May 17, 2012 08:50PM UTC by gabriel.sobrinho comment:3

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.

Changed May 17, 2012 08:52PM UTC by scottgonzalez comment:4

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.

Changed May 17, 2012 08:53PM UTC by gabriel.sobrinho comment:5

status: pendingnew

I see

Changed May 17, 2012 09:00PM UTC by scottgonzalez comment:6

status: newpending

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

Changed May 17, 2012 09:07PM UTC by gabriel.sobrinho comment:7

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?

Changed May 17, 2012 09:18PM UTC by scottgonzalez comment:8

resolution: → invalid
status: newclosed

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

Changed May 17, 2012 10:02PM UTC by gabriel.sobrinho comment:9

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?

Changed May 17, 2012 10:06PM UTC by scottgonzalez comment:10

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

Changed May 17, 2012 10:11PM UTC by gabriel.sobrinho comment:11

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?

Changed May 17, 2012 10:19PM UTC by scottgonzalez comment:12

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.

Changed May 17, 2012 10:26PM UTC by gabriel.sobrinho comment:13

Scott,

Are you talking about #8330?

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

Changed May 17, 2012 10:31PM UTC by scottgonzalez comment:14

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.

Changed May 17, 2012 10:44PM UTC by gabriel.sobrinho comment:15

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?

Changed May 17, 2012 10:48PM UTC by scottgonzalez comment:16

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.

Changed May 17, 2012 11:30PM UTC by gabriel.sobrinho comment:17

Scott,

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

Changed May 17, 2012 11:43PM UTC by scottgonzalez comment:18

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.

Changed May 17, 2012 11:55PM UTC by gabriel.sobrinho comment:19

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.

Changed May 18, 2012 12:14AM UTC by scottgonzalez comment:20

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

Changed May 18, 2012 03:11PM UTC by gabriel.sobrinho comment:21

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 :)

Changed May 18, 2012 03:16PM UTC by scottgonzalez comment:22

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.

Changed May 18, 2012 04:19PM UTC by gabriel.sobrinho comment:23

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.

Changed May 18, 2012 05:06PM UTC by scottgonzalez comment:24

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

Changed May 18, 2012 05:12PM UTC by gabriel.sobrinho comment:25

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.

Changed May 18, 2012 05:43PM UTC by scottgonzalez comment:26

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/

Changed May 18, 2012 09:16PM UTC by gabriel.sobrinho comment:27

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.

Changed May 22, 2012 04:12PM UTC by gabriel.sobrinho comment:28

Scott,

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

Thanks!