Fixed datepicker positioning calculation

[oojacoboo]

See #2057. This still needs merging.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: oojacoboo / name: Jacob Thomason (3196aa08e731058d3c7f76c5100b05d04bf3e989, 6a2872c7c675161e0d7d2877e591068765c9de16)

[oojacoboo]

@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]

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]

@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]

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]

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 https://github.com/jquery/jquery-ui/pull/2342, but it might not be necessary here.

[oojacoboo]

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.

[oojacoboo]

@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.

[mgol]

I fully understand you may not have time to finish this PR. I don't have time either (see the project status at https://blog.jqueryui.com/2021/10/jquery-maintainers-update-and-transition-jquery-ui-as-part-of-overall-modernization-efforts/); beyond code reviews, most of my time on this project is spent on fixing regressions from 1.12.1 or newer; the rest is mostly left to the community.

This will just stay unfixed until someone has time to get this to the finish line. It's possible I'll have a closer look one day but I cannot promise this will happen.

[juleslagarde]

This patch resolves an issue that only appeared on certain computers. It's strange that it's computer-dependent.