Fix Try Dart newline handling on IE11.

Fix Try Dart newline handling on IE11.

BUG=dartbug.com/20593
R=ahe@google.com, blois@google.com, efortuna@google.com

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

[aam-me, 2014-08-22 02:30:19]

Hi Peter,

  this is a follow-up to one try/web/cursor_position_test not passing for IE11.
Turns out the failing test reproduced an exception for IE11 in scenario when
editor contents is blank and user starts editing by hitting <Enter>. Useful!

  Please take a look if you have a chance.

Thanks!

[ahe, 2014-08-22 09:46:52]

It is things like this that make web programming hard :-(

This looks fine, but could you see if text.insertData would work instead? If so,
perhaps we can apply a fix to dart:html.

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
File site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:393: text.insertData(offset, newline);
Does this work the same in IE and Chrome? If so, can we use text.insertData
instead of appendText?

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:402: selection.collapse(node, 1);
How does this interact with getText in shadow_root.dart. In particular, I wonder
what would happen if IE supported shadow DOM. Concretely:

What is contents of node.text?

[aam-me, 2014-08-22 12:05:15]

Hi Peter,

  problem is that we don't have Text, only Div in that key handler in IE11. And
Div doesn't have insertData, only appendText. Or I am missing something?

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
File site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:393: text.insertData(offset, newline);
On 2014/08/22 09:46:52, ahe wrote:
> Does this work the same in IE and Chrome? If so, can we use text.insertData
> instead of appendText?

I believe it does, but appendText is on Element, while insertData is on Text. We
are using appendText because there is no Text available.

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:402: selection.collapse(node, 1);
On 2014/08/22 09:46:52, ahe wrote:
> How does this interact with getText in shadow_root.dart. 

By the time we get to that getText <br>'s are gone, replaced with Text('\n') in
normalizeMutationRecords.

> In particular, I wonder
> what would happen if IE supported shadow DOM. Concretely:
> 
> What is contents of node.text?

node is <div> with textContent='', innerHtml='<br><br>', and three children: BR,
BR, Text(with textContent.text='')

[ahe, 2014-08-22 12:13:44]

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
File site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:402: selection.collapse(node, 1);
On 2014/08/22 12:05:14, aam wrote:
> On 2014/08/22 09:46:52, ahe wrote:
> > How does this interact with getText in shadow_root.dart. 
> 
> By the time we get to that getText <br>'s are gone, replaced with Text('\n')
in
> normalizeMutationRecords.
> 
> > In particular, I wonder
> > what would happen if IE supported shadow DOM. Concretely:
> > 
> > What is contents of node.text?
> 
> node is <div> with textContent='', innerHtml='<br><br>', and three children:
BR,
> BR, Text(with textContent.text='')

In other words, this wouldn't work if IE supported shadow DOM :-(

Would this work:

node.append(new Text()..insertData('\n\n'));

[aam-me, 2014-08-22 13:42:30]

On 2014/08/22 12:13:44, ahe wrote:
>
https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
> File site/try/src/interaction_manager.dart (right):
> 
>
https://codereview.chromium.org/494253002/diff/1/site/try/src/interaction_man...
> site/try/src/interaction_manager.dart:402: selection.collapse(node, 1);
> On 2014/08/22 12:05:14, aam wrote:
> > On 2014/08/22 09:46:52, ahe wrote:
> > > How does this interact with getText in shadow_root.dart. 
> > 
> > By the time we get to that getText <br>'s are gone, replaced with Text('\n')
> in
> > normalizeMutationRecords.
> > 
> > > In particular, I wonder
> > > what would happen if IE supported shadow DOM. Concretely:
> > > 
> > > What is contents of node.text?
> > 
> > node is <div> with textContent='', innerHtml='<br><br>', and three children:
> BR,
> > BR, Text(with textContent.text='')
> 
> In other words, this wouldn't work if IE supported shadow DOM :-(
> 
> Would this work:
> 
> node.append(new Text()..insertData('\n\n'));

Yes, it would - Nice!
Please see latest patch(#2).

[ahe, 2014-08-22 13:49:01]

On 2014/08/22 13:42:30, aam wrote:
> Yes, it would - Nice!
> Please see latest patch(#2).

Should we try fixing this via dart:html then?

One has to modify these files:

tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate
tools/dom/templates/html/impl/impl_Element.darttemplate

And then run a script that doesn't work on Windows (tools/dom/scripts/go.sh).
Should I do it?

[ahe, 2014-08-22 13:50:10]

On 2014/08/22 13:49:01, ahe wrote:
> On 2014/08/22 13:42:30, aam wrote:
> > Yes, it would - Nice!
> > Please see latest patch(#2).
> 
> Should we try fixing this via dart:html then?
> 
> One has to modify these files:
> 
> tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate
> tools/dom/templates/html/impl/impl_Element.darttemplate
> 
> And then run a script that doesn't work on Windows (tools/dom/scripts/go.sh).
> Should I do it?

Or perhaps this should be fixed in
tools/dom/templates/html/impl/impl_Text.darttemplate.

[aam-me, 2014-08-23 03:17:12]

On 2014/08/22 13:50:10, ahe wrote:
> On 2014/08/22 13:49:01, ahe wrote:
> > On 2014/08/22 13:42:30, aam wrote:
> > > Yes, it would - Nice!
> > > Please see latest patch(#2).
> > 
> > Should we try fixing this via dart:html then?

Oh, that never occurred to me!

> > One has to modify these files:
> > tools/dom/templates/html/impl/impl_DocumentFragment.darttemplate

This one surprisingly already uses this.append(new Text(text))

> > tools/dom/templates/html/impl/impl_Element.darttemplate

Fixed this one(patch #3).

> > And then run a script that doesn't work on Windows
(tools/dom/scripts/go.sh).
> > Should I do it?
> Or perhaps this should be fixed in
> tools/dom/templates/html/impl/impl_Text.darttemplate.

Not sure I understand why this one.

[aam-me, 2014-08-23 03:18:04]

See follow-up comment regarding unrelated change picked up as a result of
regenerating from .darttemplate.

https://chromiumcodereview.appspot.com/494253002/diff/100001/sdk/lib/html/dar...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://chromiumcodereview.appspot.com/494253002/diff/100001/sdk/lib/html/dar...
sdk/lib/html/dartium/html_dartium.dart:28755: +    if ((blob_OR_source_OR_stream
is MediaSource || blob_OR_source_OR_stream == null)) {
These two ifs are swapped now - which correctly reflects order in
impl_URL.darttemplate. Not sure why in current version the order is different.

[ahe, 2014-08-26 08:25:37]

ahe@google.com changed reviewers:
+ blois@google.com, efortuna@google.com

[ahe, 2014-08-26 08:25:37]

This looks fine to me, but we should let Pete (blois) and Emily (efortuna)
decide if this is the right thing to do.

I wonder if we should add a regression test to tests/html/.

https://codereview.chromium.org/494253002/diff/100001/sdk/lib/html/dartium/ht...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/494253002/diff/100001/sdk/lib/html/dartium/ht...
sdk/lib/html/dartium/html_dartium.dart:28755: +    if ((blob_OR_source_OR_stream
is MediaSource || blob_OR_source_OR_stream == null)) {
On 2014/08/23 03:18:04, aam wrote:
> These two ifs are swapped now - which correctly reflects order in
> impl_URL.darttemplate. Not sure why in current version the order is different.

Normally, I discard changes that I haven't made.

[blois, 2014-08-26 15:44:28]

lgtm

[Emily Fortuna, 2014-08-26 16:34:15]

lgtm

[Emily Fortuna, 2014-08-26 16:35:27]

https://codereview.chromium.org/494253002/diff/100001/sdk/lib/html/dartium/ht...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/494253002/diff/100001/sdk/lib/html/dartium/ht...
sdk/lib/html/dartium/html_dartium.dart:28755: +    if ((blob_OR_source_OR_stream
is MediaSource || blob_OR_source_OR_stream == null)) {
On 2014/08/26 08:25:37, ahe wrote:
> On 2014/08/23 03:18:04, aam wrote:
> > These two ifs are swapped now - which correctly reflects order in
> > impl_URL.darttemplate. Not sure why in current version the order is
different.
> 
> Normally, I discard changes that I haven't made.

There is a bug tracking this somewhere; it seems to go back and forth on the
ordering depending on who generates it. You can commit it.

[aam-me, 2014-08-27 00:27:58]

Thank you Peter, Emily, Pete for the review!

On 2014/08/26 08:25:37, ahe wrote:
> This looks fine to me, but we should let Pete (blois) and Emily (efortuna)
> decide if this is the right thing to do.
> 
> I wonder if we should add a regression test to tests/html/.

Peter, please take a look at patchset #4 - is change to
tests/html/element_add_test.dart what you had in mind?

[ahe, 2014-08-27 07:38:30]

Nice! LGTM

Perhaps also test that el.text is "\n\n"?

[aam-me, 2014-08-27 11:41:04]

On 2014/08/27 07:38:30, ahe wrote:
> Nice! LGTM

Thanks!

> Perhaps also test that el.text is "\n\n"?

Like in patchset #5?

[ahe, 2014-08-27 11:48:48]

lgtm

https://codereview.chromium.org/494253002/diff/140001/tests/html/element_add_...
File tests/html/element_add_test.dart (right):

https://codereview.chromium.org/494253002/diff/140001/tests/html/element_add_...
tests/html/element_add_test.dart:86: expect(el.nodes[0].text,
equals(twoNewLines));
Suggest adding:

expect(el.text, equals(twoNewLines));

[aam-me, 2014-08-27 23:12:27]

Committed patchset #5 (id:140001) manually as r39626 (presubmit successful).

[aam-me, 2014-08-28 00:07:31]

On 2014/08/27 23:12:27, aam wrote:
> Committed patchset #5 (id:140001) manually as r39626 (presubmit successful).

After my commit there was one suspiciously looking build failure:
http://build.chromium.org/p/client.dart/builders/full-win7-ie10-2-4-be/builds...


But then I checked on my Windows8 IE10 machine and test passes(see below). Could
it be a flaky test? Should I revert the change?

===
C:\g\dart-repo\dart>python tools\test.py -mrelease --checked -cdart2js -rie10
pkg/template_binding/test/custom_element -v
Test configuration: dart2js_ie10_release_ia32_checked
Started HttpServers: C:\g\dart-repo\dart\\tools\testing\bin\windows\dart.exe
/C:/g/dart-repo/dart/tools/testing/dart/http_server.dart -p 57016 -c 57017
--build-directory=C:\g\dart-repo\dart\build\ReleaseIA32 --runtime=ie10
Running "dart2js" command: sdk\bin\dart2js.bat --enable-checked-mode
--package-root=build/ReleaseIA32/packages/
--out=C:/g/dart-repo/dart/build/ReleaseIA32/generated_tests/dart2js-ie10-checked/pkg_template_binding_test_custom_element_bindings_test/custom_element_bindings_test.js
C:\g\dart-repo\dart\pkg\template_binding
\test\custom_element_bindings_test.dart
Running "ie10" command: /C:/g/dart-repo/dart//tools/testing/bin/windows/dart.exe
tools/testing/dart/launch_browser.dart ie10
http://127.0.0.1:57016/root_build/generated_tests/dart2js-ie10-checked/pkg_te...
2014-08-27 20:03:05.707 Info: IE (BROWSER0) - Starting ie browser on:
http://127.0.0.1:57028/driver/BROWSER0

2014-08-27 20:03:05.723 Info: IE (BROWSER0) - Got version: 10.0.9200.17054

2014-08-27 20:03:05.786 Info: IE (BROWSER0) - Browser closed with exitcode 0

2014-08-27 20:03:06.358 Info: IE (BROWSER0) - Close called on browser

2014-08-27 20:03:06.358 Info: IE (BROWSER0) - The process is already dead.

Done dart2js-ie10-checked release_ia32
pkg/template_binding/test/custom_element_bindings_test: pass

C:\g\dart-repo\dart>svn info
Path: .
URL: https://dart.googlecode.com/svn/branches/bleeding_edge/dart
Repository Root: https://dart.googlecode.com/svn
Repository UUID: 260f80e4-7a28-3924-810f-c04153c831b5
Revision: 39626
Node Kind: directory
Schedule: normal
Last Changed Author: aprelev@gmail.com
Last Changed Rev: 39626
Last Changed Date: 2014-08-27 19:12:19 -0400 (Wed, 27 Aug 2014)
===

[aam-me, 2014-08-28 00:16:05]

On 2014/08/27 11:48:48, ahe wrote:
> Suggest adding:
> 
> expect(el.text, equals(twoNewLines));

Oops, sorry. Didn't notice this suggestion. Created
https://codereview.chromium.org/512043002/ for this.