Draggable: Modified to account resizable handles size in document containment mode, used by ui.dialog. Fixed #9521 ui.Dialog: Resizable dialogs move/resize out of viewport boundary - which results in scrollbar in window

[dekajp]

Draggable: Modified to account resizable handles size in document containment mode, used by ui.dialog. Fixed #9521 ui.Dialog: Resizable dialogs move/resize out of viewport boundary - which results in scrollbar in window

[mikesherov]

@dekajp, thanks for contributing! There are several issues with the patch that need addressing:

1. The unit tests belong in draggable, not dialog, because that's where the bug is being fixed.
2. You're attaching a few unnecessary properties to the instance... See comments inline.
3. You are using unnecessary bit wise logic to determine a truthy value for .attached, which is itself an unnecessary var... See comments inline.

[mikesherov]

This property is unnecessary. You're only using it in one spot, and only to avoid subtracting zero from a number.

[mikesherov]

I'm going to close this pull request for now. @dekajp , I encourage you to submit a new one with corrections if you'd like. Thanks again!

[dekajp]

@mikesherov Thanks for all your comments , i will make the changes and resubmit code.

[scottgonzalez]

@dekajp If you send a new PR, please take a new approach. Draggable cannot directly query for resizable-related code. This is a generic problem that needs to be solved.

[dekajp]

@scottgonzalez , if draggable can not directly query resizable-handles , so what other way you suggest ? i assumed it is safe to query the resizable handles.

[dekajp]

@scottgonzalez ,

1. To query whether element is resizable - is to query whether the handles are attached to it. by query .ui-resizable-xx css class . Is this ok ? are you looking for other approach .
2. _calculateResizableHandleOverflow which will calculate the excess width and height by querying the resizable handles needs to go in some common interface . this can be possible by either moving this code to base class or a common util class . what will you prefer ?

[dekajp]

@scottgonzalez , further looking into code in core class , i think we can query resizable similar to tabbable and focusable

[scottgonzalez]

The resizable handles are causing a problem, but the problem is not that they are resizable handles. You need to address the cause, not the symptom.

[dekajp]

@scottgonzalez if i understand correctly since resizable handles are causing issue we can change css style on resizable handles , so that they will not overflow the parent element boundary .

[scottgonzalez]

Modifying the resizable CSS is fine, though you probably want to modify the dialog CSS instead, since that's where the problem actually exists.

[dekajp]

@scottgonzalez ,i am going to look at css both dialog and resizable after work . I re-read several times your above comment You need to address the cause, not the symptom to understand it - what you actually meant. I am glad i am getting close in approach .

[dekajp]

@scottgonzalez , I could replicate the issue with [Dragger + Resizable] and [Dialog + Dragger + Resizable]

But by overriding the styles - ( actually overriding the positions of resizable handles ) the problem can be fixed. Let me know if this approach looks good to you.

http://jsfiddle.net/dekajp/N22vY/

[scottgonzalez]

It's really hard to tell what's changing by looking at a fiddle, but I'm assuming you just changed the left/top for the dialog's SE handle. That's fine, as long as we don't regress on http://bugs.jqueryui.com/ticket/4575.

[dekajp]

just want to let you know i am looking at this http://bugs.jqueryui.com/ticket/4575 (http://jsfiddle.net/bchiasson/Lfx8J/5/) and the fiddle in it i can see the SE image . updated fiddle to latest version of jquery-ui 1.10.2 i can not see the SE image in (Chrome 29, Opera 16 and FF 23 ) in MacOS http://jsfiddle.net/Lfx8J/24/ . i am not sure if this is a defect or expected behavior. can you please confirm .

[scottgonzalez]

You shouldn't see it. It was intentionally removed.

[dekajp]

New Pull request - #1092