fix some html-emitter problems on IE

This fixes
http://code.google.com/p/google-caja/issues/detail?id=1060
http://code.google.com/p/google-caja/issues/detail?id=1090

1. Cajoling this will fail to run on IE:

    <div id=a>a1</div>a2
    <script>1</script>
    <div id=b>b1</div>b2
    <script>2</script>
    c1

The error from IE is "'detached[...]' is null or not an object".

The reason is that html-emitter assumes that when you remove a
node from the document, node.parentNode will be null, but IE will
sometimes create another detached node to be the parent.  In this
example, one of the generated <span> elements has a
documentFragment parent.

This change fixes html-emitter so that when it searches
the detached nodes list, it makes sure it's comparing
the parentNode==null ancestor.

2. html-emitter gets confused if the source has
   <form id="x"><input name="id"></form>
because form.id is the <input>, not "x".

This change fixes that by using form.getAttribute('id')
instead of form.id.

[ihab.awad, 2009-07-06 22:28:28]

Felix, do you want me to start reviewing this or should I hold off?

[ihab.awad, 2009-07-10 04:59:40]

The patch so far is LGTM.

Felix, are you going to add a test? *Without* this patch, does
html-emitter-test.html currently work on IE? If so, what changes can you make to
it that would tickle this bug?

[felix8a, 2009-07-10 14:50:10]

On 2009/07/10 04:59:40, ihab.awad wrote:
> The patch so far is LGTM.
> 
> Felix, are you going to add a test? *Without* this patch, does
> html-emitter-test.html currently work on IE? If so, what changes can you make
to
> it that would tickle this bug?

hm, there are a couple things that prevent html-emitter-test.html
from running at all on IE, but with those fixed, yeah, what's there
is enough to trigger the detached[] problem.

[ihab.awad, 2009-07-10 14:52:00]

On 2009/07/10 14:50:10, felix8a wrote:
> hm, there are a couple things that prevent html-emitter-test.html
> from running at all on IE, but with those fixed, yeah, what's there
> is enough to trigger the detached[] problem.

Ok, how's about you add a comment to html-emitter-test.html noting the specific
parts that tickle this bug, and we'll call it even?

[ihab.awad, 2009-07-17 22:46:41]

Reminder: this is LGTM++ with recommendation to write some test if possible.

[felix8a, 2009-07-18 19:26:15]

new snapshot and new description.

[MikeSamuel, 2009-07-20 19:04:51]

http://codereview.appspot.com/89076/diff/4001/3005
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/89076/diff/4001/3005#newcode252
Line 252: var tagAttr = el.tagName.toLowerCase() + ':' + attrName;
Can we use the lower casing function from domita here?  So that on Turkish
browsers, SCRIPT doesn't lower-case to scrıpt.

Rhino at least has this problem:
  $ java -Duser.language=tr -Duser.region=TR \
    -jar third_party/java/rhino/js.jar
  Rhino 1.7 release 1 2008 03 06
  js> 'SCRIPT'.toLowerCase()
  scr?pt
  js> 'script'.toUpperCase()
  SCR?PT
but this may because Rhino delegating to Java's busted
String.to{Upper,Lower}Case

http://codereview.appspot.com/89076/diff/4001/3005#newcode254
Line 254: || html4.ATTRIBS['*:' + attrName] === html4.atype.SCRIPT) {
The second clause only applies if html4.ATTRIBS[tagAttr] is undefined, right?
I don't think this matters in practice, but it is probably worth a comment.

[ihab.awad, 2009-07-20 22:55:03]

Ok Felix, back in your plate :) -- still LTGM++ by me but maybe Mike Samuel
wants another snapshot (or is this LGTM++ by you, Mike?).

[felix8a, 2009-08-06 02:51:00]

updated snapshot and description.

a recent change by MikeSamuel obsoleted part of this patch.

I've also added an html-emitter test to domita_test,
verified that it triggers the problem on IE,
and verified this patch fixes the problem.

[felix8a, 2009-08-06 03:38:43]

hangon, forgot to fold in a small related change

[MikeSamuel, 2009-08-06 05:49:07]

http://codereview.appspot.com/89076/diff/6001/7003
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/89076/diff/6001/7003#newcode252
Line 252: if (atype === html4.atype.SCRIPT) {
I think this is also done in http://codereview.appspot.com/99060/show

Sorry I didn't coordinate with you, but can you look at that change and see
which looks better to you.

http://codereview.appspot.com/89076/diff/6001/7001
File tests/com/google/caja/plugin/HtmlCompiledPluginTest.java (right):

http://codereview.appspot.com/89076/diff/6001/7001#newcode283
Line 283: new RhinoTestBed.Input(getClass(), "html4-defs.js"),
What is this for?

http://codereview.appspot.com/89076/diff/6001/7002
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/89076/diff/6001/7002#newcode2520
Line 2520: // did html-emitter reattach everything?
good idea

[felix8a, 2009-08-11 00:20:53]

sorry for the delay, I was sick.

On 2009/08/06 05:49:07, MikeSamuel wrote:
> http://codereview.appspot.com/89076/diff/6001/7003
> File src/com/google/caja/plugin/html-emitter.js (right):
> 
> http://codereview.appspot.com/89076/diff/6001/7003#newcode252
> Line 252: if (atype === html4.atype.SCRIPT) {
> I think this is also done in http://codereview.appspot.com/99060/show
> 
> Sorry I didn't coordinate with you, but can you look at that change and see
> which looks better to you.

no worries.  yours looks better, and of course is already committed, heh.
I'll update and resnapshot later today.

[felix8a, 2009-08-11 21:25:23]

updated snapshot and description.
new snapshot removes the obsolete fix for IE event handlers,
and adds a fix for form.id confusion.

[MikeSamuel, 2009-08-11 22:52:28]

http://codereview.appspot.com/89076/diff/6007/7010
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/89076/diff/6007/7010#newcode184
Line 184: while (nConsumed < detached.length) {
Why does this condition change?

[felix8a, 2009-08-12 08:05:17]

http://codereview.appspot.com/89076/diff/6007/7010
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/89076/diff/6007/7010#newcode184
Line 184: while (nConsumed < detached.length) {
On 2009/08/11 22:52:28, MikeSamuel wrote:
> Why does this condition change?

last-ditch recovery in case we walk off the end of detached without finding
limit.  I don't know of a case where it might happen, but I didn't expect this
IE weirdness either.  the argument in my mind is fail-fast vs user-friendly
recovery, and I've flipped back and forth several times.

[MikeSamuel, 2009-08-12 16:17:29]

LGTM

http://codereview.appspot.com/89076/diff/6007/7010
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/89076/diff/6007/7010#newcode184
Line 184: while (nConsumed < detached.length) {
On 2009/08/12 08:05:17, felix8a wrote:
> On 2009/08/11 22:52:28, MikeSamuel wrote:
> > Why does this condition change?
> 
> last-ditch recovery in case we walk off the end of detached without finding
> limit.  I don't know of a case where it might happen, but I didn't expect this
> IE weirdness either.  the argument in my mind is fail-fast vs user-friendly
> recovery, and I've flipped back and forth several times.

Fair enough.  Please comment the condition so that a maintainer won't puzzle
over what case might cause it to be false early.

[felix8a, 2009-08-12 20:35:10]

@r3650