Make character text changes work in Docs

Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a reference to any node cannot use it in any logic.

To work around this issue, add a new RecoveryStrategy class along with subclasses that implement specific recovery strategies.

One such strategy is based on the tree path of the node and is useful here.

When requested, the recovery can compute the recovered node for the invalidated node.

In EditableLine, add a new member to track the line start container using a tree path recovery strategy.

When determining whether two lines are equal, use the recovered node to decide if the two lines are the same along with the local start offset.

TEST=type into Docs; verify character echo works.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2971913003
Cr-Commit-Position: refs/heads/master@{#485062}
Committed: https://chromium.googlesource.com/chromium/src/+/a28aeb7be0f77767683a8157ab9277a3dd110154

[David Tseng, 2017-07-06 20:52:07]

Description was changed from

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.

Provide complete, smart, spoken feedback for editable selection

BUG=
==========

to

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.

Provide complete, smart, spoken feedback for editable selection

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-07-06 20:52:29]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[David Tseng, 2017-07-06 20:54:20]

Description was changed from

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.

Provide complete, smart, spoken feedback for editable selection

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.
==========

[David Tseng, 2017-07-06 20:54:39]

Description was changed from

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.
==========

to

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-07-06 20:54:53]

Patchset #1 (id:1) has been deleted

[dmazzoni, 2017-07-06 21:17:07]

https://codereview.chromium.org/2971913003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
(right):

https://codereview.chromium.org/2971913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:110:
this.recoveryChildIndex_.push(nodeWalker.indexInParent);
I like this idea in general but how about a tiny bit of
redundant information, in order to avoid "recovering"
a completely unrelated part of the document?

I'm thinking something like saving the role and name for
every item in the hierarchy in addition to its index in
parent - then the recovery could reject something that's
clearly the wrong node. In the future we could consider
making it smart about a fuzzy match - but that's beyond
the scope for now.

Also, how about abstracting this idea into a general
helper class outside of cursor or editable text?
We should use something similar to save and restore
your position within a document when reloading a page,
for example.

[David Tseng, 2017-07-06 22:15:44]

https://codereview.chromium.org/2971913003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
(right):

https://codereview.chromium.org/2971913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:110:
this.recoveryChildIndex_.push(nodeWalker.indexInParent);
On 2017/07/06 21:17:06, dmazzoni wrote:
> I like this idea in general but how about a tiny bit of
> redundant information, in order to avoid "recovering"
> a completely unrelated part of the document?
> 
> I'm thinking something like saving the role and name for
> every item in the hierarchy in addition to its index in
> parent - then the recovery could reject something that's
> clearly the wrong node. In the future we could consider
> making it smart about a fuzzy match - but that's beyond
> the scope for now.
> 
> Also, how about abstracting this idea into a general
> helper class outside of cursor or editable text?
> We should use something similar to save and restore
> your position within a document when reloading a page,
> for example.

I'm actually something slightly different here that requires the Cursor class.

From experience, saving the absolute screen x, y position gives a better
recovery for web pages. Tree paths perform really poorly from what I've seen in
general so I do think we want it only for editing and specifically for line
comparisons. I can move it out of Cursor, but I also put it there since it has
both a node and index member. The index member helps restrict the possible false
positives since a static text often has soft line wraps so we want to correctly
account for that. Given two soft lines, we don't want to consider them the same
line even if all roles/tree paths matched but the local line start container
offsets don't.

[dmazzoni, 2017-07-06 22:43:40]

OK, here's what I'm thinking: keep your current logic,
but instead of every cursor having its recovery data
inside, how about an API to serialize a cursor, and
recover a cursor, like:

var previousCursorData = Cursor.serialize();
...
var previousCursor = Cursor.recoverFromSerialized(previousCursorData);

That way the caller owns the serialization and
can decide when to recover, we can more easily
unit-test recovery, and we can experiment with
different recovery strategies independent of the
editing code.

lgtm with some change along those lines

[David Tseng, 2017-07-07 17:06:44]

On 2017/07/06 22:43:40, dmazzoni wrote:
> OK, here's what I'm thinking: keep your current logic,
> but instead of every cursor having its recovery data
> inside, how about an API to serialize a cursor, and
> recover a cursor, like:
> 
> var previousCursorData = Cursor.serialize();
> ...
> var previousCursor = Cursor.recoverFromSerialized(previousCursorData);
> 
> That way the caller owns the serialization and
> can decide when to recover, we can more easily
> unit-test recovery, and we can experiment with
> different recovery strategies independent of the
> editing code.

This isn't the problem we're solving. I've uploaded a patchset to demonstrate.
I've introduced a RecoveryStrategy base class that shows what we're going for.

The ancestry recovery subclass which you basically implemented in Cursor is an
example of this type of recovery. Tree path recovery is another.

Serialization is only needed for a screen position type of recovery.

The callers in this case, Cursor and EditableLine, both always want nodes that
have been invalidated to be recovered no matter what. It doesn't make sense to
serialize data only to always have to unserialize. The only difference is Cursor
wants ancestry recovery while EditableLine needs something stronger ancestry +
tree path recovery.

[David Tseng, 2017-07-07 17:08:42]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 17:08:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2017-07-07 17:52:15]

On 2017/07/07 17:06:44, David Tseng wrote:
> On 2017/07/06 22:43:40, dmazzoni wrote:
> > OK, here's what I'm thinking: keep your current logic,
> > but instead of every cursor having its recovery data
> > inside, how about an API to serialize a cursor, and
> > recover a cursor, like:
> > 
> > var previousCursorData = Cursor.serialize();
> > ...
> > var previousCursor = Cursor.recoverFromSerialized(previousCursorData);
> > 
> > That way the caller owns the serialization and
> > can decide when to recover, we can more easily
> > unit-test recovery, and we can experiment with
> > different recovery strategies independent of the
> > editing code.
> 
> This isn't the problem we're solving. I've uploaded a patchset to demonstrate.
> I've introduced a RecoveryStrategy base class that shows what we're going for.
> 
> The ancestry recovery subclass which you basically implemented in Cursor is an
> example of this type of recovery. Tree path recovery is another.
> 
> Serialization is only needed for a screen position type of recovery.
> 
> The callers in this case, Cursor and EditableLine, both always want nodes that
> have been invalidated to be recovered no matter what. It doesn't make sense to
> serialize data only to always have to unserialize. The only difference is
Cursor
> wants ancestry recovery while EditableLine needs something stronger ancestry +
> tree path recovery.

Also, note the new test.

I'll submit if you have no problems with this.

I'll also start a thread offline about DOM re-parenting.

[David Tseng, 2017-07-07 17:52:23]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 17:52:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2017-07-07 19:56:31]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 19:56:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Tseng, 2017-07-07 21:03:30]

Description was changed from

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, keep a list of node index in parent within
ChromeVox's Cursor object which parallels the ancestry list.

When requested, the cursor can compute the recovered node for the cursor.

In EditableLine, add a new member to track the line start container using a
cursor that also tracks the local line start container offset (basically the
index given by Blink).

When determining whether two lines are equal, use the new start cursor and its
recovered node to decide if the two lines are the same.

TEST=type into Docs; verify character echo works.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, add a new RecoveryStrategy class along with
subclasses that implement specific recovery strategies.

One such strategy is based on the tree path of the node and is useful here.

When requested, the recovery can compute the recovered node for the invalidated
node.

In EditableLine, add a new member to track the line start container using a tree
path recovery strategy.

When determining whether two lines are equal, use the recovered node to decide
if the two lines are the same along with the local start offset.

TEST=type into Docs; verify character echo works.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2017-07-07 21:06:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 21:06:48]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-07-07 21:44:46]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-07-07 21:44:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2971913003/#ps80001
(title: "Rename test and make it slightly more complex.")

[commit-bot: I haz the power, 2017-07-07 21:45:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 21:51:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499463886034580,
"parent_rev": "3e3c14af2dcee49b2ed00dc9960f474b7d69f7ac", "commit_rev":
"a28aeb7be0f77767683a8157ab9277a3dd110154"}

[commit-bot: I haz the power, 2017-07-07 21:51:21]

Description was changed from

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, add a new RecoveryStrategy class along with
subclasses that implement specific recovery strategies.

One such strategy is based on the tree path of the node and is useful here.

When requested, the recovery can compute the recovered node for the invalidated
node.

In EditableLine, add a new member to track the line start container using a tree
path recovery strategy.

When determining whether two lines are equal, use the recovered node to decide
if the two lines are the same along with the local start offset.

TEST=type into Docs; verify character echo works.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make character text changes work in Docs

In "braille mode", Docs re-exports the entire paragraph if you type.

For a sample tree:
text field (paragraph (static text (inline text box)))

everything except for the text field gets destroyed. Thus, any client holding a
reference to any node cannot use it in any logic.

To work around this issue, add a new RecoveryStrategy class along with
subclasses that implement specific recovery strategies.

One such strategy is based on the tree path of the node and is useful here.

When requested, the recovery can compute the recovered node for the invalidated
node.

In EditableLine, add a new member to track the line start container using a tree
path recovery strategy.

When determining whether two lines are equal, use the recovered node to decide
if the two lines are the same along with the local start offset.

TEST=type into Docs; verify character echo works.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2971913003
Cr-Commit-Position: refs/heads/master@{#485062}
Committed:
https://chromium.googlesource.com/chromium/src/+/a28aeb7be0f77767683a8157ab92...
==========

[commit-bot: I haz the power, 2017-07-07 21:51:23]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a28aeb7be0f77767683a8157ab92...