Chromium Code Reviews Chromium Code Reviews
Issue 11888019:
Fix new Element.html with tables in IE (Closed)
Base URL: https://dart.googlecode.com/svn/branches/bleeding_edge/dart| Index: tools/dom/templates/html/impl/impl_Element.darttemplate |
| diff --git a/tools/dom/templates/html/impl/impl_Element.darttemplate b/tools/dom/templates/html/impl/impl_Element.darttemplate |
| index 24ff43e5df6cea62ab00298b869e45ffcd1df52c..77db3d0c07e3a3470f06c00d10f489ffb6495574 100644 |
| --- a/tools/dom/templates/html/impl/impl_Element.darttemplate |
| +++ b/tools/dom/templates/html/impl/impl_Element.darttemplate |
| @@ -846,11 +846,12 @@ $!MEMBERS |
| final _START_TAG_REGEXP = new RegExp('<(\\w+)'); |
| class _ElementFactoryProvider { |
| - static final _CUSTOM_PARENT_TAG_MAP = const { |
| + static const _CUSTOM_PARENT_TAG_MAP = const { |
| 'body' : 'html', |
| 'head' : 'html', |
| 'caption' : 'table', |
| 'td': 'tr', |
| + 'th': 'tr', |
| 'colgroup': 'table', |
| 'col' : 'colgroup', |
| 'tr' : 'tbody', |
| @@ -860,6 +861,19 @@ class _ElementFactoryProvider { |
| 'track' : 'audio', |
| }; |
| + // TODO(jmesserly): const set would be better |
| + static const _TABLE_TAGS = const { |
| + 'caption': null, |
| + 'col': null, |
| + 'colgroup': null, |
| + 'tbody': null, |
| + 'td': null, |
| + 'tfoot': null, |
| + 'th': null, |
| + 'thead': null, |
| + 'tr': null, |
| + }; |
| + |
| /** @domName Document.createElement */ |
| static Element createElement_html(String html) { |
| // TODO(jacobr): this method can be made more robust and performant. |
| @@ -873,29 +887,78 @@ class _ElementFactoryProvider { |
| final match = _START_TAG_REGEXP.firstMatch(html); |
| if (match != null) { |
| tag = match.group(1).toLowerCase(); |
| - if (_CUSTOM_PARENT_TAG_MAP.containsKey(tag)) { |
| - parentTag = _CUSTOM_PARENT_TAG_MAP[tag]; |
| + if (_TABLE_TAGS.containsKey(tag) && |
|
Emily Fortuna
2013/01/15 20:06:27
this is super nit-picky, but consider switching th
Jennifer Messerly
2013/01/15 20:41:21
Agreed. Will put _Device.isIE first.
|
| + window.navigator.userAgent.contains('MSIE')) { |
|
blois
2013/01/15 17:54:41
Should probably use _Device.isIE instead.
Jennifer Messerly
2013/01/15 20:11:28
Good catch! Will fix. I figured there must be a be
|
| + return _createTableForIE(html, tag); |
| } |
| + parentTag = _CUSTOM_PARENT_TAG_MAP[tag]; |
| + if (parentTag == null) parentTag = 'div'; |
| } |
| - final Element temp = new Element.tag(parentTag); |
| + |
| + final temp = new Element.tag(parentTag); |
| temp.innerHtml = html; |
| Element element; |
| if (temp.children.length == 1) { |
| element = temp.children[0]; |
| } else if (parentTag == 'html' && temp.children.length == 2) { |
| - // Work around for edge case in WebKit and possibly other browsers where |
| - // both body and head elements are created even though the inner html |
| - // only contains a head or body element. |
| + // In html5 the root <html> tag will always have a <body> and a <head>, |
| + // even though the inner html only contains one of them. |
| element = temp.children[tag == 'head' ? 0 : 1]; |
| } else { |
| - throw new ArgumentError('HTML had ${temp.children.length} ' |
| - 'top level elements but 1 expected'); |
| + _singleNode(temp.children); |
| } |
| element.remove(); |
| return element; |
| } |
| + /** |
| + * IE table elements don't support innerHTML (even in standards mode). |
| + * Instead we use a div and inject the table element in the innerHtml string. |
| + * This technique works on other browsers too, but it's probably slower, |
| + * so we only use it when running on IE. |
| + * |
| + * See also innerHTML: |
| + * <http://msdn.microsoft.com/en-us/library/ie/ms533897(v=vs.85).aspx> |
| + * and Building Tables Dynamically: |
| + * <http://msdn.microsoft.com/en-us/library/ie/ms532998(v=vs.85).aspx>. |
| + */ |
| + static Element _createTableForIE(String html, String tag) { |
|
blois
2013/01/15 17:54:41
Will be interesting when we move to SafeHTML.
Jennifer Messerly
2013/01/15 20:11:28
Agreed :)
|
| + var div = new Element.tag('div'); |
| + div.innerHtml = '<table>$html</table>'; |
| + var table = _singleNode(div.children); |
| + Element element; |
| + switch (tag) { |
| + case 'td': |
| + case 'th': |
| + element = _singleNode(_singleNode(table.rows).cells); |
| + break; |
| + case 'tr': element = _singleNode(table.rows); break; |
| + case 'tbody': element = _singleNode(table.tBodies); break; |
|
Emily Fortuna
2013/01/15 20:07:23
also, I'm not thrilled with the formatting of this
Jennifer Messerly
2013/01/15 20:41:21
will fix
|
| + case 'thead': element = table.tHead; break; |
| + case 'tfoot': element = table.tFoot; break; |
| + case 'caption': element = table.caption; break; |
| + case 'colgroup': element = _getColgroup(table); break; |
| + case 'col': |
| + element = _singleNode(_getColgroup(table).children); |
| + break; |
| + } |
| + element.remove(); |
| + return element; |
| + } |
| + |
| + static TableColElement _getColgroup(TableElement table) { |
| + // TODO(jmesserly): is there a better way to do this? |
| + return _singleNode(table.children.where((n) => n.tagName == 'COLGROUP') |
|
blois
2013/01/15 17:54:41
Use n is TableColElement instead? In general we ha
Jennifer Messerly
2013/01/15 20:11:28
I was thinking tagName might be faster than doing
blois
2013/01/15 20:47:51
Checking the tagname is fine. I believe that the '
|
| + .toList()); |
| + } |
| + |
| + static Node _singleNode(List<Node> list) { |
|
blois
2013/01/15 17:54:41
List.single instead?
Jennifer Messerly
2013/01/15 20:11:28
Thought about that. It would certainly be cleaner
blois
2013/01/15 20:47:51
I suppose it probably is one of the more common mi
|
| + if (list.length == 1) return list[0]; |
| + throw new ArgumentError('HTML had ${list.length} ' |
| + 'top level elements but 1 expected'); |
| + } |
| + |
| /** @domName Document.createElement */ |
| $if DART2JS |
| // Optimization to improve performance until the dart2js compiler inlines this |
