Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

jqLite('<div>') throws an error in an XHTML document #6917

Closed
Arnavion opened this issue Mar 31, 2014 · 11 comments · Fixed by #16518
Closed

jqLite('<div>') throws an error in an XHTML document #6917

Arnavion opened this issue Mar 31, 2014 · 11 comments · Fixed by #16518
Labels
component: jqLite frequency: low severity: confusing type: bug
Milestone
Backlog - won't fix

Comments

@Arnavion
Copy link

This one. There are a few in test/jqLiteSpec.js as well.

For example, the error on Chrome is Error: Failed to set the 'innerHTML' property on 'Element': The provided markup is invalid XML, and therefore cannot be inserted into an XML document.

The reason is that the implementation of jqLite tries to set innerHTML to that argument on this line div.innerHTML = '<div>&#160;</div>' + element; (where element is '<div>') and this is malformed without a closing </div> at the end.

It is trivial to test: just call jqLite('<div>') and see if it throws the above error.

I found this issue with its associated commit that seems to indicate the author knew about this problem but I suppose they didn't actually test it in XHTML.
@caitp
Copy link
Contributor

caitp commented Mar 31, 2014

Works for me in devtools, even by setting innerHTML to invalid markup as you suggested, in an XHTML document.

$0.ownerDocument.doctype
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
$0.innerHTML
"&lt;?xml version="1.0"?&gt;
&lt;html xmlns="http://www.w3.org/1999/xhtml"&gt;
  &lt;head&gt;
    &lt;title&gt;circles&lt;/title&gt;
  &lt;/head&gt;
  &lt;body&gt;
    &lt;object height="350" width="600" type="image/svg+xml" data="circles.svg"/&gt;
  &lt;/body&gt;
&lt;/html&gt;
"
$0.innerHTML = '<div>'
"<div>"
$0.innerHTML
"<div></div>"
$0.innerHTML = "<div>&#160;</div><div>Hello, world!"
"<div>&#160;</div><div>Hello, world!"
$0.innerHTML
"<div>&nbsp;</div><div>Hello, world!</div>"

I'll have to test it with a proper test obviously, but in any case, just include the closing tag in your template?

@Arnavion
Copy link
Author

Works for me in devtools, even by setting innerHTML to invalid markup as you suggested, in an XHTML document.

Ah, I didn't think it was specific to XHTML5 (what I'm using). Here's a minimal file (serve with Content-Type: application/xhtml+xml)

<?xml version="1.0" encoding="utf-8" ?>
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
    <head/>
    <body/>
</html>

(Notice no doctype, only the XML header, as is allowed by XHTML5).

You will see the error now.

> document.body.innerHTML = "<div>"
SyntaxError: Failed to set the 'innerHTML' property on 'Element': The provided markup is invalid XML, and therefore cannot be inserted into an XML document.

but in any case, just include the closing tag in your template?

It has nothing to do with my template (and in fact my template is well formed). The line that inserts only the opening tag is in angular's source. It's the very first thing I've linked in the OP.

@caitp
Copy link
Contributor

caitp commented Mar 31, 2014

div.innerHTML = '<div>&#160;</div>' + element; (where element is '<div>') and this is malformed without a closing at the end.

What I mean is, element is the parameter you're passing to jqLite, this is your string template. If element is "<div>" without a closing tag, then this is potentially a problem.

There is no code in the JQLite constructor where innerHTML is being set without a closing tag, unless you ask for it specifically.

@Arnavion
Copy link
Author

Again, it is the first thing I've linked in the OP. The call to jqLite('<div>') is in Angular's source and results in '<div>' being the value of element It is not my template.

@caitp
Copy link
Contributor

caitp commented Mar 31, 2014

Feel free to send a patch to add a closing tag to that call. What I'm saying is that this is not really a bug with jqLite

@Arnavion
Copy link
Author

FYI, jQuery('<div>') does succeed in the same document, and gives the expected result.

I don't know whether you want parity with jqLite(...) and jQuery(...) (which would require fixing jqLite to detect it's being called with only a starting tag and append the ending tag itself) or if it's okay to just change that call to jqLite('<div></div>') (which would require changing that and all other callsites in Angular where it's called with just a starting tag).

@caitp
Copy link
Contributor

caitp commented Mar 31, 2014

making jqLite as robust as jQuery is not really in the cards, jQuery is huge and has people actually working on it and maintaining it, while jqLite is just the minimal that is needed for angular to work. Getting feature-parity with jQuery is not going to happen

@Arnavion
Copy link
Author

All right, then as you said, this can be considered a bug in all places in Angular that call jqLite with just a starting tag, and not in jqLite itself. One such place is the one I've opened this bug about; I didn't look for more.

@caitp
Copy link
Contributor

caitp commented Mar 31, 2014

I don't think you'll find very many of these, the compiler is wrapping everything correctly when it manipulates templates on its own

@Arnavion
Copy link
Author

Well, I just grepped for jqLite\('<[^>]+>'\) and found these (filtering out results from self-closed tags, comments, etc.)

   File: C:\Stuff\Sources\angular.js\src\Angular.js

  Num Line
  --- ----
 1028 var elemHtml = jqLite('<div>').append(element).html();


   File: C:\Stuff\Sources\angular.js\test\ng\directive\inputSpec.js

  Num Line
  --- ----
   36 $element: jqLite('<input ng-model="1+2">'),


   File: C:\Stuff\Sources\angular.js\test\jqLiteSpec.js

  Num Line
  --- ----
  481 var select = jqLite('<select>');
  483 expect(jqLite('<select multiple>').attr('multiple')).toBe('multiple');
  484 expect(jqLite('<select multiple="">').attr('multiple')).toBe('multiple');
  485 expect(jqLite('<select multiple="x">').attr('multiple')).toBe('multiple');
  489 var select = jqLite('<select>');
  498 var input = jqLite('<input readonly>');
  526 var elm = jqLite('<input>');
  548 var elm = jqLite('<input type="checkbox">');
 1332 var root = jqLite('<div>').html('before-<div></div>after');
 1340 var root = jqLite('<div>').html('before-<div></div>after');
 1398 var root = jqLite('<div>');
 1403 var root = jqLite('<div>');
 1441 var root = jqLite('<div>');
 1456 var root = jqLite('<div>');

My grep wouldn't have found any calls to jqLite that don't pass in a literal string, and just grepping for jqLite finds many more results than I care to investigate...

@caitp
Copy link
Contributor

caitp commented Mar 31, 2014

These are in unit tests, they aren't going to affect the runtime.

@btford btford added this to the Backlog milestone Apr 3, 2014
@btford btford removed the gh: issue label Aug 20, 2014
mgol added a commit to mgol/angular.js that referenced this issue Mar 28, 2018
@mgol
fix(jqLite): use XHTML-compliant HTML as input for jqLite
eefd120 
Fixes angular#6917
@mgol mgol mentioned this issue Mar 28, 2018
Merged
3 tasks
Narretz pushed a commit that referenced this issue Apr 3, 2018
@mgol @Narretz
fix(jqLite): use XHTML-compliant HTML as input for jqLite
a8dedf7 
Fixes #6917
Closes #16518
mgol added a commit that referenced this issue Apr 4, 2018
@mgol
fix(jqLite): use XHTML-compliant HTML as input for jqLite
a0c55af 
Fixes #6917
Closes #16518
This was referenced Apr 18, 2018
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
component: jqLite frequency: low severity: confusing type: bug
Projects
None yet
Milestone
Backlog - won't fix
Development

Successfully merging a pull request may close this issue.

fix(jqLite): use XHTML-compliant HTML as input for jqLite mgol/angular.js
4 participants
@btford @Arnavion @Narretz @caitp