HTML Reporter since 2.14.0 doesn't add "flowing" elements to the page

smcclure15 commented 3 years ago

As a result of #1513 in release 2.14.0, we are having trouble interacting with our own "elements under test" that are added to the document body, as these are no longer being shown.

Here is my HTML runner page, pointing to QUnit 2.13.0 CDN resources:

<html>
<head>
    <title>My Title</title>
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/qunit/2.13.0/qunit.css">
    <script src="https://cdnjs.cloudflare.com/ajax/libs/qunit/2.13.0/qunit.js"></script>
    <script src="demo.js"></script>
</head>
<body>
    <div id="qunit"></div>
    <div id="qunit-fixture"></div>
</body>
</html>

and this is the demo.js contents:

QUnit.test('test', assert => {
  let button = document.createElement('button');
  button.style.width = button.style.height = '100px';
  document.body.appendChild(button);

  let rect = button.getBoundingClientRect();
  let center = [rect.left + rect.width/2, rect.top + rect.height/2];
  let target = document.elementFromPoint(...center);

  assert.strictEqual(target, button);
})

With QUnit 2.13.0, this passes - the "target" element at the position of the added button element is indeed itself.

With QUnit 2.14.0, this fails, since the "target" that it finds is actually the QUnit banner.

We have a large testbed that exercises UI gestures (click, type, drag, etc) on various elements/widgets, and we need a place to put them into the page (and not shown here, but we diligently cleanup to avoid drool). With the fixed header, the tools can actually click the banner (instead of the button under test) which re-triggers the test run in a sort of infinite loop.

I tried to use the qunit-fixture div as my safe "sandbox" area, but that doesn't work either (nor did it before).

I'm looking for guidance on potential "test fixes" (a more robust way to write such tests), although admittedly that could mean a lot of cleanup for us. Otherwise I wonder if we can negotiate on the fixed headers - I really do like the improved UX, but the specific solution ended up making this upgrade (from 2.11.2, FWIW) an issue for our CI workflows.

Krinkle commented 3 years ago

@smcclure15

Yikes, there definitely needs to be a safe area for this indeed. I have actually ended up changing this in my qunit3-theme work in progress (source), which might make this less likely to break.

However, in trying out your example I found that it seems to rely on there being few enough tests that the space under the test results are within the browser's current size viewport. Is that the case for real, or was that simplified away in your example? In particular, it seems that if there are a dozen or so test cases before this one, it fails on QUnit 2.13 as well as the button falls below the fold. (And even with my theme fix, this variation remains failing.)

Repro: https://codepen.io/Krinkle/pen/PoWgjYa?editors=1010

Krinkle commented 3 years ago

At Wikimedia, we have similar tests and do use the fixture for this so as to have a reliable starting position, front-most z-index, and built-in reset. I can confirm, however, that merely appending it to the fixture, still fails your test case as the qunit-fixture is by default positioned negative 10,000 pixels off-screen.

Our tests, though, use more direct approach to trigger specific behaviours without positioned mouse eventing (e.g. we simulate the events directly; and even that is rare, we mostly use WebdriverIO for end-to-end cases, acting on a CI-local instance of the application itself), which works even when the content is off-screen.

I was going to recommend to use an intermediary element to host your elements under test which you could give the relevant styling needed, but in trying so I realized this wouldn't work well since absolute top/left 0 within our fixture is still off-screen. And while QUnit does reset attributes, it's also not so straight-forward to set fixture.className = "myapp-fixture"; with custom styles since the #qunit-fixture styles would continue to weigh heavier. The only thing I can think of for now would be to undo the strong styles in your test suite.

Example: https://codepen.io/Krinkle/pen/oNBOwZo?editors=0010

smcclure15 commented 3 years ago

It's true that as-written, the repro steps would fail with enough tests above it, or with a very short window; this is accounted for (but was left out of the example) via button.scrollIntoView() sort of calls.

Spot-checking other test failures looks like some positioning tests failed (specifying a width/height/margin but its calculated value not matching), some "resize" tests failed (comparing computed width/height before and after a resize gesture on divs or table columns), or mouseover events didn't fire as expected (because I guess it was "behind" the QUnit runner decorations).

And thanks for the codepen - I think that's likely the most viable option given our scale.

Krinkle commented 3 years ago

For the record: I'm leaving this open because I do think it's a suboptimal layout that I'd like to improve. But yeah, in terms of "support", I'd say using an element that's explicitly known to remain in view would be more stable, possibly using the built-in fixture while at it, if that helps.

Krinkle commented 3 years ago

@smcclure15

The fix I had in mind (from my qunit3-theme branch) is to give the header position: sticky and let the rest of the page just be normal without using any overflow, flexbox, min-height, or otherwise. It'd be like it was before 2.14, but the header would be sticky. That seems quite simple and unobtrusive, and yet, is visually the same as what we have today.

I tried to apply just that part of it to today's main branch, but alas, I forgot that today our header elements are rather split up. Thus there isn't really a way to use position-sticky since that would apply to only one element at the top at any given time (e.g. either the title, or pixel banner, or the toolbar, or the testresult line). This could be worked around with absolute positions, but that gets rather messy and backfires a bit given the toolbar can wrap over multiple lines at narrower viewports.

Perhaps it's best to leave this as-is for now and let it naturally resolve when the new theme is ready. Is this still blocking your upgrade, or did the workaround suffice for now?

smcclure15 commented 3 years ago

That all sounds great, thanks for digging deep on this. I made a one-liner CSS change in our extension and the masses of tests looked good again. There were a few tests I changed by-hand since they were verifying padding/margins and were dropping widgets into the body node, but they should've been using the #qunit-fixture all along to isolate themselves.

We extend a few areas of QUnit with custom plugins and whatnot, but this was the one area where I had to intentionally undo something from QUnit's core. Not a great long-term thing to have to maintain, but I'm certainly at ease hanging on to it for a bit knowing that a solution should be right around the corner with 3.0; no worries there.

Bottom line, it was a little alarming given our scale, but all is good now. Again, thanks for pouncing on this and offering the interim solution.

qunitjs / qunit

HTML Reporter since 2.14.0 doesn't add "flowing" elements to the page #1603