Opened 4 years ago

Last modified 3 years ago

#9491 open bug

DatePicker does not use the correct ownerDocument (does not support multiple windows)

Reported by: lukepage Owned by:
Priority: minor Milestone: none
Component: ui.datepicker Version: 1.10.3
Keywords: rewrite Cc:
Blocked by: Blocking:

Description

This issue came up here

http://bugs.jqueryui.com/ticket/4908

It was however closed as saying most issues are fixed. However the datepicker control frequently decides to request a specific id (e.g. $(id)) without specifying a document. This makes the datepicker unusable on a 2nd window.

I need this fixed and will happily provide a patch if I can get some feedback on the correct fix for this.

Change History (7)

comment:1 Changed 4 years ago by lukepage

so.. here https://github.com/jquery/jquery-ui/blob/master/ui/jquery.ui.datepicker.js#L1575 why does it capture the id instead of inst? is it to stop memory leaks ( a reference from the function capture to the element). One fix would be to pass the ownerDocument from e.target.ownerDocument in each of the event handles. what do people think about that?

comment:2 Changed 4 years ago by scottgonzalez

  • Status changed from new to open

I only looked at a few of the methods where id is being passed around, but it looks like they all just do $(id) right away. So it should be safe to just pass the DOMElement instead of the id. You'll need to check if id is being used anywhere else in those functions though.

When #6228 is fixed this problem will just naturally go away. But if you send a PR fixing all of the queries now, we'd accept that.

comment:3 follow-up: Changed 4 years ago by lukepage

comment:4 in reply to: ↑ 3 Changed 4 years ago by tj.vantoll

Replying to lukepage:

patch added here https://github.com/jquery/jquery-ui/pull/1052

@lukepage Could you please create a test case that shows this issue. I made one here http://jsfiddle.net/tj_vantoll/5za9s/ and it looks like your patch isn't handling for that.

Thanks.

comment:5 Changed 4 years ago by lukepage

yes 2 issues here.. one was you were referencing the https version so it wasn't included.

The 2nd was that I tested it inside my application which wraps jquery ui a bit and appends it to the right thing.. jqueryUI appends the datepicker to the current window rather than the window with the control in it. I will have a look to see if I can patch that bit as well.

comment:6 Changed 4 years ago by lukepage

It looks like a much bigger patch to fix the issue completely rather than patch the small area that is effecting my usage of the datepicker. Will have to think whether it is worth patching this or contributing a fix for #6228

comment:7 Changed 3 years ago by scottgonzalez

  • Keywords rewrite added
Note: See TracTickets for help on using tickets.