emberjs / ember-test-helpers

Test-framework-agnostic helpers for testing Ember.js applications
Apache License 2.0
188 stars 254 forks source link

Add reseting `ember-testing-container` scroll position during render() #729

Open danDanV1 opened 4 years ago

danDanV1 commented 4 years ago

Expectation: When you use render() the DOM should be in a clean state and there shouldn't be any leftover DOM state of scrollbar positions.

Result: if you are scrolling elements into view during tests, and then doing subsequent clearRender() and render() within the same test, the scroll position of ember-testing-container is not reset.

Would you be open to a pull request for adding the following into the render() or clearRender() helpers? getRootElement().parentElement.scrollTo(0, 0);

Since clearRender() is mostly concerned with triggering teardown behaviour, my vote would be to add a call to reset scroll in render() on line 107.

https://github.com/emberjs/ember-test-helpers/blob/5c4f7025cf530273b7fc240adec04a5a24113c57/addon-test-support/%40ember/test-helpers/setup-rendering-context.ts#L107

rwjblue commented 4 years ago

Hmm. Doesn't it naturally scroll back to the top when all the content is removed?

danDanV1 commented 4 years ago

That is correct. It does not scroll <div id="ember-testing-container"> back to the top when content is removed.

I have made a reproduction with a test showing scroll position fails to reset. https://github.com/edeis53/ember-testing-container-scroll-reproduction

Let me know if the test is clear. I am clearing the render and doing a second render within the same test. It's cleaner to illustrate that way.

The issue also exists if the second render was within a separate test. So if you have TestA which does some scrolling, and then TestB that requires an element to be in the viewport, TestB will fail if TestA runs before it because the scrollposition of the container is not guaranteed.

rwjblue commented 4 years ago

The example test was very helpful thank you! I think a PR to add a reset mechanism seems totally good.

danDanV1 commented 4 years ago

I am preparing the tests for the PR, but there is an issue with CSS used in the tests viatesting-support.css .

As of at least 3.7 Ember apps/addons are using "ember-qunit": "^3.4.1", but ember-test-helpers which is on 3.8.2 is using "qunit": "^2.9.2",

Ember-qunit adds some extra CSS, in particular, constrains the testing container.

#ember-testing-container {
...
  width: 640px;
  height: 384px;
  overflow: auto;
...
}

So currently, it's not possible test the scroll issue properly because there's no constraints on the testing container and without the constraints from ember-qunit it's document.documentElement that is scrolling, which is always going to scroll if you have lots of tests and should keep the scroll position between tests.

May I upgrade from qunit to Ember-qunit in my PR as well?

rwjblue commented 4 years ago

May I upgrade from qunit to Ember-qunit in my PR as well?

No, this library can't depend on ember-qunit (since ember-qunit depends on us), but we can add the same styles that we expect ember-qunit and ember-mocha to have added.