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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by felix8a
Modified:
4 years, 8 months 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
4 years, 9 months ago #1
ihab.awad
Felix, do you want me to start reviewing this or should I hold off?
4 years, 9 months ago #2
ihab.awad
The patch so far is LGTM. Felix, are you going to add a test? *Without* ...
4 years, 9 months ago #3
felix8a
On 2009/07/10 04:59:40, ihab.awad wrote: > The patch so far is LGTM. > > Felix, ...
4 years, 9 months ago #4
ihab.awad
On 2009/07/10 14:50:10, felix8a wrote: > hm, there are a couple things that prevent html-emitter-test.html ...
4 years, 9 months ago #5
ihab.awad
Reminder: this is LGTM++ with recommendation to write some test if possible.
4 years, 9 months ago #6
felix8a
new snapshot and new description.
4 years, 9 months ago #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; ...
4 years, 9 months ago #8
ihab.awad
Ok Felix, back in your plate :) -- still LTGM++ by me but maybe Mike ...
4 years, 9 months ago #9
felix8a
updated snapshot and description. a recent change by MikeSamuel obsoleted part of this patch. I've ...
4 years, 8 months ago #10
felix8a
hangon, forgot to fold in a small related change
4 years, 8 months ago #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 ...
4 years, 8 months ago #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 > ...
4 years, 8 months ago #13
felix8a
updated snapshot and description. new snapshot removes the obsolete fix for IE event handlers, and ...
4 years, 8 months ago #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 ...
4 years, 8 months ago #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, ...
4 years, 8 months ago #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 ...
4 years, 8 months ago #17
felix8a
4 years, 8 months ago #18
@r3650
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5