Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1)

Issue 89076: fix some html-emitter problems on IE (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 1 month ago by felix8a
Modified:
6 years ago
Reviewers:
MikeSamuel, ihab.awad
CC:
google-caja-discuss_googlegroups.com
Base URL:
http://google-caja.googlecode.com/svn/trunk/
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : fix some html-emitter problems on IE #

Total comments: 2

Patch Set 3 : fix some html-emitter problems on IE #

Total comments: 3

Patch Set 4 : fix some html-emitter problems on IE #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -5 lines) Patch
M src/com/google/caja/plugin/html-emitter.js View 1 2 3 2 chunks +11 lines, -5 lines 3 comments Download
M tests/com/google/caja/plugin/domita_test_untrusted.html View 3 3 chunks +52 lines, -0 lines 0 comments Download

Messages

Total messages: 18
felix8a
6 years, 1 month ago (2009-07-01 21:34:31 UTC) #1
ihab.awad
Felix, do you want me to start reviewing this or should I hold off?
6 years, 1 month ago (2009-07-06 22:28:28 UTC) #2
ihab.awad
The patch so far is LGTM. Felix, are you going to add a test? *Without* ...
6 years, 1 month ago (2009-07-10 04:59:40 UTC) #3
felix8a
On 2009/07/10 04:59:40, ihab.awad wrote: > The patch so far is LGTM. > > Felix, ...
6 years, 1 month ago (2009-07-10 14:50:10 UTC) #4
ihab.awad
On 2009/07/10 14:50:10, felix8a wrote: > hm, there are a couple things that prevent html-emitter-test.html ...
6 years, 1 month ago (2009-07-10 14:52:00 UTC) #5
ihab.awad
Reminder: this is LGTM++ with recommendation to write some test if possible.
6 years, 1 month ago (2009-07-17 22:46:41 UTC) #6
felix8a
new snapshot and new description.
6 years, 1 month ago (2009-07-18 19:26:15 UTC) #7
MikeSamuel
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; ...
6 years, 1 month ago (2009-07-20 19:04:51 UTC) #8
ihab.awad
Ok Felix, back in your plate :) -- still LTGM++ by me but maybe Mike ...
6 years, 1 month ago (2009-07-20 22:55:03 UTC) #9
felix8a
updated snapshot and description. a recent change by MikeSamuel obsoleted part of this patch. I've ...
6 years ago (2009-08-06 02:51:00 UTC) #10
felix8a
hangon, forgot to fold in a small related change
6 years ago (2009-08-06 03:38:43 UTC) #11
MikeSamuel
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 ...
6 years ago (2009-08-06 05:49:07 UTC) #12
felix8a
sorry for the delay, I was sick. On 2009/08/06 05:49:07, MikeSamuel wrote: > http://codereview.appspot.com/89076/diff/6001/7003 > ...
6 years ago (2009-08-11 00:20:53 UTC) #13
felix8a
updated snapshot and description. new snapshot removes the obsolete fix for IE event handlers, and ...
6 years ago (2009-08-11 21:25:23 UTC) #14
MikeSamuel
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 ...
6 years ago (2009-08-11 22:52:28 UTC) #15
felix8a
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, ...
6 years ago (2009-08-12 08:05:17 UTC) #16
MikeSamuel
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 ...
6 years ago (2009-08-12 16:17:29 UTC) #17
felix8a
6 years ago (2009-08-12 20:35:10 UTC) #18
@r3650
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld fd993c8