Use text affinity to return correct accessible line boundaries.

Use text affinity to return correct accessible line boundaries.

Our calculations of line boundaries for accessibility were wrong
in certain cases where the character offset is ambiguous. It's
possible for there to be a line break immediately between two
characters, such that one character is on the previous line and
one is on the next line. If the character index is between those
two characters, previously we had no way of knowing if the cursor
was on the first line or second line.

Fix this by passing through the text affinity of the selection
focus and anchor from Blink all the way into Chrome's accessibility
tree. If the affinity is upstream, it means the cursor is on the
previous line, and downstream means the next line.

I added a LayoutTest for Blink and a unit test for Chrome. I also
manually verified this fixes at least one broken case with VoiceOver,
see the bug for a very specific repro.

BUG=632450
Committed: https://crrev.com/25a949f677564d5e7ee1e7379528fd9c79a6ed6e
Cr-Commit-Position: refs/heads/master@{#408989}

[dmazzoni, 2016-07-28 19:52:36]

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

[commit-bot: I haz the power, 2016-07-28 19:53:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2016-07-28 19:54:06]

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

[commit-bot: I haz the power, 2016-07-28 20:28:43]

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

[commit-bot: I haz the power, 2016-07-28 20:28:44]

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

[nektarios, 2016-07-28 20:52:20]

1. Could we do the affinity calculations in Blink and expose the corresponding
anchor or focus objects for the previous line if the affinity is upstream? I
mean, why does the browser need to know the affinity? Couldn't we just return
the previous object and its max offset for the upstream case?
2. It appears to me that the value for the affinity is always downstream except
when selection endpoint is at the end of the line. Could we expose a boolean
value instead of a pair of enum values, like "anchor_at_end_of_line"?

[dmazzoni, 2016-07-28 21:09:00]

On Thu, Jul 28, 2016 at 1:52 PM <nektar@chromium.org> wrote:

> 1. Could we do the affinity calculations in Blink and expose the
> corresponding
> anchor or focus objects for the previous line if the affinity is upstream?
> I
> mean, why does the browser need to know the affinity? Couldn't we just
> return
> the previous object and its max offset for the upstream case?
>

That's a good idea and I think that could work someday, but I don't think
we could do that until we finish deleting all of the code that uses the
flattened representation. Currently we're implementing support for
<textarea> using only the flattened text, selection start index, selection
end index, and a vector of line break indices. There's no way to fix this
bug given that representation, we need the affinity too.

We could consider revisiting this once the flattened representation is gone.


> 2. It appears to me that the value for the affinity is always downstream
> except
> when selection endpoint is at the end of the line. Could we expose a
> boolean
> value instead of a pair of enum values, like "anchor_at_end_of_line"?
>

I don't think that's any better. Affinity is the right technical term for
this within the Chrome codebase, we should use that term for consistency
and comment it for people who are unfamiliar with it, rather than trying to
explain it with a verbose name like anchor_at_end_of_line. Another reason
is that the Chrome style strongly encourages using enums instead of
booleans for improved readability. All of the unit tests for line breaks
are a lot more readable when they pass an argument called Upstream or
Downstream rather than true or false.

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

[dmazzoni, 2016-07-28 21:09:01]

On Thu, Jul 28, 2016 at 1:52 PM <nektar@chromium.org> wrote:

> 1. Could we do the affinity calculations in Blink and expose the
> corresponding
> anchor or focus objects for the previous line if the affinity is upstream?
> I
> mean, why does the browser need to know the affinity? Couldn't we just
> return
> the previous object and its max offset for the upstream case?
>

That's a good idea and I think that could work someday, but I don't think
we could do that until we finish deleting all of the code that uses the
flattened representation. Currently we're implementing support for
<textarea> using only the flattened text, selection start index, selection
end index, and a vector of line break indices. There's no way to fix this
bug given that representation, we need the affinity too.

We could consider revisiting this once the flattened representation is gone.


> 2. It appears to me that the value for the affinity is always downstream
> except
> when selection endpoint is at the end of the line. Could we expose a
> boolean
> value instead of a pair of enum values, like "anchor_at_end_of_line"?
>

I don't think that's any better. Affinity is the right technical term for
this within the Chrome codebase, we should use that term for consistency
and comment it for people who are unfamiliar with it, rather than trying to
explain it with a verbose name like anchor_at_end_of_line. Another reason
is that the Chrome style strongly encourages using enums instead of
booleans for improved readability. All of the unit tests for line breaks
are a lot more readable when they pass an argument called Upstream or
Downstream rather than true or false.

-- 
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.

[nektarios, 2016-07-28 22:03:09]

lgtm

[dmazzoni, 2016-07-28 22:10:34]

dmazzoni@chromium.org changed reviewers:
+ mkwst@chromium.org, tsepez@chromium.org

[dmazzoni, 2016-07-28 22:10:34]

+mkwst for public/web and Source/web
+tsepez for ipc *messages* and *param_traits* files

[Tom Sepez, 2016-07-28 22:15:22]

On 2016/07/28 22:10:34, dmazzoni wrote:
> +mkwst for public/web and Source/web
> +tsepez for ipc *messages* and *param_traits* files

LGTM for adding two new range-checked enums to data structures.

[dmazzoni, 2016-07-28 22:16:42]

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

[commit-bot: I haz the power, 2016-07-28 22:17:14]

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, 2016-07-28 23:32:40]

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

[commit-bot: I haz the power, 2016-07-28 23:32:41]

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

[dmazzoni, 2016-07-28 23:50:43]

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

[commit-bot: I haz the power, 2016-07-28 23:51:25]

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, 2016-07-29 01:34:23]

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

[commit-bot: I haz the power, 2016-07-29 01:34:24]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-08-01 11:45:11]

LGTM.

[dmazzoni, 2016-08-01 15:47:43]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-08-01 15:47:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2191833003/#ps40001
(title: "Fix bug in browser_accessibility_win.cc")

[commit-bot: I haz the power, 2016-08-01 15:47:57]

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, 2016-08-01 17:21:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-01 17:22:56]

Description was changed from

==========
Use text affinity to return correct accessible line boundaries.

Our calculations of line boundaries for accessibility were wrong
in certain cases where the character offset is ambiguous. It's
possible for there to be a line break immediately between two
characters, such that one character is on the previous line and
one is on the next line. If the character index is between those
two characters, previously we had no way of knowing if the cursor
was on the first line or second line.

Fix this by passing through the text affinity of the selection
focus and anchor from Blink all the way into Chrome's accessibility
tree. If the affinity is upstream, it means the cursor is on the
previous line, and downstream means the next line.

I added a LayoutTest for Blink and a unit test for Chrome. I also
manually verified this fixes at least one broken case with VoiceOver,
see the bug for a very specific repro.

BUG=632450
==========

to

==========
Use text affinity to return correct accessible line boundaries.

Our calculations of line boundaries for accessibility were wrong
in certain cases where the character offset is ambiguous. It's
possible for there to be a line break immediately between two
characters, such that one character is on the previous line and
one is on the next line. If the character index is between those
two characters, previously we had no way of knowing if the cursor
was on the first line or second line.

Fix this by passing through the text affinity of the selection
focus and anchor from Blink all the way into Chrome's accessibility
tree. If the affinity is upstream, it means the cursor is on the
previous line, and downstream means the next line.

I added a LayoutTest for Blink and a unit test for Chrome. I also
manually verified this fixes at least one broken case with VoiceOver,
see the bug for a very specific repro.

BUG=632450

Committed: https://crrev.com/25a949f677564d5e7ee1e7379528fd9c79a6ed6e
Cr-Commit-Position: refs/heads/master@{#408989}
==========

[commit-bot: I haz the power, 2016-08-01 17:22:58]

Patchset 3 (id:??) landed as
https://crrev.com/25a949f677564d5e7ee1e7379528fd9c79a6ed6e
Cr-Commit-Position: refs/heads/master@{#408989}