Eager DOM creation for module filter triggers delay in test execution #1664
Assignees
Labels
Milestone
Comments
|
Thanks for the report. I think within the QUnit 2.x series, in the interest of compatibility, I'd recommend the second approach:
I'll also take a look this weekend to reproduce the issue in Chrome. There's a very small feeling that Chrome may be misattributing this, which I'd like to rule out, but the improvement would be welcome either way. If confirmed, I'd also like to report it to upstream Chromium if you haven't already. Thanks again! |
Krinkle
added a commit
that referenced
this issue
Apr 11, 2022
|
OK. This is definitely slow to load in Firefox as well, and there isn't anything else going on in this example test suite, so it's gotta be the module count. I made a few related changes to the performance of this at #1685. @mixonic I propose we do both of the following:
I'd aim for 50 rather than 20, and to keep it populated on open (similarly, upto 50 instead of unlimited). This keeps the free navigation we have today that I think is valuable for beginners, to scroll and explore when uncertain. For the simple cases when one has under 50 modules in total, I think it's benefitial to the experience to have the list there right away and select what you need. (I don't expect to make use of that myself.) We can ignore the compat concern from my previous comment. This would apply to themes, but that's for styling. I'm not aware of an (undocumented) expectation in plugins or themes relying on suggestions being in the DOM by default. And in terms of the results being complete, prior to #1685 we showed relatively few suggestions, and I actually increased it by removing the scoring threshold. To avoid making large test suites feel even slower, I think we should bring back some limitation to the results prior to releasing that change, but this time by absolute count rather than by fuzzy scoring threshold. |
|
I did take a stab at this while waiting for triage here: mixonic@9590890 it seems pretty simple. Via the branch at https://github.com/mixonic/qunit/commits/mixonic/lazy-dropdown-built we've been using that successfully. Funny enough, we always found Firefox fast. It was only for a subset of our Chrome users that it was extremely slow (never for all). |
Krinkle
added a commit
that referenced
this issue
Apr 17, 2022
== Background == As part of fixing #1664, I found that the minimal patch of simply appending the module HTML on focus, no longer worked correctly in certain edge cases due to various refactors that took place since. This commit is the prep work to make the code simpler again, which then makes fixing #1664 really simple. I also fixed numerous bugs I found along the way, and improved the usability in a couple of ways. == Fix bugs == Most of these are specific to a narrow viewport, such as the iframe on the https://qunitjs.com homepage! * Dropdown menu was forced to 400px min-width, causing overflow on narrow viewports. Fixed for modern browsers by limiting min-width to 80vw. * The dropdown menu was positioned too high in narrow viewports. The position was based on `top: 50%; margin-top: 0.8em;`, which approximated "1 line-height from the top". This worked in most viewports, where the dropdown label and input render next to each other. However, in narrow viewports, they wrap are render below each other, and "1 line from the top" is then overlapping half-way through the input field. Fixed by positioning the menu at `top: 100%` instead, which is always below the input field. * The dropdown "arrow" icon was dislodged from the input field in narrow viewports due to being positioned absolutely on the right edge of the parent `<form>` and `<label>`. These are inline-block, which automatically follows the needed width, except when children wrap over multiple lines, then it reports its width as 100% even if there isn't anything in that space. Fixed by giving the parent element `text-align: right;` so that the dropdown menu and input field start from the right even in narrow viewports. On wider viewports this was already accomodated by `float: right;` for the then-single-line element. * Dropdown items had `text-overflow: ellipsis;` but this did not work in practice because it was applied to the `<li>` element instead of the `<label>` where the text resides. The second reason is that the dropdown menu did not have a maximum width, which meant longer module names resulted in the menu getting wider instead of the options being trimmed with ellipsis. This is good and desirable when there is sufficient space. Fixed by seting a liberal max-width, and moving overflow to the `<label>` element, which the ellipsis will appear as-needed. * There was an uncoloured padding between the line that divided the dropdown actions from the dropdown list, and the first selected item's background color. Fixed by removing padding from `#qunit-modulefilter-dropdown-list`. == Usability improvements == * When searching for a module, selecting it, and then searching for something else; previously the only indication of what you selected so far was the "placeholder" of the input field. Apart from being cropped and thus not showing most of the info, this was by definition not visible when actually using the filter to search. It was only shown when not using the filter. Get rid of this placeholder, and instead hoist the selected modules to the top of the results, shown always, even if they don't match. To ensure a retention of vision and ofer an easy way to undo accidental clicks, we only hoist options when re-rendering the dropdown (e.g. after deciding to refine or change your search). When the results are idle, clicks simply toggle the options in their current position. * Remove display toggling of the Reset/Apply buttons. Let these always be visible, with "Apply" always available, and the "Reset" button disabled when the selection is identical to the last-used (same as before). * Replace the "All modules" checkbox with a "Select none" button, and for the common case where this is functionally identical to the "Reset" button, hide it to reduce clutter. * Add tooltips to buttons to clarify their meaning. * Add proper button styles, including high-contrast border and hover/active/disabled styles. * For most viewports (wider than 350px) remove the reserved space for dropdown action buttons, and instead float them to the side and overlap the right side of the first dropdown result. == Remove == Unused code: ``` #qunit-modulefilter-dropdown a { color: inherit; text-decoration: none; } ``` Originally added in d14107a for the "All modules" action, which was an anchor link at the time. This hasn't been the case of a while.
Krinkle
added a commit
that referenced
this issue
Apr 17, 2022
* Remove eager rendering of the dropdown menu from the critical path of `QUnit.begin()`. Defer this to first focus instead. * Limit both initial and fuzzysort results to 20 items, and display them in full without any inline scrollbars. * Render the dropdown in a single parse HTML task via one innerHTML assignment, instead of N createElement calls and N parse HTML tasks. Fixes #1664.
Krinkle
added a commit
that referenced
this issue
Apr 17, 2022
== Background == As part of fixing #1664, I found that the minimal patch of simply appending the module HTML on focus, no longer worked correctly in certain edge cases due to various refactors that took place since. This commit is the prep work to make the code simpler again, which then makes fixing #1664 really simple. I also fixed numerous bugs I found along the way, and improved the usability in a couple of ways. == Fix bugs == Most of these are specific to a narrow viewport, such as the iframe on the https://qunitjs.com homepage! * Dropdown menu was forced to 400px min-width, causing overflow on narrow viewports. Fixed for modern browsers by limiting min-width to 80vw. * The dropdown menu was positioned too high in narrow viewports. The position was based on `top: 50%; margin-top: 0.8em;`, which approximated "1 line-height from the top". This worked in most viewports, where the dropdown label and input render next to each other. However, in narrow viewports, they wrap are render below each other, and "1 line from the top" is then overlapping half-way through the input field. Fixed by positioning the menu at `top: 100%` instead, which is always below the input field. * The dropdown "arrow" icon was dislodged from the input field in narrow viewports due to being positioned absolutely on the right edge of the parent `<form>` and `<label>`. These are inline-block, which automatically follows the needed width, except when children wrap over multiple lines, then it reports its width as 100% even if there isn't anything in that space. Fixed by giving the parent element `text-align: right;` so that the dropdown menu and input field start from the right even in narrow viewports. On wider viewports this was already accomodated by `float: right;` for the then-single-line element. * Dropdown items had `text-overflow: ellipsis;` but this did not work in practice because it was applied to the `<li>` element instead of the `<label>` where the text resides. The second reason is that the dropdown menu did not have a maximum width, which meant longer module names resulted in the menu getting wider instead of the options being trimmed with ellipsis. This is good and desirable when there is sufficient space. Fixed by seting a liberal max-width, and moving overflow to the `<label>` element, which the ellipsis will appear as-needed. * There was an uncoloured padding between the line that divided the dropdown actions from the dropdown list, and the first selected item's background color. Fixed by removing padding from `#qunit-modulefilter-dropdown-list`. == Usability improvements == * When searching for a module, selecting it, and then searching for something else; previously the only indication of what you selected so far was the "placeholder" of the input field. Apart from being cropped and thus not showing most of the info, this was by definition not visible when actually using the filter to search. It was only shown when not using the filter. Get rid of this placeholder, and instead hoist the selected modules to the top of the results, shown always, even if they don't match. To ensure a retention of vision and ofer an easy way to undo accidental clicks, we only hoist options when re-rendering the dropdown (e.g. after deciding to refine or change your search). When the results are idle, clicks simply toggle the options in their current position. * Remove display toggling of the Reset/Apply buttons. Let these always be visible, with "Apply" always available, and the "Reset" button disabled when the selection is identical to the last-used (same as before). * Replace the "All modules" checkbox with a "Select none" button, and for the common case where this is functionally identical to the "Reset" button, hide it to reduce clutter. * Add tooltips to buttons to clarify their meaning. * Add proper button styles, including high-contrast border and hover/active/disabled styles. * For most viewports (wider than 350px) remove the reserved space for dropdown action buttons, and instead float them to the side and overlap the right side of the first dropdown result. == Remove == Unused code: ``` #qunit-modulefilter-dropdown a { color: inherit; text-decoration: none; } ``` Originally added in d14107a for the "All modules" action, which was an anchor link at the time. This hasn't been the case of a while.
|
For future reference, here is a before and after of the usability and design changes I made in preparation for fixing this bug. The code changes were committed separately (design and prep in 06c3951, actual bug fix in 1222426).
Highlights:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
At the codepath
toolbarModuleFilterwheremoduleListHtmlis called the list of all modules in a test suite is iterated and a set of sibling<li>elements is created. For a given test suite, the number of nodes created may be fairly high (for example about 800 in a large codebase I maintain). When these are appended to the DOM, Chrome seems to trigger a slow frame after the append where some kind of internal bookkeeping for the DOM is handled.This may manifest as a 5-15 second delay before tests run (in some cases worse). On the devtools timeline, it looks like this:
In Firefox, this does not seem be reliably be an issue. I suspect there is a Chrome issue when there are a large number of sibling nodes in the DOM, but so far haven't been able to run that down.
Regardless, removing
moduleListHtmlcauses the entire long task to go away. Re-writing themoduleListHtmlfunction to manipulate DOM instead of generating HTML did not improve the issue.In
toolbarModuleFilter, the call tomoduleListHtmlis being run eagerly. It runs regardless of if the module dropdown is going to be opened. There are several paths forward to improving QUnit's load time (even absent the pathological issue in Chrome) that could be considered:I'm happy to land an upstream change. What approach, or what other approach, seems viable?
Tell us about your runtime:
The text was updated successfully, but these errors were encountered: