Fix new Element.html with tables in IE

Fix new Element.html with tables in IE
http://dartbug.com/7902

Committed: https://code.google.com/p/dart/source/detail?r=17104

[Jennifer Messerly, 2013-01-15 05:23:11]

This change fixes "new Element.html" in tables in IE. Basically, we workaround
innerHTML in tables by using a div and injecting a <table>.

This change adds tests for Element.html + tables. I tested it in DRT, as well as
testing a modified version of my change that runs the new code in DRT (otherwise
it would require MSIE to test). I also tested our web_ui article examples in IE9
against this change, and it fixes the bug in our table example.

I'm not sure how to run ./tools/test.py on Windows but if you give me some
pointers I can try it, or I can submit during a quiet time and be ready to
revert.

Cheers! - John

[blois, 2013-01-15 17:54:41]

For testing, I just have a server on my Linux box at my Dart root and I just
manually run the tests from Windows & OSX. I took a look and it looks good on
IE10 & IE9 compat mode (still small chance of something lurking in genuine IE9).

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:891:
window.navigator.userAgent.contains('MSIE')) {
Should probably use _Device.isIE instead.

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:926: static Element
_createTableForIE(String html, String tag) {
Will be interesting when we move to SafeHTML.

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:952: return
_singleNode(table.children.where((n) => n.tagName == 'COLGROUP')
Use n is TableColElement instead? In general we haven't been using tagName much
at all.

Alternatively, table.query('colgroup')?

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:956: static Node
_singleNode(List<Node> list) {
List.single instead?

[Emily Fortuna, 2013-01-15 20:06:27]

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:890: if
(_TABLE_TAGS.containsKey(tag) &&
this is super nit-picky, but consider switching the order of these two clauses
in the "and" expression -- since createElement_html is a frequently used method,
the number of times that tag is in _TABLE_TAG  is likely greater than the number
of times userAgent == MSIE in the grand scope of usage of this function, so we
can take advantage of that lazy evaluation. But perhaps this is unnecessary to
worry about at this point... so just take this as a suggestion.

[Emily Fortuna, 2013-01-15 20:07:22]

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:937: case 'tbody':
element = _singleNode(table.tBodies); break;
also, I'm not thrilled with the formatting of this switch statement, but I can
be persuaded otherwise.

[Jennifer Messerly, 2013-01-15 20:11:28]

thanks!

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:891:
window.navigator.userAgent.contains('MSIE')) {
On 2013/01/15 17:54:41, blois wrote:
> Should probably use _Device.isIE instead.

Good catch! Will fix. I figured there must be a better way :)

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:926: static Element
_createTableForIE(String html, String tag) {
On 2013/01/15 17:54:41, blois wrote:
> Will be interesting when we move to SafeHTML.

Agreed :)

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:952: return
_singleNode(table.children.where((n) => n.tagName == 'COLGROUP')
On 2013/01/15 17:54:41, blois wrote:
> Use n is TableColElement instead?

I was thinking tagName might be faster than doing an "is" check in JS (which
IIRC is a bit complicated for DOM elements). Also TableColElement is shared by
"col" and "colgroup" (here we should only get colgroup, but it makes me a little
nervous). Is there any reason to avoid .tagName? It seems like a nice way to
guarentee we get exactly the tag we expect.

In general we haven't been using tagName much
> at all.
> 
> Alternatively, table.query('colgroup')?

Was trying to avoid query/queryAll because it might return something in the
subtree, and it's more expensive (query whole subtree)

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:956: static Node
_singleNode(List<Node> list) {
On 2013/01/15 17:54:41, blois wrote:
> List.single instead?

Thought about that. It would certainly be cleaner (I could delete this
function). But we wouldn't get the nice error message, instead just a generic
StateError. Does it matter? WDYT?

[Jennifer Messerly, 2013-01-15 20:41:21]

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:890: if
(_TABLE_TAGS.containsKey(tag) &&
On 2013/01/15 20:06:27, Emily Fortuna wrote:
> this is super nit-picky, but consider switching the order of these two clauses
> in the "and" expression -- since createElement_html is a frequently used
method,
> the number of times that tag is in _TABLE_TAG  is likely greater than the
number
> of times userAgent == MSIE in the grand scope of usage of this function, so we
> can take advantage of that lazy evaluation. But perhaps this is unnecessary to
> worry about at this point... so just take this as a suggestion.

Agreed. Will put _Device.isIE first.

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:937: case 'tbody':
element = _singleNode(table.tBodies); break;
On 2013/01/15 20:07:23, Emily Fortuna wrote:
> also, I'm not thrilled with the formatting of this switch statement, but I can
> be persuaded otherwise.

will fix

[blois, 2013-01-15 20:47:51]

lgtm

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:952: return
_singleNode(table.children.where((n) => n.tagName == 'COLGROUP')
On 2013/01/15 20:11:28, John Messerly wrote:
> On 2013/01/15 17:54:41, blois wrote:
> > Use n is TableColElement instead?
> 
> I was thinking tagName might be faster than doing an "is" check in JS (which
> IIRC is a bit complicated for DOM elements). Also TableColElement is shared by
> "col" and "colgroup" (here we should only get colgroup, but it makes me a
little
> nervous). Is there any reason to avoid .tagName? It seems like a nice way to
> guarentee we get exactly the tag we expect.
> 
> In general we haven't been using tagName much
> > at all.
> > 
> > Alternatively, table.query('colgroup')?
> 
> Was trying to avoid query/queryAll because it might return something in the
> subtree, and it's more expensive (query whole subtree)

Checking the tagname is fine. I believe that the 'is' check is fairly cheap in
dart2js right now- it's just an overloaded is$TableColElement function that
returns true on TableColElement (though still not as cheap as comparing strings,
but probably within the noise level).

https://codereview.chromium.org/11888019/diff/1005/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:956: static Node
_singleNode(List<Node> list) {
On 2013/01/15 20:11:28, John Messerly wrote:
> On 2013/01/15 17:54:41, blois wrote:
> > List.single instead?
> 
> Thought about that. It would certainly be cleaner (I could delete this
> function). But we wouldn't get the nice error message, instead just a generic
> StateError. Does it matter? WDYT?

I suppose it probably is one of the more common mistakes someone would make with
this API, so OK as is.