Rich editable text implementation using spannables

Rich editable text implementation using spannables
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[David Tseng, 2017-05-24 21:58:48]

Description was changed from

==========
Incremental support for rich character output

- use next/previous on line to track state changes in rich editable text
- add a few more workarounds for bugs

TEST=navigate simple snippet by line and character. Verify output is mostly
correct when measured against plain text behavior.
BUG=
==========

to

==========
Incremental support for rich character output

- use next/previous on line to track state changes in rich editable text
- add a few more workarounds for bugs

TEST=navigate simple snippet by line and character. Verify output is mostly
correct when measured against plain text behavior.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-05-24 21:59:17]

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

[David Tseng, 2017-05-25 20:45:07]

Description was changed from

==========
Incremental support for rich character output

- use next/previous on line to track state changes in rich editable text
- add a few more workarounds for bugs

TEST=navigate simple snippet by line and character. Verify output is mostly
correct when measured against plain text behavior.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Rich editable text implementation using spannables
==========

[David Tseng, 2017-05-25 20:46:54]

FYI; I reworked this cl to use spannables and hopefully provide a better
foundation with a data structure that will be easier to write output rules for.
Rather than dealing with the tree representation, I'm now computing the text
content of a line and annotating with nodes.

[dmazzoni, 2017-05-25 21:00:37]

Cool idea, I like the idea of EditableLine.

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

https://codereview.chromium.org/2903973002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:91: DIV
&& node.state.richlyEditable &&
nit: very weird wrapping of DIV

Also, there's probably nothing special about DIV, right?
Do you want this to be any role other than certain roles like
TEXT_FIELD, rather than only DIV?

[David Tseng, 2017-05-25 21:03:21]

Description was changed from

==========
Rich editable text implementation using spannables
==========

to

==========
Rich editable text implementation using spannables
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-05-25 21:39:20]

PTAL.

No longer wip. This is really close but has a bunch of small corner cases;  rich
lines are now working. We now need rich word and character feedback. Tests also
updated.

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

https://codereview.chromium.org/2903973002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:91: DIV
&& node.state.richlyEditable &&
On 2017/05/25 21:00:37, dmazzoni wrote:
> nit: very weird wrapping of DIV
Done.
> 
> Also, there's probably nothing special about DIV, right?
> Do you want this to be any role other than certain roles like
> TEXT_FIELD, rather than only DIV?
>  

Do you have any examples?

This one deals with the specific case where we put selection on the div of a
static text when a new line gets inserted.

[David Tseng, 2017-05-25 21:39:49]

Description was changed from

==========
Rich editable text implementation using spannables
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Rich editable text implementation using spannables
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2017-05-25 21:42:04]

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

[commit-bot: I haz the power, 2017-05-25 21:42:56]

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-05-25 22:41:22]

Patchset #6 (id:100001) has been deleted

[David Tseng, 2017-05-25 22:42:32]

Patchset #6 (id:120001) has been deleted

[David Tseng, 2017-05-26 22:14:06]

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

[David Tseng, 2017-05-26 22:14:07]

+ nektar as FYI. Latest patchset adds text marker verbalizations. This is
probably ready to remove from behind the flag.
PTAL.

[David Tseng, 2017-05-26 22:23:17]

Patchset #8 (id:180001) has been deleted

[David Tseng, 2017-05-30 22:57:06]

After holiday friendly ping. Added text style support as well.

[David Tseng, 2017-06-06 17:51:33]

Friendly week-ping.

[dmazzoni, 2017-06-07 17:30:13]

lgtm

OK to land this code behind a flag with some TODOs.
Please remove extra logging.

I tried patching it in and flipping the flag to try
it out, but found it's still pretty buggy. One simple
test I tried was arrowing through this one character
at a time, it breaks when you get to the second word.

<div contenteditable role=textbox>Hello <b>World</b></div>

For cases that did work, the style information is great.

https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
(right):

https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:80: //
Re-interpret this case as the beginning of the next node.
Add a TODO to take affinity as an argument to Cursor and to
check affinity. If affinity is downstream, which is the most
common, this is correct - but if it's upstream, we should leave
the cursor at the end of that node.

https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
(right):

https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:330: if
(markerType & 1)
Maybe add constants for these at the top of the file and add a
TODO to expose them in automation?

It'd probably be better if automation had a nice clean API for
this anyway.

[chromium-reviews, 2017-06-07 21:35:28]

dmazzoni@chromium.org writes:

> lgtm
>
> OK to land this code behind a flag with some TODOs.
> Please remove extra logging.
>
> I tried patching it in and flipping the flag to try
> it out, but found it's still pretty buggy. One simple
> test I tried was arrowing through this one character
> at a time, it breaks when you get to the second word.
>

This is a good test; I added the case and fixed the work around (see below).

> <div contenteditable role=textbox>Hello <b>World</b></div>
>
> For cases that did work, the style information is great.
>
>
>
>
https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js
> (right):
>
>
https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:80:
> // Re-interpret this case as the beginning of the next node.
> Add a TODO to take affinity as an argument to Cursor and to
> check affinity. If affinity is downstream, which is the most
> common, this is correct - but if it's upstream, we should leave
> the cursor at the end of that node.

Note that I also fixed the next on line logic (should have been next on
line for the current node, not the next node). Do you have a bug for
this selection issue? I'd rather not have to do this at all.

Added TODO for the upstream case.

>
>
https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js
> (right):
>
>
https://codereview.chromium.org/2903973002/diff/240001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:330:
> if (markerType & 1)
> Maybe add constants for these at the top of the file and add a
> TODO to expose them in automation?
>

Added TODO to expose in automation.
> It'd probably be better if automation had a nice clean API for
> this anyway.
>
> https://codereview.chromium.org/2903973002/


-- 

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-06-07 21:44:30]

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

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

Dry run: 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-06-07 21:56:33]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[David Tseng, 2017-06-08 16:32:54]

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

[commit-bot: I haz the power, 2017-06-08 16:33:08]

Dry run: 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-06-08 17:31:45]

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

[commit-bot: I haz the power, 2017-06-08 17:31:46]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-06-08 17:41:21]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-06-08 17:41:21]

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/2903973002/#ps260001
(title: "Address feedback.")

[commit-bot: I haz the power, 2017-06-08 17:41:54]

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-06-08 17:51:12]

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

[commit-bot: I haz the power, 2017-06-08 17:51:13]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[David Tseng, 2017-06-08 17:58:43]

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

[David Tseng, 2017-06-08 17:58:44]

+ michaelpg for externs/

[michaelpg, 2017-06-08 21:16:12]

https://codereview.chromium.org/2903973002/diff/260001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2903973002/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:669: long? textStyle;
This isn't sufficient documentation -- what are the numeric values? Can we make
them constants, or expand this into four booleans?

[David Tseng, 2017-06-08 22:20:10]

https://codereview.chromium.org/2903973002/diff/260001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2903973002/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:669: long? textStyle;
On 2017/06/08 21:16:12, michaelpg wrote:
> This isn't sufficient documentation -- what are the numeric values? Can we
make
> them constants, or expand this into four booleans?

Done.

[David Tseng, 2017-06-08 22:24:34]

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

[commit-bot: I haz the power, 2017-06-08 22:25:10]

Dry run: CQ is trying da patch.

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

[michaelpg, 2017-06-08 22:27:41]

lgtm

https://codereview.chromium.org/2903973002/diff/260001/chrome/common/extensio...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2903973002/diff/260001/chrome/common/extensio...
chrome/common/extensions/api/automation.idl:669: long? textStyle;
On 2017/06/08 22:20:09, David Tseng wrote:
> On 2017/06/08 21:16:12, michaelpg wrote:
> > This isn't sufficient documentation -- what are the numeric values? Can we
> make
> > them constants, or expand this into four booleans?
> 
> Done.

Thanks!

[commit-bot: I haz the power, 2017-06-08 23:17:13]

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

[commit-bot: I haz the power, 2017-06-08 23:17:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[David Tseng, 2017-06-09 03:19:24]

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

[commit-bot: I haz the power, 2017-06-09 03:19:43]

Dry run: 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-06-09 04:40:58]

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

[commit-bot: I haz the power, 2017-06-09 04:40:59]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-06-09 05:39:35]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-06-09 05:39:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2903973002/#ps300001
(title: "Make boolean expressions explicit.")

[commit-bot: I haz the power, 2017-06-09 05:39:52]

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-06-09 05:46:51]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1496986775973850,
"parent_rev": "e76d87acc9a55f551ddab222b5f8f656eee63042", "commit_rev":
"feef72d0d19c8336b0199b6b7c8ad10b0251f675"}

[commit-bot: I haz the power, 2017-06-09 05:47:08]

Description was changed from

==========
Rich editable text implementation using spannables
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Rich editable text implementation using spannables
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-06-09 05:47:09]

Committed patchset #13 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/feef72d0d19c8336b0199b6b7c8a...