Fixed datepicker positioning calculation #2337

oojacoboo · 2025-03-14T02:53:01Z

See #2057. This still needs merging.

See jquery#2057. This still needs merging.

linux-foundation-easycla · 2025-03-14T02:53:09Z

The committers listed above are authorized under a signed CLA.

✅ login: oojacoboo / name: Jacob Thomason (3196aa0, 6a2872c)

mgol

Thanks for the PR. We'll need tests for this change, though, as I indicated in the other PR. Have a look at existing test in https://github.com/jquery/jquery-ui/tree/main/tests/unit/datepicker to see how to write one.

oojacoboo · 2025-03-14T19:18:53Z

@mgol these are unit tests. Can you please explain what about this change needs to be added or updated with unit tests? As for integration tests, which is actually the only way to add tests around this - does this lib even have those? And if so, can you please direct me to one as an example where you can test the visibility of the datepicker's location within a browser window?

mgol · 2025-03-14T21:56:40Z

We only have unit tests. While I agree some tests would better work as integration ones, most of the changes can be tested via unit tests.

I'd start with constructing a runnable example on a platform like JS Bin where the issue is obvious - this would be useful for people looking at the reasons for this patch in the future as well. Then we can think about a possible test case that would represent what we saw.

If you don't have any idea for how the unit test would look like, start with a runnable example and maybe I'll be able to propose something.

oojacoboo · 2025-03-15T01:02:27Z

@mgol How are you going to write a unit test for something that's dynamically calculated based on a browser window's scroll position?

mgol · 2025-03-15T01:16:54Z

Tests are run on real browsers, the HTML files used for tests are also defined by us. You can scroll programmatically as long as you make sure there’s enough content to have something to scroll even on large screens.

mgol · 2025-03-24T12:58:49Z

The existing spinner.html template probably doesn't have too much to scroll, but you can always dynamically attach a div with a large size to #qunit-fixture and then scroll.

If more invasive changes are needed, I added infrastructure for isolated iframe tests in #2342, but it might not be necessary here.

oojacoboo · 2025-03-26T21:10:02Z

Okay, I've added a test for the positioning with the more lazily calculated top position. The issue occurs in more complex positioning scenarios. I've added a test that creates a more complex positioning scenario to verify positioning.

mgol

Thanks for the updates. I added some comments. Please make sure lint & tests are passing for you locally before pushing changes. Right now, neither the lint nor the test you added is passing.

mgol · 2025-03-26T23:01:35Z

tests/unit/datepicker/core.js

+	$container.css({
+		position: "static",
+		top: 0,
+		left: 0,
+		height: "5000px",
+	});


Let's not change these; this is so that the fixture is not obscuring the test report view if not necessary. Also, I'm not sure if these styles are reset between runs.

If for some reason this has to be visible on the screen, please append another container to #qunit-fixture and style it.

mgol · 2025-03-26T23:03:43Z

tests/unit/datepicker/core.js

+	assert.expect( 4 );
+
+	var $container = $( "#qunit-fixture" ),
+		$modal = $container.children('div'),


Make sure lint passes. Because it fails at the lint stage, it doesn't even run tests in half of the cases.

mgol · 2025-03-26T23:05:31Z

tests/unit/datepicker/core.js

+	});
+
+	// Add an internal wrapper around all the children nodes
+	$modal.wrapInner(`<div id="relative-wrapper" style="position: relative"></div>`)


ES6+ is not currently allowed; ESLint also catches that.

mgol · 2025-03-27T10:15:10Z

tests/unit/datepicker/core.js

+	assert.expect( 4 );
+
+	var $container = $( "#qunit-fixture" ),
+		$modal = $container.children('div'),


That's a bit fragile to depend on it always being a single div. If you need a reference to this element, can you maybe start from the inp input and go to the parent?

mgol · 2025-03-27T10:17:04Z

tests/unit/datepicker/core.js

+	// Wrap inputs with a container
+	$inputOne.wrap(`<div id="input-one-container" style="height: 200px;"></div>`);
+	$inputTwo.wrap(`<div id="input-two-container" style="height: 200px;"></div>`);


This is doing the wrapping after the widget is already initialized. I think we'd rather want to prepare the intended HTML structure before initializing the widget. If you modify the structure of an already running widget, various things may not work correctly and I don't think this issue requires any of that, does it?

Also, are all those modifications really needed? If it gets too heavy, it might be easier to write a separate iframe test where you can put all the style info in a style tag. But let's first verify why all those modifications are needed. Can you explain them all?

Plus, you're wrapping two different inputs with divs with the same ID; that's not compliant.

mgol · 2025-03-27T10:17:22Z

tests/unit/datepicker/core.js

+
+	// Set the page scroll position way down the page
+	var done = assert.async();
+	$(document).ready(function() {


It shouldn't be needed to wrap any code in a test in document ready.

mgol · 2025-03-27T10:17:38Z

tests/unit/datepicker/core.js

+	assert.ok( $modal.is(":hidden"), "Modal is hidden" );
+
+	// Set the page scroll position way down the page
+	var done = assert.async();


If the test is async, please put this declaration at the top of the test, as in other tests.

mgol · 2025-03-27T10:18:23Z

tests/unit/datepicker/core.js

+		// Disable the scrollbar
+		$('body').css('overflow', 'hidden');


Is this necessary? Why? All such code modifying global elements needs to be reset manually at the end of the test or it may affect other ones.

mgol · 2025-03-27T10:18:37Z

tests/unit/datepicker/core.js

+		// Initialize datepickers
+		$modal.find('input').datepicker();
+
+		setTimeout(() => {


Why is the setTimeout needed, especially with such a long delay?

mgol · 2025-03-27T10:19:00Z

tests/unit/datepicker/core.js

+
+			$inputTwo.focus();
+			assert.ok( $("#ui-datepicker-div").is(":visible"), "Datepicker is visible" );
+			done();


Let's not call done before all the code is run, and we have some below.

oojacoboo · 2025-03-28T00:39:21Z

@mgol thanks for the feedback on this. The reality here is that I am not familiar with qunit, or this lib's unique testing setup that comes with lots of pre-build assumptions. Nor is there any documentation on testing (linting, etc). The goal of the test is to create a scenario where the datepicker is initialized in a fixed position container, but with varying alterations to the DOM that affect it's calculated top value.

There appear to be a number of race conditions within tests as well, hence the timeout and $(document).ready calls. Not to mention, this stock html template with default styling, etc.

I really don't have enough familiarity with all of this to generate a better test. I can clean this one up. But that's about it, and the time spent on this has already exceeded my goals by about 10x.

Fixed datepicker positioning calculation

6a2872c

See jquery#2057. This still needs merging.

mgol requested changes Mar 14, 2025

View reviewed changes

mgol added the Needs tests label Mar 14, 2025

Datepicker: Added test to ensure proper positioning of datepicker

3196aa0

mgol requested changes Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed datepicker positioning calculation #2337

Fixed datepicker positioning calculation #2337

oojacoboo commented Mar 14, 2025

linux-foundation-easycla bot commented Mar 14, 2025 •

edited

Loading

mgol left a comment

oojacoboo commented Mar 14, 2025

mgol commented Mar 14, 2025 •

edited

Loading

oojacoboo commented Mar 15, 2025

mgol commented Mar 15, 2025

mgol commented Mar 24, 2025

oojacoboo commented Mar 26, 2025

mgol left a comment

mgol Mar 26, 2025

mgol Mar 26, 2025

mgol Mar 26, 2025

mgol Mar 27, 2025

mgol Mar 27, 2025

mgol Mar 27, 2025

mgol Mar 27, 2025

mgol Mar 27, 2025

mgol Mar 27, 2025

mgol Mar 27, 2025

oojacoboo commented Mar 28, 2025

		// Disable the scrollbar
		$('body').css('overflow', 'hidden');

Fixed datepicker positioning calculation #2337

Are you sure you want to change the base?

Fixed datepicker positioning calculation #2337

Conversation

oojacoboo commented Mar 14, 2025

linux-foundation-easycla bot commented Mar 14, 2025 • edited Loading

mgol left a comment

Choose a reason for hiding this comment

oojacoboo commented Mar 14, 2025

mgol commented Mar 14, 2025 • edited Loading

oojacoboo commented Mar 15, 2025

mgol commented Mar 15, 2025

mgol commented Mar 24, 2025

oojacoboo commented Mar 26, 2025

mgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oojacoboo commented Mar 28, 2025

linux-foundation-easycla bot commented Mar 14, 2025 •

edited

Loading

mgol commented Mar 14, 2025 •

edited

Loading