tools/dom/templates/html/impl/impl_Element.darttemplate - Issue 11888019: Fix new Element.html with tables in IE

Side by Side Diff: tools/dom/templates/html/impl/impl_Element.darttemplate

Issue 11888019: Fix new Element.html with tables in IE (Closed) Base URL: https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Patch Set: Created 7 years, 11 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

OLD	NEW
1 // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file	1 // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file

2 // for details. All rights reserved. Use of this source code is governed by a	2 // for details. All rights reserved. Use of this source code is governed by a

3 // BSD-style license that can be found in the LICENSE file.	3 // BSD-style license that can be found in the LICENSE file.

4	4

5 part of html;	5 part of html;

6	6

7 // TODO(jacobr): use _Lists.dart to remove some of the duplicated	7 // TODO(jacobr): use _Lists.dart to remove some of the duplicated

8 // functionality.	8 // functionality.

9 class _ChildrenElementList implements List {	9 class _ChildrenElementList implements List {

10 // Raw Element.	10 // Raw Element.

(...skipping 828 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
839 }	839 }

840 }	840 }

841 $else	841 $else

842 $endif	842 $endif

843	843

844 $!MEMBERS	844 $!MEMBERS

845 }	845 }

846	846

847 final _START_TAG_REGEXP = new RegExp('<(\\w+)');	847 final _START_TAG_REGEXP = new RegExp('<(\\w+)');

848 class _ElementFactoryProvider {	848 class _ElementFactoryProvider {

849 static final _CUSTOM_PARENT_TAG_MAP = const {	849 static const _CUSTOM_PARENT_TAG_MAP = const {

850 'body' : 'html',	850 'body' : 'html',

851 'head' : 'html',	851 'head' : 'html',

852 'caption' : 'table',	852 'caption' : 'table',

853 'td': 'tr',	853 'td': 'tr',

	854 'th': 'tr',

854 'colgroup': 'table',	855 'colgroup': 'table',

855 'col' : 'colgroup',	856 'col' : 'colgroup',

856 'tr' : 'tbody',	857 'tr' : 'tbody',

857 'tbody' : 'table',	858 'tbody' : 'table',

858 'tfoot' : 'table',	859 'tfoot' : 'table',

859 'thead' : 'table',	860 'thead' : 'table',

860 'track' : 'audio',	861 'track' : 'audio',

861 };	862 };

862	863

	864 // TODO(jmesserly): const set would be better

	865 static const _TABLE_TAGS = const {

	866 'caption': null,

	867 'col': null,

	868 'colgroup': null,

	869 'tbody': null,

	870 'td': null,

	871 'tfoot': null,

	872 'th': null,

	873 'thead': null,

	874 'tr': null,

	875 };

	876

863 /** @domName Document.createElement */	877 /** @domName Document.createElement */

864 static Element createElement_html(String html) {	878 static Element createElement_html(String html) {

865 // TODO(jacobr): this method can be made more robust and performant.	879 // TODO(jacobr): this method can be made more robust and performant.

866 // 1) Cache the dummy parent elements required to use innerHTML rather than	880 // 1) Cache the dummy parent elements required to use innerHTML rather than

867 // creating them every call.	881 // creating them every call.

868 // 2) Verify that the html does not contain leading or trailing text nodes.	882 // 2) Verify that the html does not contain leading or trailing text nodes.

869 // 3) Verify that the html does not contain both <head> and <body> tags.	883 // 3) Verify that the html does not contain both <head> and <body> tags.

870 // 4) Detatch the created element from its dummy parent.	884 // 4) Detatch the created element from its dummy parent.

871 String parentTag = 'div';	885 String parentTag = 'div';

872 String tag;	886 String tag;

873 final match = _START_TAG_REGEXP.firstMatch(html);	887 final match = _START_TAG_REGEXP.firstMatch(html);

874 if (match != null) {	888 if (match != null) {

875 tag = match.group(1).toLowerCase();	889 tag = match.group(1).toLowerCase();

876 if (_CUSTOM_PARENT_TAG_MAP.containsKey(tag)) {	890 if (_TABLE_TAGS.containsKey(tag) &&
	Emily Fortuna 2013/01/15 20:06:27 this is super nit-picky, but consider switching th 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. Jennifer Messerly 2013/01/15 20:41:21 Agreed. Will put _Device.isIE first. Show quoted text 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.
877 parentTag = _CUSTOM_PARENT_TAG_MAP[tag];	891 window.navigator.userAgent.contains('MSIE')) {
	blois 2013/01/15 17:54:41 Should probably use _Device.isIE instead. 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 Show quoted text 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 :)
	892 return _createTableForIE(html, tag);

878 }	893 }

	894 parentTag = _CUSTOM_PARENT_TAG_MAP[tag];

	895 if (parentTag == null) parentTag = 'div';

879 }	896 }

880 final Element temp = new Element.tag(parentTag);	897

	898 final temp = new Element.tag(parentTag);

881 temp.innerHtml = html;	899 temp.innerHtml = html;

882	900

883 Element element;	901 Element element;

884 if (temp.children.length == 1) {	902 if (temp.children.length == 1) {

885 element = temp.children[0];	903 element = temp.children[0];

886 } else if (parentTag == 'html' && temp.children.length == 2) {	904 } else if (parentTag == 'html' && temp.children.length == 2) {

887 // Work around for edge case in WebKit and possibly other browsers where	905 // In html5 the root <html> tag will always have a <body> and a <head>,

888 // both body and head elements are created even though the inner html	906 // even though the inner html only contains one of them.

889 // only contains a head or body element.

890 element = temp.children[tag == 'head' ? 0 : 1];	907 element = temp.children[tag == 'head' ? 0 : 1];

891 } else {	908 } else {

892 throw new ArgumentError('HTML had ${temp.children.length} '	909 _singleNode(temp.children);

893 'top level elements but 1 expected');

894 }	910 }

895 element.remove();	911 element.remove();

896 return element;	912 return element;

897 }	913 }

898	914

	915 /**

	916 * IE table elements don't support innerHTML (even in standards mode).

	917 * Instead we use a div and inject the table element in the innerHtml string.

	918 * This technique works on other browsers too, but it's probably slower,

	919 * so we only use it when running on IE.

	920 *

	921 * See also innerHTML:

	922 * <http://msdn.microsoft.com/en-us/library/ie/ms533897(v=vs.85).aspx>

	923 * and Building Tables Dynamically:

	924 * <http://msdn.microsoft.com/en-us/library/ie/ms532998(v=vs.85).aspx>.

	925 */

	926 static Element _createTableForIE(String html, String tag) {
	blois 2013/01/15 17:54:41 Will be interesting when we move to SafeHTML. Will be interesting when we move to SafeHTML. Jennifer Messerly 2013/01/15 20:11:28 Agreed :) Show quoted text On 2013/01/15 17:54:41, blois wrote: > Will be interesting when we move to SafeHTML. Agreed :)
	927 var div = new Element.tag('div');

	928 div.innerHtml = '<table>$html</table>';

	929 var table = _singleNode(div.children);

	930 Element element;

	931 switch (tag) {

	932 case 'td':

	933 case 'th':

	934 element = _singleNode(_singleNode(table.rows).cells);

	935 break;

	936 case 'tr': element = _singleNode(table.rows); break;

	937 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 also, I'm not thrilled with the formatting of this switch statement, but I can be persuaded otherwise. Jennifer Messerly 2013/01/15 20:41:21 will fix Show quoted text 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
	938 case 'thead': element = table.tHead; break;

	939 case 'tfoot': element = table.tFoot; break;

	940 case 'caption': element = table.caption; break;

	941 case 'colgroup': element = _getColgroup(table); break;

	942 case 'col':

	943 element = _singleNode(_getColgroup(table).children);

	944 break;

	945 }

	946 element.remove();

	947 return element;

	948 }

	949

	950 static TableColElement _getColgroup(TableElement table) {

	951 // TODO(jmesserly): is there a better way to do this?

	952 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 Use n is TableColElement instead? In general we haven't been using tagName much at all. Alternatively, table.query('colgroup')? Jennifer Messerly 2013/01/15 20:11:28 I was thinking tagName might be faster than doing Show quoted text 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 Show quoted text > 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) blois 2013/01/15 20:47:51 Checking the tagname is fine. I believe that the ' Show quoted text 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).
	953 .toList());

	954 }

	955

	956 static Node _singleNode(List<Node> list) {
	blois 2013/01/15 17:54:41 List.single instead? List.single instead? Jennifer Messerly 2013/01/15 20:11:28 Thought about that. It would certainly be cleaner Show quoted text 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? blois 2013/01/15 20:47:51 I suppose it probably is one of the more common mi Show quoted text 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.
	957 if (list.length == 1) return list[0];

	958 throw new ArgumentError('HTML had ${list.length} '

	959 'top level elements but 1 expected');

	960 }

	961

899 /** @domName Document.createElement */	962 /** @domName Document.createElement */

900 $if DART2JS	963 $if DART2JS

901 // Optimization to improve performance until the dart2js compiler inlines this	964 // Optimization to improve performance until the dart2js compiler inlines this

902 // method.	965 // method.

903 static dynamic createElement_tag(String tag) =>	966 static dynamic createElement_tag(String tag) =>

904 JS('Element', 'document.createElement(#)', tag);	967 JS('Element', 'document.createElement(#)', tag);

905 $else	968 $else

906 static Element createElement_tag(String tag) =>	969 static Element createElement_tag(String tag) =>

907 document.$dom_createElement(tag);	970 document.$dom_createElement(tag);

908 $endif	971 $endif

909 }	972 }

OLD	NEW

« no previous file with comments | « tests/html/element_test.dart ('k') | no next file » | no next file with comments »