Replace host innerHTML with our own HTML5-compliant serializer.

Our previous strategy to provide a innerHTML getter was to invoke the
browser's, then sanitize the output. This strategy cannot implement
foreign node protection, since whether a node is foreign depends on its
object identity, which is not available starting from an innerHTML
string.

Therefore, it is replaced by our own implementation working off the
tame nodes (and therefore unable to provide excess authority over
regular DOM traversal), and operating according to the HTML5
specification's HTML fragment serialization algorithm.

@r5144

[felix8a, 2012-09-13 21:00:17]

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1827: if (/["'>=]/.test(aName)) {
this test seems inadequate, like spaces are also problematic in attribute names.
 why have the test at all? innerHTML is not guaranteed to produce sensible
results from nonsensical dom trees, and it seems to me that reading .innerHTML
should never throw an error.

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1867: if (tameCurrent.firstChild.nodeType
=== 3 &&
firstChild might be null

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1905: throw new Error('Internal: HTML
fragment serialization '
I'd prefer silently ignoring unexpected node types, but don't feel strongly
about this.

https://codereview.appspot.com/6498123/diff/1/tests/com/google/caja/plugin/es...
File tests/com/google/caja/plugin/es53-test-domado-foreign-guest.html (right):

https://codereview.appspot.com/6498123/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-domado-foreign-guest.html:119: if (parent
&& 'innerHTML' in parent) {
I'm wary of this condition here, because it loses the distinction between "test
passed correctly because we didn't get parent, as expected" vs "test passed
incorrectly, because we unexpectedly didn't get parent"

how about put this part of the test in testExternalForeign and
testEmbeddedForeign, without the conditional

[kpreid2, 2012-09-13 21:37:49]

Patch Set 2 incorporates your feedback.

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1827: if (/["'>=]/.test(aName)) {
On 2012/09/13 21:00:18, felix8a wrote:
> this test seems inadequate, like spaces are also problematic in attribute
names.
> why have the test at all? innerHTML is not guaranteed to produce sensible
> results from nonsensical dom trees, and it seems to me that reading .innerHTML
> should never throw an error.

Agreed. Removed the test.

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1867: if (tameCurrent.firstChild.nodeType
=== 3 &&
On 2012/09/13 21:00:18, felix8a wrote:
> firstChild might be null

Done.

https://codereview.appspot.com/6498123/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1905: throw new Error('Internal: HTML
fragment serialization '
On 2012/09/13 21:00:18, felix8a wrote:
> I'd prefer silently ignoring unexpected node types, but don't feel strongly
> about this.

Changed to console.error and ignore.

https://codereview.appspot.com/6498123/diff/1/tests/com/google/caja/plugin/es...
File tests/com/google/caja/plugin/es53-test-domado-foreign-guest.html (right):

https://codereview.appspot.com/6498123/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-domado-foreign-guest.html:119: if (parent
&& 'innerHTML' in parent) {
On 2012/09/13 21:00:18, felix8a wrote:
> how about put this part of the test in testExternalForeign and
> testEmbeddedForeign, without the conditional

Changed the test to be a parameter to testForeign. I don't want to make it
harder to update the tests if we get a third foreign test case (e.g. foreign w/
no parent vs foreign w/ outside-vdoc parent).

[felix8a, 2012-09-13 21:47:22]

lgtm

[felix8a, 2012-10-12 22:40:47]

this now conflicts with virtual element changes in domado.js on trunk, could you
please update it?

[kpreid2, 2012-10-12 22:49:08]

On 2012/10/12 22:40:47, felix8a wrote:
> this now conflicts with virtual element changes in domado.js on trunk, could
you
> please update it?

Updated.