Correctly finds line boundaries in objects with rich text on Windows.

Correctly finds line boundaries in objects with rich text on Windows.

The issue is that we were not taking into account embedded object characters.
Took the opportunity to update the code to use the new |nextOnLine| and |previousOnLine| Blink APIs.
BUG=605670
R=dmazzoni@chromium.org

Committed: https://crrev.com/78d5c7d32a7bd67f292397ddf7e79e079ff6f1d7
Cr-Commit-Position: refs/heads/master@{#390017}

[dmazzoni, 2016-04-22 15:34:49]

lgtm

https://codereview.chromium.org/1905263002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1905263002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:554: ;
nit: extra semicolon?

[blink-reviews, 2016-04-22 21:15:06]

Added a layout test for the behavior of next/previousOnLine Blink APIs 
on AXLayoutObject because I thought it might not be obvious and to make 
sure it remains consistent in the future.
Also fixed a bug with IsNext/PreviousSiblingOnSameLine. I now only store 
next_on_line IDs for text-only objects.
Please have another look.

>
https://codereview.chromium.org/1905263002/diff/1/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility.cc (right):
>
>
https://codereview.chromium.org/1905263002/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility.cc:554: ;
> nit: extra semicolon?

Removed.

-- 
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-04-22 21:43:38]

Good except for the roles

https://codereview.chromium.org/1905263002/diff/20001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1905263002/diff/20001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:406: if (dst->role ==
ui::AX_ROLE_STATIC_TEXT ||
It wasn't my intent for "next on line" and "previous on line" to only be
supported for those roles. I think it can be quite helpful for other roles too.

As an example, suppose you have an inline image:

<p>Before <img src="graphic.png" alt="graphic"> After</p>

You should be able to use nextOnLine and previousOnLine to trace from the word
Before to the inline image, to the word After, if they all happen to be laid out
on the same line.

[chromium-reviews, 2016-04-22 22:14:39]

Added a layout test for the behavior of next/previousOnLine Blink APIs 
on AXLayoutObject because I thought it might not be obvious and to make 
sure it remains consistent in the future.
Also fixed a bug with IsNext/PreviousSiblingOnSameLine. I now only store 
next_on_line IDs for text-only objects.
Please have another look.

>
https://codereview.chromium.org/1905263002/diff/1/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility.cc (right):
>
>
https://codereview.chromium.org/1905263002/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility.cc:554: ;
> nit: extra semicolon?

Removed.

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

[blink-reviews, 2016-04-24 00:58:57]

https://codereview.chromium.org/1905263002/diff/20001/content/renderer/access...
> File content/renderer/accessibility/blink_ax_tree_source.cc (right):
>
>
https://codereview.chromium.org/1905263002/diff/20001/content/renderer/access...
> content/renderer/accessibility/blink_ax_tree_source.cc:406: if
> (dst->role == ui::AX_ROLE_STATIC_TEXT ||
> It wasn't my intent for "next on line" and "previous on line" to only be
> supported for those roles. I think it can be quite helpful for other
> roles too.
>
> As an example, suppose you have an inline image:
>
> <p>Before <img src="graphic.png" alt="graphic"> After</p>
>
> You should be able to use nextOnLine and previousOnLine to trace from
> the word Before to the inline image, to the word After, if they all
> happen to be laid out on the same line.

That's what I initially thought. But then I found out that some of the 
samples I had for manual testing were not working and that the Blink 
code hangs my tests if I call it on objects that are more complex. So, I 
formed the impression that the Blink APIs were not designed that way.
Let's talk more offline.

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

[chromium-reviews, 2016-04-24 00:58:57]

https://codereview.chromium.org/1905263002/diff/20001/content/renderer/access...
> File content/renderer/accessibility/blink_ax_tree_source.cc (right):
>
>
https://codereview.chromium.org/1905263002/diff/20001/content/renderer/access...
> content/renderer/accessibility/blink_ax_tree_source.cc:406: if
> (dst->role == ui::AX_ROLE_STATIC_TEXT ||
> It wasn't my intent for "next on line" and "previous on line" to only be
> supported for those roles. I think it can be quite helpful for other
> roles too.
>
> As an example, suppose you have an inline image:
>
> <p>Before <img src="graphic.png" alt="graphic"> After</p>
>
> You should be able to use nextOnLine and previousOnLine to trace from
> the word Before to the inline image, to the word After, if they all
> happen to be laid out on the same line.

That's what I initially thought. But then I found out that some of the 
samples I had for manual testing were not working and that the Blink 
code hangs my tests if I call it on objects that are more complex. So, I 
formed the impression that the Blink APIs were not designed that way.
Let's talk more offline.

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

[blink-reviews, 2016-04-26 19:37:27]

Okay, I found out where the bug is.
|previousOnLine| crashes when called on a link object. Perhaps on an 
image too, I didn't check.
Could I check in what I have and then try and fix Blink in another patch?

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

[chromium-reviews, 2016-04-26 19:37:27]

Okay, I found out where the bug is.
|previousOnLine| crashes when called on a link object. Perhaps on an 
image too, I didn't check.
Could I check in what I have and then try and fix Blink in another patch?

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

[dmazzoni, 2016-04-26 20:13:15]

On 2016/04/26 19:37:27, chromium-reviews wrote:
> Okay, I found out where the bug is.
> |previousOnLine| crashes when called on a link object. Perhaps on an 
> image too, I didn't check.
> Could I check in what I have and then try and fix Blink in another patch?

ok, sounds good.

lgtm

[dmazzoni, 2016-04-26 20:13:16]

On 2016/04/26 19:37:27, chromium-reviews wrote:
> Okay, I found out where the bug is.
> |previousOnLine| crashes when called on a link object. Perhaps on an 
> image too, I didn't check.
> Could I check in what I have and then try and fix Blink in another patch?

ok, sounds good.

lgtm

[nektarios, 2016-04-26 20:54:48]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2016-04-26 20:55:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905263002/20001

[commit-bot: I haz the power, 2016-04-26 21:06:48]

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

[commit-bot: I haz the power, 2016-04-26 21:06:49]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[nektarios, 2016-04-26 21:23:18]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-04-26 21:23:19]

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/1905263002/#ps60001
(title: "Added logging output for tests.")

[commit-bot: I haz the power, 2016-04-26 21:23:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905263002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905263002/60001

[commit-bot: I haz the power, 2016-04-26 21:34:12]

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

[commit-bot: I haz the power, 2016-04-26 21:34:13]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[nektarios, 2016-04-26 23:29:28]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2016-04-26 23:29:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905263002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905263002/60001

[commit-bot: I haz the power, 2016-04-26 23:39:37]

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

[commit-bot: I haz the power, 2016-04-26 23:39:38]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[nektarios, 2016-04-26 23:46:24]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-04-26 23:46:25]

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/1905263002/#ps80001
(title: "Fixed ax_tree_combiner.")

[commit-bot: I haz the power, 2016-04-26 23:46:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905263002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905263002/80001

[commit-bot: I haz the power, 2016-04-27 01:32:19]

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

[commit-bot: I haz the power, 2016-04-27 01:32:20]

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

[nektarios, 2016-04-27 06:48:50]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-04-27 06:48:50]

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/1905263002/#ps100001
(title: "Fixed standard text fields in unit test.")

[commit-bot: I haz the power, 2016-04-27 06:49:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1905263002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1905263002/100001

[commit-bot: I haz the power, 2016-04-27 08:14:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-27 08:15:04]

Description was changed from

==========
Correctly finds line boundaries in objects with rich text on Windows.

The issue is that we were not taking into account embedded object characters.
Took the opportunity to update the code to use the new |nextOnLine| and
|previousOnLine| Blink APIs.
BUG=605670
R=dmazzoni@chromium.org
==========

to

==========
Correctly finds line boundaries in objects with rich text on Windows.

The issue is that we were not taking into account embedded object characters.
Took the opportunity to update the code to use the new |nextOnLine| and
|previousOnLine| Blink APIs.
BUG=605670
R=dmazzoni@chromium.org

Committed: https://crrev.com/78d5c7d32a7bd67f292397ddf7e79e079ff6f1d7
Cr-Commit-Position: refs/heads/master@{#390017}
==========

[commit-bot: I haz the power, 2016-04-27 08:15:06]

Patchset 6 (id:??) landed as
https://crrev.com/78d5c7d32a7bd67f292397ddf7e79e079ff6f1d7
Cr-Commit-Position: refs/heads/master@{#390017}