Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes testing container layout issues for QUnit 2.14 #817

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

drewlee
Copy link
Contributor

@drewlee drewlee commented Feb 12, 2021

This change attempts to address the numerous issues that have been introduced by the CSS positional updates in QUnit 2.14.0, including #793 and #801. Besides visual layout problems, the relative percentage-based dimensions introduced in PR #786 have been found to cause random test failures in existing applications, especially those which assert visibility states. The modifications did not consider that the ember testing container resides in qunit's fixture for v5. These have also been found to cause ember-a11y-testing failures with false-positives/negatives.

Given that the QUnit layout is now position: fixed based, it is no longer possible for the testing container to fall directly below the test results. As such, the container now also requires fixed positioning in relation to the viewport, similar to the "docked container" layout.

To follow the convention applied in https://github.com/qunitjs/qunit/pull/1513/files, the proposed fixes ensure that the UI reverts to the old layouts if the browser doesn't support CSS flex display, or the viewport height is constricted below the 500px threshold. Percentage based dimensions have been reverted back to fixed pixels to prevent testing container jank and false-positive/negative failures mentioned above. I've iterated over a bunch of different ways of remediating these problems, and believe this is probably the most balanced approach between usability and not having to override a bunch of QUnit's styling.

For the default layout, the proposed fix pins the testing container to the bottom of the viewport and centered, essentially dividing the screen into two horizontal panes with some semblance of the old layout.

Default view:
image
Test results scrolled to bottom:
image

Viewport below 500px height (or lack of flex support) reverts back to old layout.
Top:
image
Bottom:
image

The "docked container" option pretty much remains similar and pins the testing container to the bottom right side of the screen.

Default view:
image

Test results scrolled to bottom:
image

Height constriction (or lack of flex support) reverts to old layout
image
image

Dev mode:
Screen Shot 2021-02-12 at 11 24 24 AM

Notable items:

  • Since the percentage-based dimensions have been removed, the fixes applied in PR fix: ensure "Development Mode" is full screen #804 are no longer necessary as "full screen" mode works as expected.
  • These changes require a new qualifier to differentiate between standard layout and "docked container" layout, hence the addition of the docked-container CSS class to the body.
  • Theoretically, these changes are backportable to v.4.0.x.
  • To simplify things, we may want to consider making the "docked container" layout the new default and remove it as a selectable option from the UI altogether.
  • These changes still require some testing in various browser permutations, and also verification in real applications. But, I think we're in a good enough state to open it up for discussion.

@drewlee drewlee marked this pull request as draft February 12, 2021 19:33
@drewlee
Copy link
Contributor Author

drewlee commented Feb 12, 2021

Marking this as a draft for cross-browser and app testing.

@scalvert
Copy link
Contributor

Wow thanks for doing this, @drewlee. Really appreciate you looking into fixing this.

@drewlee
Copy link
Contributor Author

drewlee commented Feb 16, 2021

Updated PR with the following changes:

  • Removed scripted relative position styling in favor of a CSS default. This makes it easier to override with fixed positioning for the new layout without having to resort to using !important declarations.
  • Removed scripted marginBottom styling in favor of a CSS default. Again, this makes it easier to override without resorting to !important declarations.
  • Added scripted marginBottom override when the container is hidden. Otherwise, the full page isn't used to show test results and looks odd.
  • Updated styling for bottom margin on test results for docked container layout. The initial implementation didn't work with Firefox on Win/Mac.

Visually verified fixes on Safari, Chrome (Win/Mac), Firefox (Win/Mac), and Edge (Win/Mac).
Ran fixes in several large-scale apps. All related tests pass as expected.

Example layout with hidden container:
Screen Shot 2021-02-16 at 5 21 34 PM

@drewlee drewlee marked this pull request as ready for review February 16, 2021 23:23
@drewlee
Copy link
Contributor Author

drewlee commented Feb 17, 2021

I created a minimal reproduction case demonstrating the visibility-related false-negatives at https://github.com/drewlee/ember-qunit-layout-bugs/. Yarn linking to this set of fixes allows the test to pass as expected.

@rwjblue
Copy link
Member

rwjblue commented Feb 17, 2021

To simplify things, we may want to consider making the "docked container" layout the new default and remove it as a selectable option from the UI altogether.

Agree. How much more work is this to include here?

Copy link
Contributor

@scalvert scalvert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me. Once @rwjblue's question is sorted I think this is GTG.

@drewlee
Copy link
Contributor Author

drewlee commented Feb 17, 2021

@rwjblue should be fairly straightforward, would just need to remove and streamline some of the CSS/JS. I can probably push out an update by EOD.

@drewlee
Copy link
Contributor Author

drewlee commented Feb 18, 2021

Makes "docked container" layout the default, which effectively means that:

  1. The testing container is always in fixed position, so it's no longer necessary to modify position property via script
  2. The "Dock container" option is removed from the UI
  3. Bottom margin for test results set in the CSS instead of via scripting
  4. The overall implementation is greatly simplified as scripted checks are no longer necessary to conditionally apply styling based on layout type

I've found it necessary to apply a small right gutter for the testing container so that it doesn't occlude the test results scroll bar. This is done automatically by the browser when the layout reverts to the previous version (below 500px height).

Scrolled top:
Screen Shot 2021-02-17 at 6 06 55 PM
Scrolled bottom:
Screen Shot 2021-02-17 at 6 06 59 PM
Under 500px height:
Screen Shot 2021-02-17 at 6 07 14 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants