Adding support for containment #1988

fglueck · 2021-09-29T08:08:01Z

No description provided.

linux-foundation-easycla · 2021-09-29T08:08:04Z

The committers are authorized under a signed CLA.

✅ Frank Glück (73dd781)

mgol · 2021-10-11T17:38:47Z

Can you post an online demo of usage of this new property?

fglueck · 2021-10-25T19:43:13Z

I use it to keep a jquery-ui-dialog inside a div (containment). It works very fine, but currently I don't have a sample/demo page for this.

mgol · 2021-10-25T21:35:32Z

@fglueck All I need is a small test case of a dialog working inside of such a container using a version of jQuery UI with your patch. I want to manually verify it looks & works fine before merging the PR and I don't have time to set it up myself at the moment.

We would also need a test case added in this PR to make sure we don't regress.

fglueck · 2021-10-28T08:35:04Z

You can try to use on every page that have some <div>'s, if you want to open a dialog that keeps inside one special div:

$('<div>test</div>').dialog({containment:'#content'});

In this case it was <div id="content">

https://jsfiddle.net/fglueck/qhnremof/1/

mgol · 2021-12-01T13:43:10Z

@fglueck Thanks, the example looks good. However, we still need test cases (for both when the option is provided and when it's missing). Tests should be added to https://github.com/jquery/jquery-ui/blob/main/tests/unit/draggable/options.js.

mgol · 2021-12-01T13:43:46Z

If you can add a test case, I'd be in favor of merging this. @fnagel what do you think?

fglueck · 2021-12-01T16:28:09Z

Dear mgol, I never do this before. Can you go to hand?

QUnit.test( "{ containment: Element }", function( assert ) {
	assert.expect( 1 );
	var content = $( '<div id="xxx"></div>' ).css( {
			height: '50px',border: '0px', margin:'0px', width:'50px', position: 'absolute', bottom: '0px', right: '0px'
		} ).appendTo( "body" ),
		element = $('<div>X/div>').dialog({containment: content, position: { my: "left top", at: "left top", of: content}}), // open on left top corner
		offsetAfter,
		expected = content.offset();
console.log(content.offset(), element.offset();
		testHelper.drag( element, '.ui-dialog-titlebar', -200, -200);  // try to move out
	    offsetAfter = element.offset(); // should be the same 
console.log(content.offset(), element.offset());	
	assert.deepEqual( offsetAfter, expected, "compare offset );
} );

When I try this, there is a change between the offsets and I don't know why? :-(

_{EDIT by @mgol: I wrapped the code in code blocks for easier reading}

mgol · 2021-12-01T20:47:18Z

@fglueck It's not just in tests, you can see that in your JSFiddle. If you try to drag the dialog, it immediately moves inside of the content element. This suggests more source changes may be needed to achieve the goal.

BTW, what's your goal here apart from the DOM attachment? What do you expect in the case of your Fiddle? Should the dialog show up in the middle of the screen or in the middle of the container?

fnagel · 2021-12-02T22:11:08Z

@fglueck What exactly are you trying to achive here? Why not just use the appendTo option?

https://api.jqueryui.com/dialog/#option-appendTo
https://jsfiddle.net/vyw2egoc/

fglueck · 2021-12-06T08:11:50Z

@fnagel: you meen the goal of the "test" or the goal for my "patch"?
I try your https://jsfiddle.net/vyw2egoc/ doesn't do the same then containment, in your fiddle I can move outside the div.

The patch works fine an works since the last months on different sites very well. But I never written a test.

QUnit.test( "{ containment: Element }", function( assert ) {
	assert.expect( 1 );
	var content = $( '<div id="xxx"></div>' ).css( {
			height: '300px',border: '0px', margin:'0px', width:'300px', position: 'absolute', bottom: '0px', right: '0px'
		} ).appendTo( "body" ),
		element = $('<div>X/div>').dialog({containment: content, position: { my: "left top", at: "left top", of: content}}), // open on left top corner
		offsetAfter,
		expected = content.offset();
console.log(content.offset(), element.offset());
		testHelper.drag( element, '.ui-dialog-titlebar', -200, -200);  // try to move out
	    offsetAfter = element.offset(); // should be the same 
console.log(content.offset(), element.offset()); // position differ a little, same then "content" div	
		testHelper.drag( element, '.ui-dialog-titlebar', -200, -200);  // try to move out
console.log(content.offset(), element.offset()); // position same as above that GOOD!
	assert.deepEqual( offsetAfter, expected, "compare offset" );
} );

If you look in the console output, you can see that the outer div "content" also change the position.
First log top:510, after moving the "element", top:501.
The difference between start and end position of element is also -9px.

So the patch works fine, I think I do something wrong in this test.

fglueck · 2021-12-06T11:08:06Z

I found my error I check the position of the dialog content, not the dialog it self:

`QUnit.test( "Dialog can't break out containment", function( assert ) {
assert.expect( 1 );
var box = $( '

' ).css( {
height: '500px',border: '0px, margin:'0px', width:'500px', position: 'absolute', bottom: '0px', right: '0px', left: '50px', top: '50px'
} ).appendTo( "body" ),
element = $('

X

').dialog({containment: box, position: { at: "left top", of: box}}), // open on left top corner
dlg = element.dialog('widget'),
offsetAfter,
expected = box.offset();

// console.log(box, box.offset(), box.position(), dlg, dlg.offset(), dlg.position());
testHelper.drag( element, '.ui-dialog-titlebar', -200, -200); // try to move out
offsetAfter = dlg.offset(); // should be the same
assert.deepEqual( offsetAfter, expected, "compare offset" );
} );`

fnagel · 2021-12-06T17:03:11Z

@fglueck "move outside the div" was the missing part. I was not quite sure what you are trying to achieve here in general :-)

If you try to drag the dialog, it immediately moves inside of the content element.

It should ONLY move inside the red bordered div, right? So in general this looks right to me, but the initial position is wrong.

@mgol Not sure if we really want to add new features. More code means more bugs and we don't have much resources anyway. Docs and demos are needed too.

mgol · 2021-12-06T17:20:11Z

@mgol Not sure if we really want to add new features. More code means more bugs and we don't have much resources anyway. Docs and demos are needed too.

@fnagel Yeah, that's true. I initially misinterpreted this as something having a possibly high performance impact but that's already what the appendTo option is for.

mcanepa · 2021-12-07T01:44:58Z

If I'm not mistaken, what @fglueck is trying to achieve is having a dialog constrained to a certain div while dragging, and that div will be set via options (default is set to "document").

To achieve that behaviour, I currently do this

$("#div_dialog").dialog({
  title: "hello"
}).dialog("widget").draggable("option", "containment", "#some_div");

So having the requested feature is a +1 for me because it will be more clean. It will be as simple as

$("#div_dialog").dialog({
    title: "hello"
    containment: "#some_div"
});

In my opinion, it does't add much complexity, it's just adding an option to what is now harcoded

adding cleaning up

fglueck · 2021-12-07T08:02:36Z

Documentation is added with
"Adding support for containment (#1988) #340" an
api.jqueryui.com

mgol · 2021-12-17T13:32:03Z

Without committing to whether we'll merge this yet (it's a new API and we're trying to be cautious about these now), I'd like to point out that there's still a major issue that containment is only enforced while dragging and initially the dialog may appear out of the container, as can even be seen by the fiddle that @fglueck provided. I think that's a blocker issue.

mgol · 2022-01-01T12:11:26Z

After discussing this with Felix I can say we’re open to merging this feature despite generally trying to avoid adding new features as long as all our concerns are addressed, including my last comment here. That also includes docs; I saw there’s an API PR, I can review it if this PR turns out to be ready to land.

LifeIsStrange · 2022-08-20T11:46:33Z

@fglueck any update? I'd like this to be merged

fixed GitHub Actions / Grunt based tests with Node.js 14.x

fglueck · 2022-08-25T07:44:34Z

Please excuse that you had to wait so long

LifeIsStrange · 2022-08-25T08:06:28Z

@fglueck don't worry I did not have to wait and this is volunteer work :)
I just thought this would be a nice optimization.

mgol · 2022-10-05T12:34:50Z

FYI, I think my concerns haven't been addressed so this is still blocked.

mcanepa · 2022-10-05T13:49:19Z

@mgol so we should mimic this behaviour?

$("#test").dialog({
	containment: "#content",
	appendTo: "#content", 
	position: { my: "center", at: "center", of: "#content"}
});

https://jsfiddle.net/5w9rt270/

The dialog should be placed at center of what is set in containment option? (obviously this should be handled backstage, in the fiddle I've just set the appendTo and position options...)

If that's the case, may be the new contaiment option shoud default to body, so appentTo defaults to what is set in containment and position.of defaults to what is set in appendTo?

mgol · 2022-10-05T14:12:19Z

The dialog should be placed at center of what is set in containment option?

Yes, I think so.

In your fiddle, that's still not happening, though. The dialog gets attached at the top left of the page and only moves into the container when it gets moved.

mgol

Marking as "Request changes" until my remarks are addressed.

mcanepa · 2022-10-05T15:07:51Z

In your fiddle, that's still not happening, though

Indeed, but I guess it has to do with something about the iframe of the fiddle. If you test this html you will see it works as expected (no containment option)

<!DOCTYPE html>
<html lang="en">
	<head>
		<meta charset="UTF-8">
		<meta http-equiv="X-UA-Compatible" content="IE=edge">
		<meta name="viewport" content="width=device-width, initial-scale=1.0">
		<title>Document</title>
		<link rel="stylesheet" href="https://code.jquery.com/ui/1.13.2/themes/base/jquery-ui.css">
		<script src="https://code.jquery.com/jquery-3.6.1.min.js"></script>
		<script src="https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"></script>
		<script>
			$(document).ready(function()
			{
				$("#dialog").dialog({
					appendTo: "#content",
					position: { of: "#content"}
				});
			});

		</script>
	</head>
	<body>
		<div id="header" style="height:150px;border:1px solid blue;">header</div>
		<div id="content" style="height:600px;border:1px solid red;">body</div>
		<div id="dialog">dialog box</div>
	</body>
</html>

mgol · 2022-10-05T15:26:52Z

Oh, I see, the large JS snippet influces the timing in a bad way. Then yes, I think your defaults make sense.

Apart from applying proper defaults we would need to have tests for these defaults - both with & without the containment options, with different appendTo values, with overrides (to make sure they apply); all the things that could potentially touch this area.

Adding support for containment

73dd781

fnagel added Comp: Dialog Feature labels Oct 18, 2021

Unittest for jquery#1988

b0aafb4

fglueck added 2 commits December 7, 2021 08:24

Update options.js

6df435a

Update options.js

dd03747

adding cleaning up

fglueck added 3 commits January 10, 2022 11:14

Update options.js

335a3cc

Update options.js

a38631f

Update options.js

a5751c6

Adding support for containment

6c67052

fixed GitHub Actions / Grunt based tests with Node.js 14.x

mgol requested changes Oct 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for containment #1988

Adding support for containment #1988

fglueck commented Sep 29, 2021

linux-foundation-easycla bot commented Sep 29, 2021 •

edited

mgol commented Oct 11, 2021

fglueck commented Oct 25, 2021

mgol commented Oct 25, 2021

fglueck commented Oct 28, 2021 •

edited

mgol commented Dec 1, 2021

mgol commented Dec 1, 2021

fglueck commented Dec 1, 2021 •

edited by mgol

mgol commented Dec 1, 2021

fnagel commented Dec 2, 2021

fglueck commented Dec 6, 2021 •

edited

fglueck commented Dec 6, 2021

fnagel commented Dec 6, 2021

mgol commented Dec 6, 2021

mcanepa commented Dec 7, 2021

fglueck commented Dec 7, 2021

mgol commented Dec 17, 2021

mgol commented Jan 1, 2022

LifeIsStrange commented Aug 20, 2022

fglueck commented Aug 25, 2022

LifeIsStrange commented Aug 25, 2022

mgol commented Oct 5, 2022

mcanepa commented Oct 5, 2022

mgol commented Oct 5, 2022

mgol left a comment

mcanepa commented Oct 5, 2022

mgol commented Oct 5, 2022

Adding support for containment #1988

Are you sure you want to change the base?

Adding support for containment #1988

Conversation

fglueck commented Sep 29, 2021

linux-foundation-easycla bot commented Sep 29, 2021 • edited

mgol commented Oct 11, 2021

fglueck commented Oct 25, 2021

mgol commented Oct 25, 2021

fglueck commented Oct 28, 2021 • edited

mgol commented Dec 1, 2021

mgol commented Dec 1, 2021

fglueck commented Dec 1, 2021 • edited by mgol

mgol commented Dec 1, 2021

fnagel commented Dec 2, 2021

fglueck commented Dec 6, 2021 • edited

fglueck commented Dec 6, 2021

fnagel commented Dec 6, 2021

mgol commented Dec 6, 2021

mcanepa commented Dec 7, 2021

fglueck commented Dec 7, 2021

mgol commented Dec 17, 2021

mgol commented Jan 1, 2022

LifeIsStrange commented Aug 20, 2022

fglueck commented Aug 25, 2022

LifeIsStrange commented Aug 25, 2022

mgol commented Oct 5, 2022

mcanepa commented Oct 5, 2022

mgol commented Oct 5, 2022

mgol left a comment

Choose a reason for hiding this comment

mcanepa commented Oct 5, 2022

mgol commented Oct 5, 2022

linux-foundation-easycla bot commented Sep 29, 2021 •

edited

fglueck commented Oct 28, 2021 •

edited

fglueck commented Dec 1, 2021 •

edited by mgol

fglueck commented Dec 6, 2021 •

edited