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

appendTestResults() creates DOM that is invalid HTML if serialised #1301

Closed
kjax opened this issue Aug 2, 2018 · 4 comments
Closed

appendTestResults() creates DOM that is invalid HTML if serialised #1301

kjax opened this issue Aug 2, 2018 · 4 comments
Assignees
Labels
Milestone

Comments

@kjax
Copy link

kjax commented Aug 2, 2018

Tell us about your runtime:

  • QUnit version: 2.6.1
  • What environment are you running QUnit in? (e.g., browser, Node): Browser
  • How are you running QUnit? (e.g., script, testem, Grunt): grunt

What are you trying to do?

If the contents of "#qunit" is extracted with innerHTML and written to a HTML file, the resultant HTML is invalid. This is because "#qunit-testresult" is created as a "p" and its innerHTML is filled with several "divs" like "#qunit-testresult-display" and "#qunit-testresult-controls".

The problem is that a "div" element is not valid content of a "p". In fact the HTML spec states that a "p" followed by a "div" should assume that the end tag of the "p" has been omitted.

The result of parsing <p id=qunit-testresult"><div id=qunit-testresult-display /></p> is an empty "p" element with the "divs" as siblings. This breaks any CSS used to format this section.

The fix is to make "#qunit-testresult" a "div" instead.

@Krinkle
Copy link
Member

Krinkle commented Dec 22, 2018

Thanks for reporting. I've reproduced it in a small test case at CodePen: https://codepen.io/Krinkle/pen/zywGYE.

While I'm familiar with this behaviour for other elements in the HTML spec (such as list items and tables), I was surprised to see it apply to well-balanced elements like this. I know that for anchor tags, for example, the spec was updated to match reality (that is, they are allowed to span block elements).

Looks like we should get this fixed!

@mgol
Copy link
Member

mgol commented Feb 28, 2019

I hit this issue when migrating the jQuery repository from QUnit 1 to 2. Not a very big deal but it messes up formatting of the tests results header a little.
screen shot 2019-02-28 at 10 45 07

@Krinkle Krinkle added this to the 3.0 milestone Aug 30, 2020
@Krinkle
Copy link
Member

Krinkle commented Aug 31, 2020

Easy to fix, but will do in 3.0 in case any plugins depend on this.

@Krinkle Krinkle removed this from the 3.0 release milestone Nov 7, 2020
@Krinkle Krinkle mentioned this issue Nov 7, 2020
14 tasks
@Krinkle Krinkle added this to the 3.0 release milestone Nov 11, 2020
@Krinkle
Copy link
Member

Krinkle commented May 8, 2021

I've addressed this as part of a draft for the QUnit 3.0 theme, at https://github.com/Krinkle/qunit/tree/qunit3-theme:

  • Previously, the #qunit-testresult element was a P with a DIV child
    of #qunit-testresult-display. This element nesting has been reversed,
    to be less strange. The testresult is now a DIV, and the display child
    is now a P.

@Krinkle Krinkle self-assigned this May 8, 2021
@Krinkle Krinkle changed the title Incorrect DOM built in appendTestResults() appendTestResults() creates a DOM that becomes invalid HTML if serialised Aug 1, 2021
@Krinkle Krinkle changed the title appendTestResults() creates a DOM that becomes invalid HTML if serialised appendTestResults() creates DOM that is invalid HTML if serialised Aug 1, 2021
Krinkle added a commit to Krinkle/qunit-theme-ninja that referenced this issue Apr 18, 2022
This version still works fine with older versions of QUnit as well,
but it now handles better the changes to the module filter buttons
in QUnit 2.18.2.

Also fix a few old alignment bugs, and make it not pointer-cursor
hover the unclickable `<strong>` element for test source URL. This
style was intended for the (clickable) test name, which happens to
also be in a `<strong>` element. Not great, to be improved in QUnit 3
with new HTML Reporter structure.

Ref qunitjs/qunit#1301.
@Krinkle Krinkle closed this as completed in f3faf02 Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants