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.

Sign up for GitHub

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

Jump to bottom

An assignment to innerHTML fails in XHTML #1255

Closed
phormio opened this issue Jan 23, 2018 · 5 comments
Closed

An assignment to innerHTML fails in XHTML #1255

phormio opened this issue Jan 23, 2018 · 5 comments
Labels
Component: HTML Reporter Type: Bug

Comments

@phormio
Copy link

phormio commented Jan 23, 2018 •
edited

Tell us about your runtime:

  • QUnit version: 2.5.0
  • What environment are you running QUnit in? (e.g., browser, Node): Browser. Opera 50.0.2762.67 on Linux.
  • How are you running QUnit? (e.g., script, testem, Grunt): I have an XHTML page. It loads RequireJS with a script tag. RequireJS is then configured with the location of QUnit. Then a tiny test suite is run, which contains one call to QUnit.test.

What are you trying to do?

Here is my XHTML page. It contains all you need to trigger the bug.

<?xml version="1.0"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
  <head>
    <meta http-equiv="Content-Style-Type" content="text/css"/>
    <meta name="viewport" content="width=device-width, initial-scale=1"/>

    <title>Demonstrate QUnit bug affecting XHTML pages</title>

    <link
      rel="stylesheet"
      href="https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.css"
    />
  </head>

  <body>
    <div id="qunit"/>
    <div id="qunit-fixture"/>

    <script
      type="text/javascript"
      src="https://cdn.jsdelivr.net/npm/requirejs@2.3.5/require.js"
    />

    <script type="text/javascript">
      <![CDATA[
      requirejs.config(
        {
          paths: {
            QUnit:
              'https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit',
          },
        }
      );

      require(['QUnit'], run_test_suite);

      function run_test_suite(QUnit) {
        QUnit.test(
          "Failing test",
          function (assert) {
            assert.ok(false);
          }
        );
        QUnit.start();
      }
      ]]>
    </script>
  </body>
</html>

If you're not familiar with an AMD loader such as RequireJS, you can ignore the AMD mechanics by just concentrating on the run_test_suite function.

What did you expect to happen?

I loaded the above page into my web browser, expecting that the page would contain a message from QUnit reporting a failing assertion.

What actually happened?

The page showed a number of things, including the text "Running..." and a button labelled "Abort". However, the outcome of the test was not shown.

In the console, the following message appeared:

Uncaught DOMException: Failed to set the 'innerHTML' property on 'Element': The provided markup is invalid XML, and therefore cannot be inserted into an XML document.
    at toolbarModuleFilter (https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:3540:23)
    at appendToolbar (https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:3652:26)
    at appendInterface (https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:3726:5)
    at Array.<anonymous> (https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:3790:5)
    at runLoggingCallbacks (https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:1047:17)
    at begin (https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:3052:5)
    at https://cdn.jsdelivr.net/npm/qunit@2.5.0/qunit/qunit.js:3019:6
@phormio
Copy link
Author

phormio commented Jan 23, 2018

I forgot to say: you can find the buggy code here:

https://github.com/qunitjs/qunit/blob/2.5.0/reporter/html.js#L352-L358

@phormio
Copy link
Author

phormio commented Jan 23, 2018 •
edited

Further to my previous comment: I have verified that you can fix the bug by closing the input element with /> rather than >.

UPDATE

But when you do that, it seems that you uncover another bug. I get the following error message in the console:

Uncaught DOMException: Failed to set the 'innerHTML' property on 'Element': The provided markup is invalid XML, and therefore cannot be inserted into an XML document.
    at Array.<anonymous> (file:///tmp/qunit-2.5.0.modified.js:4054:27)
    at runLoggingCallbacks (file:///tmp/qunit-2.5.0.modified.js:1047:17)
    at Test.finish (file:///tmp/qunit-2.5.0.modified.js:1603:5)
    at file:///tmp/qunit-2.5.0.modified.js:1674:12
    at Object.advance (file:///tmp/qunit-2.5.0.modified.js:1116:26)
    at begin (file:///tmp/qunit-2.5.0.modified.js:3059:20)
    at file:///tmp/qunit-2.5.0.modified.js:3019:6

@platinumazure
Copy link
Contributor

platinumazure commented Jan 23, 2018 •
edited

Hi @phormio, thanks for the issue!

Interesting! <input> is valid HTML, but not valid XML. Guess it depends on the DOM runtime to some degree. <input /> should also be valid HTML, so I think that fix is safe.

That said: Holy crap, we really should use template strings or something else to make that file more readable. 😆

@phormio
Copy link
Author

phormio commented Jan 23, 2018

thanks for the issue!

You're welcome 😀.

<input /> should also be valid HTML

Yes, it's valid syntax for HTML. Quoting https://html.spec.whatwg.org/multipage/syntax.html#start-tags:

if the element is one of the void elements [...], then there may be a single U+002F SOLIDUS character (/).

shlomif added a commit to shlomif/qunit that referenced this issue Oct 3, 2018
@shlomif
HTML Reporter: Fix XHTML output (issue 1255).
969c46d 
See qunitjs#1255 .
@shlomif shlomif mentioned this issue Oct 3, 2018
Merged
shlomif added a commit to shlomif/qunit that referenced this issue Oct 4, 2018
@shlomif
HTML Reporter: Add an XHTML output test
5457418 
See qunitjs#1255 .

See qunitjs#1317 .
trentmwillis pushed a commit that referenced this issue Oct 8, 2018
@shlomif @trentmwillis
HTML Reporter: Fix XHTML output (issue 1255).
f9b7e33 
See #1255 .
trentmwillis pushed a commit that referenced this issue Oct 8, 2018
@shlomif @trentmwillis
HTML Reporter: Add an XHTML output test
2f734ad 
See #1255 .

See #1317 .
@trentmwillis
Copy link
Member

Fixed via #1317. Will be in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component: HTML Reporter Type: Bug
Milestone
No milestone
Development

No branches or pull requests
3 participants
@platinumazure @trentmwillis @phormio