Fix issues that broke editing on browsers without Shadow DOM support.

Fix issues that broke editing on browsers without Shadow DOM support.

R=johnniwinther@google.com

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

[ahe, 2014-06-27 11:23:28]

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/decora...
File dart/site/try/src/decoration.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/decora...
dart/site/try/src/decoration.dart:71: ..append(tip)
Assume the user have written "foo" and we want to show a diagnostic around
"foo".  We have two options:

1. <a class="diagnostic">foo<span>tip</span></a>

2. <a class="diagnostic"><span>tip</span>foo</a>

Turns out that option 1 doesn't work well with backspace in Firefox. So option 2
it is :-)

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/html_t...
File dart/site/try/src/html_to_text.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/html_t...
dart/site/try/src/html_to_text.dart:41: int htmlToText(Node root,
This code was working mostly fine, but it has been refactored to be reused by
selection.dart.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/html_t...
dart/site/try/src/html_to_text.dart:47: if (selection.anchorNode == node) {
This code was moved here, it relied on an assumption which only holds in Chrome:
the anchorNode is always CharacterData.  That doesn't hold for Firefox and
Safari.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
File dart/site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:425: if
(mainEditorPane.contains(trySelection.anchorNode)) {
This sometimes happens on Firefox. It is related to when you delete the last
character in a line, but it isn't deterministic.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:1151: if (record.type ==
"characterData" && record.target.parent != null) {
Firefox sends two records when the last character of a text node was deleted.

1. Stating that the text node was removed from its parent element (the text node
is in record.removedNodes, and record.target is the parent).

2. A "characterData" record with the empty text node as target.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:1210: if (selection.anchorNode is
Element && selection.anchorOffset != 0) {
In some cases, Firefox reports the wrong anchorOffset (always seems to be 6)
when anchorNode is an Element. I've discovered that moving the cursor back and
forth fixes that.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/select...
File dart/site/try/src/selection.dart (left):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/select...
dart/site/try/src/selection.dart:78: if (anchorNode == text) {
This assumed that the anchorNode would always be a text node. Naturally, the
globalOffset was wrong, and consequently the cursor would jump "randomly" around
when I added syntax highlighting.

[Johnni Winther, 2014-07-02 08:40:54]

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/decora...
File dart/site/try/src/decoration.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/decora...
dart/site/try/src/decoration.dart:71: ..append(tip)
On 2014/06/27 11:23:27, ahe wrote:
> Assume the user have written "foo" and we want to show a diagnostic around
> "foo".  We have two options:
> 
> 1. <a class="diagnostic">foo<span>tip</span></a>
> 
> 2. <a class="diagnostic"><span>tip</span>foo</a>
> 
> Turns out that option 1 doesn't work well with backspace in Firefox. So option
2
> it is :-)

Put this explanation as comment in the code.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
File dart/site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:425: if
(mainEditorPane.contains(trySelection.anchorNode)) {
On 2014/06/27 11:23:28, ahe wrote:
> This sometimes happens on Firefox. It is related to when you delete the last
> character in a line, but it isn't deterministic.

Put this in a comment.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:1151: if (record.type ==
"characterData" && record.target.parent != null) {
On 2014/06/27 11:23:28, ahe wrote:
> Firefox sends two records when the last character of a text node was deleted.
> 
> 1. Stating that the text node was removed from its parent element (the text
node
> is in record.removedNodes, and record.target is the parent).
> 
> 2. A "characterData" record with the empty text node as target.

Put this in a comment.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:1210: if (selection.anchorNode is
Element && selection.anchorOffset != 0) {
On 2014/06/27 11:23:28, ahe wrote:
> In some cases, Firefox reports the wrong anchorOffset (always seems to be 6)
> when anchorNode is an Element. I've discovered that moving the cursor back and
> forth fixes that.

Again, put this in a comment.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/shadow...
File dart/site/try/src/shadow_root.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/shadow...
dart/site/try/src/shadow_root.dart:92: if (action == WALKER_SKIP_NODE)
skip(node, walker);
'continue' after skip to avoid the assertion of `action == WALKER_NEXT` ?

[Johnni Winther, 2014-07-02 09:03:31]

lgtm

[ahe, 2014-07-04 13:52:01]

Thank you, Johnni!

I'm submitting this now, but I'd appreciate if you could take another look at
the last patch set after the fact.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/decora...
File dart/site/try/src/decoration.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/decora...
dart/site/try/src/decoration.dart:71: ..append(tip)
On 2014/07/02 08:40:53, Johnni Winther wrote:
> On 2014/06/27 11:23:27, ahe wrote:
> > Assume the user have written "foo" and we want to show a diagnostic around
> > "foo".  We have two options:
> > 
> > 1. <a class="diagnostic">foo<span>tip</span></a>
> > 
> > 2. <a class="diagnostic"><span>tip</span>foo</a>
> > 
> > Turns out that option 1 doesn't work well with backspace in Firefox. So
option
> 2
> > it is :-)
> 
> Put this explanation as comment in the code.

Done.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
File dart/site/try/src/interaction_manager.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:425: if
(mainEditorPane.contains(trySelection.anchorNode)) {
On 2014/07/02 08:40:54, Johnni Winther wrote:
> On 2014/06/27 11:23:28, ahe wrote:
> > This sometimes happens on Firefox. It is related to when you delete the last
> > character in a line, but it isn't deterministic.
> 
> Put this in a comment.

Done.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:1151: if (record.type ==
"characterData" && record.target.parent != null) {
On 2014/07/02 08:40:54, Johnni Winther wrote:
> On 2014/06/27 11:23:28, ahe wrote:
> > Firefox sends two records when the last character of a text node was
deleted.
> > 
> > 1. Stating that the text node was removed from its parent element (the text
> node
> > is in record.removedNodes, and record.target is the parent).
> > 
> > 2. A "characterData" record with the empty text node as target.
> 
> Put this in a comment.

Done.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/intera...
dart/site/try/src/interaction_manager.dart:1210: if (selection.anchorNode is
Element && selection.anchorOffset != 0) {
On 2014/07/02 08:40:54, Johnni Winther wrote:
> On 2014/06/27 11:23:28, ahe wrote:
> > In some cases, Firefox reports the wrong anchorOffset (always seems to be 6)
> > when anchorNode is an Element. I've discovered that moving the cursor back
and
> > forth fixes that.
> 
> Again, put this in a comment.

Done.

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/shadow...
File dart/site/try/src/shadow_root.dart (right):

https://codereview.chromium.org/345553008/diff/20001/dart/site/try/src/shadow...
dart/site/try/src/shadow_root.dart:92: if (action == WALKER_SKIP_NODE)
skip(node, walker);
On 2014/07/02 08:40:54, Johnni Winther wrote:
> 'continue' after skip to avoid the assertion of `action == WALKER_NEXT` ?

I turned it into a switch.

https://codereview.chromium.org/345553008/diff/80001/dart/site/try/src/select...
File dart/site/try/src/selection.dart (right):

https://codereview.chromium.org/345553008/diff/80001/dart/site/try/src/select...
dart/site/try/src/selection.dart:43: if (start == end) return null;
This check prevents adding empty Text nodes. I had originally removed it in an
attempt to avoid the check added to line 455 in interaction_manager.dart. But it
wasn't sufficient.

The extra empty text node broke checkAtEndOfSecondLineWithFisk in
tests/try/cursor_position_test.dart, in particular this line:

  Text text = secondLine.firstChild.firstChild;

secondLine.firstChild is now the empty text node, not <span>fisk</span> as
expected.

https://codereview.chromium.org/345553008/diff/80001/dart/site/try/src/shadow...
File dart/site/try/src/shadow_root.dart (right):

https://codereview.chromium.org/345553008/diff/80001/dart/site/try/src/shadow...
dart/site/try/src/shadow_root.dart:100: throw 'Unexpected action returned from
[f]: $action';
This revealed that I had forgotten a return statement in selection.dart.

https://codereview.chromium.org/345553008/diff/80001/dart/tests/try/paste_con...
File dart/tests/try/paste_content_rewriting_test.dart (right):

https://codereview.chromium.org/345553008/diff/80001/dart/tests/try/paste_con...
dart/tests/try/paste_content_rewriting_test.dart:50: node.replaceWith(new
Text('<DIAGNOSTIC>'));
This code replaces diagnostics with <DIAGNOSTIC>...</DIAGNOSTIC>.

Before it was operating on:

<a>...<span>tip</span></a>

Now it looks like:

<a><span>tip</span>...</a>

[ahe, 2014-07-04 13:53:47]

Committed patchset #3 manually as r38014 (presubmit successful).

[Johnni Winther, 2014-07-06 08:27:53]

Still LGTM