I'd like you to do a code review. To review this change, run gvn review --project https://google-caja.googlecode.com/svn mikesamuel/[EMAIL PROTECTED]
Alternatively, to review the latest snapshot of this change branch, run gvn --project https://google-caja.googlecode.com/svn review mikesamuel/bug-730 to review the following change: *mikesamuel/[EMAIL PROTECTED] | mikesamuel | 2008-09-29 11:22:25 -0800 (Mon, 29 Sep 2008) Description: Issue 730: IE7 is cranky about <option> in .innerHTML Modified DomAttributeEvents to check the depth of an optimization. >From the bug: this fails to show any options when cajoled: <form><select id='x'><option>abc</option></select></form> in contrast, this does work: <form><select><option>abc</option></select></form> the problem is, version 1 cajoles to .ih('<option>abc</option>') and ie7 strips out the leading option tag there, maybe because it thinks that option is invalid in that context. version 2 cajoles to .ih('<select><option>abc</option></select>') which works fine. example (run this without cajoling): <html> <body> <form id="f1"></form> <form id="f2"><select id="s2"></select></form> <script> var f1 = document.getElementById('f1'); f1.innerHTML = '<select><option>abc</option></select>'; alert('f1 is ' + f1.innerHTML); var f2 = document.getElementById('f2'); f2.s2.innerHTML = '<option>abc</option>'; alert('f2 is ' + f2.innerHTML); </script> </body> </html> Affected Paths: M //trunk/src/com/google/caja/plugin/DomProcessingEvents.java M //trunk/tests/com/google/caja/plugin/DomProcessingEventsTest.java This is a semiautomated message from "gvn mail". See <http://code.google.com/p/gvn/> to learn more. Index: src/com/google/caja/plugin/DomProcessingEvents.java =================================================================== --- src/com/google/caja/plugin/DomProcessingEvents.java (^/trunk/src/com/google/caja/plugin/[EMAIL PROTECTED]) +++ src/com/google/caja/plugin/DomProcessingEvents.java (^/changes/mikesamuel/bug-730/trunk/src/com/google/caja/plugin/[EMAIL PROTECTED]) @@ -111,8 +111,16 @@ final class DomProcessingEvents { int start = top.b + 1; List<DomProcessingEvent> content = events.subList(start, i); if (content.isEmpty()) { continue; } + int depth = 0; for (DomProcessingEvent ce : content) { - if (!ce.canOptimizeToInnerHtml()) { continue eventloop; } + if (ce instanceof BeginElementEvent) { + ++depth; + } else if (ce instanceof EndElementEvent + || (ce instanceof FinishAttrsEvent + && ((FinishAttrsEvent) ce).unary)) { + --depth; + } + if (!ce.canOptimizeToInnerHtml(depth)) { continue eventloop; } } StringBuilder innerHtml = new StringBuilder(); for (DomProcessingEvent ce : content) { @@ -146,8 +154,12 @@ final class DomProcessingEvents { * True iff the event's state is entirely statically determinable, so can * be replaced by a call to innerHTML when part of a run of events that * include entire balanced tag sets and text nodes. + * @param depth the number of containing elements also being optimized. + * This allows us to handle elements that cannot be inserted via innerHTML + * due to browser quirks. See the unittests for specific examples. + * If depth is -1, then acts conservatively. */ - abstract boolean canOptimizeToInnerHtml(); + abstract boolean canOptimizeToInnerHtml(int depth); /** * Appends HTML to the given buffer. * Only callable if [EMAIL PROTECTED] #canOptimizeToInnerHtml}. @@ -159,7 +171,7 @@ final class DomProcessingEvents { String cn = getClass().getSimpleName(); StringBuilder sb = new StringBuilder(); sb.append('(').append(cn, cn.lastIndexOf('.') + 1, cn.length()); - if (canOptimizeToInnerHtml()) { + if (canOptimizeToInnerHtml(-1)) { sb.append(' '); toInnerHtml(sb); } @@ -174,7 +186,9 @@ final class DomProcessingEvents { @Override void toJavascript(BlockAndEmitter out) { out.emitCall("b", TreeConstruction.stringLiteral(name)); } - @Override boolean canOptimizeToInnerHtml() { return true; } + @Override boolean canOptimizeToInnerHtml(int depth) { + return depth > 1 || !"option".equals(name); + } @Override void toInnerHtml(StringBuilder out) { out.append('<').append(name); } @@ -194,7 +208,7 @@ final class DomProcessingEvents { @Override void toJavascript(BlockAndEmitter out) { out.emitCall("a", TreeConstruction.stringLiteral(name), value); } - @Override boolean canOptimizeToInnerHtml() { + @Override boolean canOptimizeToInnerHtml(int depth) { return value instanceof StringLiteral; } @Override void toInnerHtml(StringBuilder out) { @@ -217,7 +231,7 @@ final class DomProcessingEvents { @Override void toJavascript(BlockAndEmitter out) { out.emitCall("f", new BooleanLiteral(unary)); } - @Override boolean canOptimizeToInnerHtml() { return true; } + @Override boolean canOptimizeToInnerHtml(int depth) { return true; } @Override void toInnerHtml(StringBuilder out) { out.append(unary ? " />" : ">"); } @@ -233,7 +247,7 @@ final class DomProcessingEvents { @Override void toJavascript(BlockAndEmitter out) { out.emitCall("e", TreeConstruction.stringLiteral(name)); } - @Override boolean canOptimizeToInnerHtml() { return true; } + @Override boolean canOptimizeToInnerHtml(int depth) { return true; } @Override void toInnerHtml(StringBuilder out) { out.append("</").append(name).append('>'); } @@ -250,7 +264,7 @@ final class DomProcessingEvents { out.emitCall(getEmitterMethodName(), TreeConstruction.stringLiteral(text)); } - @Override boolean canOptimizeToInnerHtml() { return true; } + @Override boolean canOptimizeToInnerHtml(int depth) { return true; } protected abstract String getEmitterMethodName(); @Override boolean checkContext(boolean inTag) { if (inTag) { throw new IllegalStateException(this.toString()); } @@ -280,7 +294,7 @@ final class DomProcessingEvents { @Override void toJavascript(BlockAndEmitter out) { out.getBlock().appendChild(stmt); } - @Override boolean canOptimizeToInnerHtml() { return false; } + @Override boolean canOptimizeToInnerHtml(int depth) { return false; } @Override void toInnerHtml(StringBuilder out) { throw new UnsupportedOperationException(); } Index: tests/com/google/caja/plugin/DomProcessingEventsTest.java =================================================================== --- tests/com/google/caja/plugin/DomProcessingEventsTest.java (^/trunk/tests/com/google/caja/plugin/[EMAIL PROTECTED]) +++ tests/com/google/caja/plugin/DomProcessingEventsTest.java (^/changes/mikesamuel/bug-730/trunk/tests/com/google/caja/plugin/[EMAIL PROTECTED]) @@ -87,6 +87,67 @@ public class DomProcessingEventsTest extends CajaT dpe); } + public void testDeOptimization() throws Exception { + // Some elements can't be created at the top level by innerHTML. So + // var select = document.createElement('SELECT'); + // select.innerHTML = '<option>1</option>; + // doesn't work on some browsers. + // But the below does work, so another test below allows <OPTION>s to be + // optimized properly: + // var div = document.createElement('DIV'); + // div.innerHTML = '<select><option>1</option></select>'; + // See bug 730 for more details. + DomProcessingEvents dpe = new DomProcessingEvents(); + dpe.begin("select"); + dpe.finishAttrs(false); + dpe.begin("option"); + dpe.finishAttrs(false); + dpe.pcdata("1"); + dpe.end("option"); + dpe.begin("option"); + dpe.finishAttrs(false); + dpe.pcdata("2"); + dpe.end("option"); + dpe.end("select"); + assertEmittingCode( + "IMPORTS___.htmlEmitter___.b('select').f(false)" + + ".b('option')" + + ".f(false)" + + ".ih('1')" + + ".e('option')" + + ".b('option')" + + ".f(false)" + + ".ih('2')" + + ".e('option')" + + ".e('select');", + "<select><option>1</option><option>2</option></select>", + dpe); + } + + public void testReDeOptimization() throws Exception { + DomProcessingEvents dpe = new DomProcessingEvents(); + dpe.begin("div"); + dpe.finishAttrs(false); + dpe.begin("select"); + dpe.finishAttrs(false); + dpe.begin("option"); + dpe.finishAttrs(false); + dpe.pcdata("1"); + dpe.end("option"); + dpe.begin("option"); + dpe.finishAttrs(false); + dpe.pcdata("2"); + dpe.end("option"); + dpe.end("select"); + dpe.end("div"); + assertEmittingCode( + "IMPORTS___.htmlEmitter___.b('div').f(false)" + + ".ih('<select><option>1</option><option>2</option></select>')" + + ".e('div');", + "<div><select><option>1</option><option>2</option></select></div>", + dpe); + } + public void testInterleaving() throws Exception { DomProcessingEvents dpe = new DomProcessingEvents(); dpe.begin("p"); --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to http://groups.google.com/group/google-caja-discuss To unsubscribe, email [EMAIL PROTECTED] -~----------~----~----~----~------~----~------~--~---
