Fix Try Dart handling of Backspace that joins lines

Fix Try Dart handling of Backspace that joins lines

BUG=dartbug.com/20731
R=ahe@google.com

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

[aam-me, 2014-09-04 10:37:46]

aprelev@gmail.com changed reviewers:
+ ahe@google.com

[aam-me, 2014-09-04 10:37:46]

Hi Peter,

  with [modify] fix in place this is the only thing remaining in Firefox
Backspace fix.
  Please take a look when you have a chance,

Thanks!

[ahe, 2014-09-04 11:42:37]

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

https://codereview.chromium.org/542623003/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:1237: if (first is Text && line.nextNode
!= null) {
Does this have a significant impact on when we can detect single line changes
when you hit backspace?

[aam-me, 2014-09-04 12:01:06]

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

https://codereview.chromium.org/542623003/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:1237: if (first is Text && line.nextNode
!= null) {
Your question is about impact in those cases when lines are not being merged?
Certainly there is impact(we always normalize current plus next line), not sure
how to measure it though.

We can try to limit this change to only those cases when we are certain lines
are being merged.(selection is at the end of the line/start of the line?)
Do you remember what was the rationale for checking first.data=='\n'? Must be
something along similar lines.

[ahe, 2014-09-04 12:11:10]

I sounds like you have found the right approach to this really annoying problem.

Can you add another test case to tests/try/web/cursor_position_test.dart?

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

https://codereview.chromium.org/542623003/diff/1/site/try/src/interaction_man...
site/try/src/interaction_manager.dart:1237: if (first is Text && line.nextNode
!= null) {
On 2014/09/04 12:01:06, aam wrote:
> Your question is about impact in those cases when lines are not being merged?
> Certainly there is impact(we always normalize current plus next line), not
sure
> how to measure it though.

I normally just look at the console and make some changes.

> We can try to limit this change to only those cases when we are certain lines
> are being merged.(selection is at the end of the line/start of the line?)
> Do you remember what was the rationale for checking first.data=='\n'? Must be
> something along similar lines.

It was an attempt to identify this problem more precisely.

Perhaps a better approach is to change the "single line change" to "consecutive
lines changed".

[aam-me, 2014-09-04 12:21:49]

On 2014/09/04 12:11:10, ahe wrote:
> I sounds like you have found the right approach to this really annoying
problem.
> 
> Can you add another test case to tests/try/web/cursor_position_test.dart?

Sure. I have looked at it briefly, but got stumbled on how to emulate Backspace
keydown.
Will look again.

[ahe, 2014-09-04 12:31:34]

On 2014/09/04 12:21:49, aam wrote:
> Sure. I have looked at it briefly, but got stumbled on how to emulate
Backspace
> keydown.
> Will look again.

Oh yeah. That was the same problem I had.

[ahe, 2014-09-04 12:42:20]

On 2014/09/04 12:31:34, ahe wrote:
> On 2014/09/04 12:21:49, aam wrote:
> > Sure. I have looked at it briefly, but got stumbled on how to emulate
> Backspace
> > keydown.
> > Will look again.
> 
> Oh yeah. That was the same problem I had.

So perhaps you could make a test based on AppleScript?

[ahe, 2014-09-05 09:52:37]

On 2014/09/04 12:42:20, ahe wrote:
> On 2014/09/04 12:31:34, ahe wrote:
> > On 2014/09/04 12:21:49, aam wrote:
> > > Sure. I have looked at it briefly, but got stumbled on how to emulate
> > Backspace
> > > keydown.
> > > Will look again.
> > 
> > Oh yeah. That was the same problem I had.
> 
> So perhaps you could make a test based on AppleScript?

I think this is a fairly good approximation to backspace:

window.getSelection()
    ..modify('extend', 'backward', 'character')
    ..deleteFromDocument();

But as we already know, modify doesn't work in IE. Perhaps there is an
alternative. I'll keep looking.

[ahe, 2014-09-05 09:59:37]

On 2014/09/05 09:52:37, ahe wrote:
> But as we already know, modify doesn't work in IE. Perhaps there is an
> alternative. I'll keep looking.

Perhaps we can polyfill Selection.modify using something like:

window.getSelection().createRange().moveStart('character', -1)

[aam-me, 2014-09-05 11:06:10]

> So perhaps you could make a test based on AppleScript?

Hi Peter, 

  can you please take a look at firefox-linemerges.applescript I added in patch
set #2?
  There are several differences in setup between this and original
firefox.applescript(no activate, incognito window, example selection).
  Not sure whether it should be merged into firefox.applescript. Also copied
into safari.applescript.

Thanks!

[ahe, 2014-09-05 12:08:25]

On 2014/09/05 09:59:37, ahe wrote:
> window.getSelection().createRange().moveStart('character', -1)

That doesn't work :-(

IE11 removed window.selection, but window.getSelection() isn't compatible.

So far I've managed to create a TextRange, but I'm having a hard time
initializing it to the selection.

[ahe, 2014-09-05 13:10:44]

LGTM

https://codereview.chromium.org/542623003/diff/20001/tests/try/firefox-lineme...
File tests/try/firefox-linemerges.applescript (right):

https://codereview.chromium.org/542623003/diff/20001/tests/try/firefox-lineme...
tests/try/firefox-linemerges.applescript:33: repeat 8 times
Fancy stuff :-)

[aam-me, 2014-09-05 21:03:36]

Committed patchset #2 (id:20001) manually as 39930 (presubmit successful).

[aam-me, 2014-09-08 05:09:56]

On 2014/09/05 12:08:25, ahe wrote:
> On 2014/09/05 09:59:37, ahe wrote:
> > window.getSelection().createRange().moveStart('character', -1)
> 
> That doesn't work :-(
> 
> IE11 removed window.selection, but window.getSelection() isn't compatible.
> 
> So far I've managed to create a TextRange, but I'm having a hard time
> initializing it to the selection.

I managed to write simple backspace test passing on IE11 using TextRange
moveToPoint/moveStart/execCommand:

=== JS ===
      var range = window.getSelection().getRangeAt(0).cloneRange();
      range.collapse(true);
      var rects = range.getClientRects();
      if (rects.length > 0) {
        var rect = range.getClientRects()[0];
        var r = document.body.createTextRange();
        r.moveToPoint(rect.left, rect.top);
        r.moveStart("character", -1);
        r.execCommand("Delete");
      }
=== end of JS ===

=== Dart ===
import 'dart:js' as hack;
...
        var sel = window.getSelection();
        var range = sel.getRangeAt(0).cloneRange();
        range.collapse(true);
        var rects = range.getClientRects();
        if (rects.length > 0) {
          var rect = rects[0];
          var d = hack.context['document'];
          var b = new hack.JsObject.fromBrowserObject(d.body);
          var r = b.callMethod('createTextRange');

          r.callMethod("moveToPoint", [rect.left, rect.top]);
          r.callMethod("moveStart", ["character", -1]);
          r.callMethod("execCommand", ["Delete"]);
        }
      }
=== end of Dart ===

[ahe, 2014-09-08 08:30:25]

On 2014/09/08 05:09:56, aam wrote:
> On 2014/09/05 12:08:25, ahe wrote:
> > On 2014/09/05 09:59:37, ahe wrote:
> > > window.getSelection().createRange().moveStart('character', -1)
> > 
> > That doesn't work :-(
> > 
> > IE11 removed window.selection, but window.getSelection() isn't compatible.
> > 
> > So far I've managed to create a TextRange, but I'm having a hard time
> > initializing it to the selection.
> 
> I managed to write simple backspace test passing on IE11 using TextRange
> moveToPoint/moveStart/execCommand:
> 
> === JS ===
>       var range = window.getSelection().getRangeAt(0).cloneRange();
>       range.collapse(true);
>       var rects = range.getClientRects();
>       if (rects.length > 0) {
>         var rect = range.getClientRects()[0];
>         var r = document.body.createTextRange();
>         r.moveToPoint(rect.left, rect.top);
>         r.moveStart("character", -1);
>         r.execCommand("Delete");
>       }
> === end of JS ===
> 
> === Dart ===
> import 'dart:js' as hack;
> ...
>         var sel = window.getSelection();
>         var range = sel.getRangeAt(0).cloneRange();
>         range.collapse(true);
>         var rects = range.getClientRects();
>         if (rects.length > 0) {
>           var rect = rects[0];
>           var d = hack.context['document'];
>           var b = new hack.JsObject.fromBrowserObject(d.body);
>           var r = b.callMethod('createTextRange');
> 
>           r.callMethod("moveToPoint", [rect.left, rect.top]);
>           r.callMethod("moveStart", ["character", -1]);
>           r.callMethod("execCommand", ["Delete"]);
>         }
>       }
> === end of Dart ===

Awesome. We can also polyfill Selection.modify using this approach. I always got
"unspecified error" when calling "moveStart", but perhaps that is because I
called it from the developer console.