Get rid of AX_LINE_BREAKS attribute to improve performance.

Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other platforms.
TESTED=Manually with Voiceover ChromeVox Jaws and NVDA, existing Windows browser tests, new Automation API tests, existing ChromeVox tests
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/91926613c70561f2ddeb86896496e7306518c747
Cr-Commit-Position: refs/heads/master@{#420807}

[dmazzoni, 2016-09-02 15:55:00]

Overall looks great, thanks for doing this so fast!

Is this covered by any existing tests?

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

https://codereview.chromium.org/2301833005/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:1152: case
ui::AX_ROLE_INLINE_TEXT_BOX:
This algorithm is fine, it will certainly work correctly for plaintext

I think there might be a few cases where it doesn't do what we intend if you mix
content other than text into a paragraph. For example imagine you have a tiny
inline <img> element that happens to be the last thing on a line. I think you
still want to call IsNextSiblingOnSameLine() on that image even though it's not
an inline text box

So my initial thought is to just call IsNextSiblingOnSameLine on any leaf node

But I'm okay to land this and try to fix it better with AXPosition

https://codereview.chromium.org/2301833005/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:1163:
std::transform(child_breaks.begin(), child_breaks.end(), child_breaks.begin(),
std::bind2nd()std::plus<int>, offset));
Needs indentation

Could you write this using a lambda instead of bind2nd and plus? I think that'd
be more readable.

This involves a fair amount of copying, an alternative that comes to mind is to
use a helper that takes an offset argument, then you can do it all in one pass.

// Recursively finds line breaks in this subtree and appends them to the passed
vector, adding |offset| to each one.
void BrowserAccessibility::AppendLineBreaksWithOffset(int offset,
std::vector<int>* line_breaks) const {
}

std::vector<int> BrowserAccessibility::ComputeLineBreaks() const {
  std::vector<int> result;
  AppendLineBreaksWithOffset(0, &result);
  return result;
}

[dmazzoni, 2016-09-02 15:55:24]

Description was changed from

==========
Get rid of AX_LINE_BREAKS attribute to improve performance.

R=dmazzoni@chromium.org
==========

to

==========
Get rid of AX_LINE_BREAKS attribute to improve performance.

R=dmazzoni@chromium.org
BUG=642337
==========

[chromium-reviews, 2016-09-02 16:01:13]

Drive by.

How was performance measured? Please run through try bots. ChromeVox uses
line breaks quite heavily.
On Fri, Sep 2, 2016 at 8:41 AM nektar@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Reviewers: dmazzoni
> CL: https://codereview.chromium.org/2301833005/
>
> Description:
> Get rid of AX_LINE_BREAKS attribute to improve performance.
>
> R=dmazzoni@chromium.org
>
> Affected files (+38, -28 lines):
> M content/browser/accessibility/browser_accessibility.h
> M content/browser/accessibility/browser_accessibility.cc
> M content/browser/accessibility/browser_accessibility_cocoa.mm
> M content/browser/accessibility/browser_accessibility_win.cc
> M content/renderer/accessibility/blink_ax_tree_source.cc
> M ui/accessibility/ax_enums.idl
> M ui/accessibility/ax_node_data.cc
> M ui/accessibility/ax_tree_combiner.cc
>
>
> Index: content/browser/accessibility/browser_accessibility.cc
> diff --git a/content/browser/accessibility/browser_accessibility.cc
> b/content/browser/accessibility/browser_accessibility.cc
> index
>
0797a36a0fc0c8af0e8faa15f4e5189615321aa3..575404aba2925ab160905bb769c613b097caa391
> 100644
> --- a/content/browser/accessibility/browser_accessibility.cc
> +++ b/content/browser/accessibility/browser_accessibility.cc
> @@ -7,6 +7,7 @@
> #include <stddef.h>
>
> #include <algorithm>
> +#include <functional>
>
> #include "base/logging.h"
> #include "base/strings/string_number_conversions.h"
> @@ -533,9 +534,7 @@ int BrowserAccessibility::GetLineStartBoundary(
> DCHECK_LE(start, static_cast<int>(GetText().length()));
>
> if (IsSimpleTextControl()) {
> - const std::vector<int32_t>& line_breaks =
> - GetIntListAttribute(ui::AX_ATTR_LINE_BREAKS);
> - return ui::FindAccessibleTextBoundary(GetText(), line_breaks,
> + return ui::FindAccessibleTextBoundary(GetText(), ComputeLineBreaks(),
> ui::LINE_BOUNDARY, start, direction,
> affinity);
> }
> @@ -1142,6 +1141,33 @@ std::string
> BrowserAccessibility::ComputeAccessibleNameFromDescendants() {
> return name;
> }
>
> +std::vector<int> BrowserAccessibility::ComputeLineBreaks() const {
> + std::vector<int> line_breaks;
> + int offset = 0;
> + for (size_t i = 0; i < InternalChildCount(); ++i) {
> + const BrowserAccessibility* child = InternalGetChild(i);
> + DCHECK(child);
> + switch
> + child->GetRole() {
> + case ui::AX_ROLE_INLINE_TEXT_BOX:
> + offset += child->GetText().length();
> + if (!child->IsNextSiblingOnSameLine())
> + line_breaks.push_back(offset);
> + break;
> + case ui::AX_ROLE_LINE_BREAK:
> + line_breaks.push_back(offset);
> + offset += child->GetText().length();
> + break;
> + default:
> + std::vector<int> child_breaks = child->ComputeLineBreaks();
> + std::transform(child_breaks.begin(), child_breaks.end(),
> child_breaks.begin(), std::bind2nd()std::plus<int>, offset));
> + line_breaks.insert(line_breaks.end(), child_breaks.begin(),
> + child_breaks.end());
> + }
> + }
> + return line_breaks;
> +}
> +
> base::string16 BrowserAccessibility::GetInnerText() const {
> if (IsTextOnlyObject())
> return GetString16Attribute(ui::AX_ATTR_NAME);
> Index: content/browser/accessibility/browser_accessibility.h
> diff --git a/content/browser/accessibility/browser_accessibility.h
> b/content/browser/accessibility/browser_accessibility.h
> index
>
8192a195e90a3811c168c35cd7ff736f748ecdfe..6264a616f295f8505b4917c21453c4e93a048f5a
> 100644
> --- a/content/browser/accessibility/browser_accessibility.h
> +++ b/content/browser/accessibility/browser_accessibility.h
> @@ -344,6 +344,9 @@ class CONTENT_EXPORT BrowserAccessibility {
> // to compute a name from its descendants.
> std::string ComputeAccessibleNameFromDescendants();
>
> + // Computes the text offsets where line breaks occur.
> + std::vector<int> ComputeLineBreaks() const;
> +
> protected:
> BrowserAccessibility();
>
> Index: content/browser/accessibility/browser_accessibility_cocoa.mm
> diff --git a/content/browser/accessibility/browser_accessibility_cocoa.mm
> b/content/browser/accessibility/browser_accessibility_cocoa.mm
> index
>
b6d73a815fff640a1df856ecb587d1396f53aafc..1e79ab4a9c430b6642ae9a55c4b8220c1586b18c
> 100644
> --- a/content/browser/accessibility/browser_accessibility_cocoa.mm
> +++ b/content/browser/accessibility/browser_accessibility_cocoa.mm
> @@ -1022,8 +1022,8 @@ NSString* const NSAccessibilityRequiredAttribute =
> @"AXRequired";
> if (selStart > selEnd)
> std::swap(selStart, selEnd);
>
> - const std::vector<int32_t>& line_breaks =
> - browserAccessibility_->GetIntListAttribute(ui::AX_ATTR_LINE_BREAKS);
> + const std::vector<int> line_breaks =
> + browserAccessibility_->ComputeLineBreaks();
> for (int i = 0; i < static_cast<int>(line_breaks.size()); ++i) {
> if (line_breaks[i] > selStart)
> return [NSNumber numberWithInt:i];
> @@ -1972,8 +1972,8 @@ NSString* const NSAccessibilityRequiredAttribute =
> @"AXRequired";
> if (![self instanceActive])
> return nil;
>
> - const std::vector<int32_t>& line_breaks =
> - browserAccessibility_->GetIntListAttribute(ui::AX_ATTR_LINE_BREAKS);
> + const std::vector<int> line_breaks =
> + browserAccessibility_->ComputeLineBreaks();
> base::string16 value = browserAccessibility_->GetValue();
> int len = static_cast<int>(value.size());
>
> Index: content/browser/accessibility/browser_accessibility_win.cc
> diff --git a/content/browser/accessibility/browser_accessibility_win.cc
> b/content/browser/accessibility/browser_accessibility_win.cc
> index
>
2e3d24f90c880afd4c89815b1c4f449e27041fc3..00ecf6c0077d0aa7244e7b56e3a29424ebdd23e9
> 100644
> --- a/content/browser/accessibility/browser_accessibility_win.cc
> +++ b/content/browser/accessibility/browser_accessibility_win.cc
> @@ -4384,10 +4384,8 @@ LONG BrowserAccessibilityWin::FindBoundary(
> }
>
> ui::TextBoundaryType boundary =
> IA2TextBoundaryToTextBoundary(ia2_boundary);
> - const std::vector<int32_t>& line_breaks =
> - GetIntListAttribute(ui::AX_ATTR_LINE_BREAKS);
> - return ui::FindAccessibleTextBoundary(
> - text, line_breaks, boundary, start_offset, direction, affinity);
> + return ui::FindAccessibleTextBoundary(text, ComputeLineBreaks(),
> boundary,
> + start_offset, direction, affinity);
> }
>
> LONG BrowserAccessibilityWin::FindStartOfStyle(
> Index: content/renderer/accessibility/blink_ax_tree_source.cc
> diff --git a/content/renderer/accessibility/blink_ax_tree_source.cc
> b/content/renderer/accessibility/blink_ax_tree_source.cc
> index
>
bc55845aa20f4a80ab89cfffa2aed950ab4bb4ba..deab7bfe0f1b60b9713328e4ae1d2485950d5699
> 100644
> --- a/content/renderer/accessibility/blink_ax_tree_source.cc
> +++ b/content/renderer/accessibility/blink_ax_tree_source.cc
> @@ -508,16 +508,6 @@ void
> BlinkAXTreeSource::SerializeNode(blink::WebAXObject src,
> if (src.isEditable()) {
> dst->AddIntAttribute(ui::AX_ATTR_TEXT_SEL_START, src.selectionStart());
> dst->AddIntAttribute(ui::AX_ATTR_TEXT_SEL_END, src.selectionEnd());
> -
> - WebVector<int> src_line_breaks;
> - src.lineBreaks(src_line_breaks);
> - if (src_line_breaks.size()) {
> - std::vector<int32_t> line_breaks;
> - line_breaks.reserve(src_line_breaks.size());
> - for (size_t i = 0; i < src_line_breaks.size(); ++i)
> - line_breaks.push_back(src_line_breaks[i]);
> - dst->AddIntListAttribute(ui::AX_ATTR_LINE_BREAKS, line_breaks);
> - }
> }
>
> // ARIA role.
> Index: ui/accessibility/ax_enums.idl
> diff --git a/ui/accessibility/ax_enums.idl b/ui/accessibility/ax_enums.idl
> index
>
010460c56d517c21dd49dfc738c40b972465af37..2e754858ce4ebada5de70310012722d447649f07
> 100644
> --- a/ui/accessibility/ax_enums.idl
> +++ b/ui/accessibility/ax_enums.idl
> @@ -389,9 +389,6 @@
> flowto_ids,
> labelledby_ids,
>
> - // For static text. Character indices where line breaks occur.
> - line_breaks,
> -
> // For static text. These int lists must be the same size; they represent
> // the start and end character offset of each marker. Examples of markers
> // include spelling and grammar errors, and find-in-page matches.
> Index: ui/accessibility/ax_node_data.cc
> diff --git a/ui/accessibility/ax_node_data.cc
> b/ui/accessibility/ax_node_data.cc
> index
>
0f04517978c944131f8cce87eaa8c12cb85f763c..72180ab4f85194fb06cab403333690c7442c9e82
> 100644
> --- a/ui/accessibility/ax_node_data.cc
> +++ b/ui/accessibility/ax_node_data.cc
> @@ -692,9 +692,6 @@ std::string AXNodeData::ToString() const {
> case AX_ATTR_LABELLEDBY_IDS:
> result += " labelledby_ids=" + IntVectorToString(values);
> break;
> - case AX_ATTR_LINE_BREAKS:
> - result += " line_breaks=" + IntVectorToString(values);
> - break;
> case AX_ATTR_MARKER_TYPES: {
> std::string types_str;
> for (size_t i = 0; i < values.size(); ++i) {
> Index: ui/accessibility/ax_tree_combiner.cc
> diff --git a/ui/accessibility/ax_tree_combiner.cc
> b/ui/accessibility/ax_tree_combiner.cc
> index
>
0731802512fb2fc79f25812306f9e01fa96c261c..98994bca2356a3a44289e147dda480d1d798ad12
> 100644
> --- a/ui/accessibility/ax_tree_combiner.cc
> +++ b/ui/accessibility/ax_tree_combiner.cc
> @@ -82,7 +82,6 @@ bool IsNodeIdIntListAttribute(AXIntListAttribute attr) {
> // add a new attribute without explicitly considering whether it's
> // a node id attribute or not.
> case AX_INT_LIST_ATTRIBUTE_NONE:
> - case AX_ATTR_LINE_BREAKS:
> case AX_ATTR_MARKER_TYPES:
> case AX_ATTR_MARKER_STARTS:
> case AX_ATTR_MARKER_ENDS:
>
>
>

-- 
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, 2016-09-02 16:08:12]

On 2016/09/02 15:55:00, dmazzoni wrote:
> Overall looks great, thanks for doing this so fast!
> 
> Is this covered by any existing tests?
> 

Can we get a BUG= or a summary of the offline discussion on this?

Was this tested with content editables? Are we actually getting a measured
performance improvement using this cl?

>
https://codereview.chromium.org/2301833005/diff/1/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility.cc (right):
> 
>
https://codereview.chromium.org/2301833005/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility.cc:1152: case
> ui::AX_ROLE_INLINE_TEXT_BOX:
> This algorithm is fine, it will certainly work correctly for plaintext
> 
> I think there might be a few cases where it doesn't do what we intend if you
mix
> content other than text into a paragraph. For example imagine you have a tiny
> inline <img> element that happens to be the last thing on a line. I think you
> still want to call IsNextSiblingOnSameLine() on that image even though it's
not
> an inline text box
> 
> So my initial thought is to just call IsNextSiblingOnSameLine on any leaf node
> 
> But I'm okay to land this and try to fix it better with AXPosition
> 
>
https://codereview.chromium.org/2301833005/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility.cc:1163:
> std::transform(child_breaks.begin(), child_breaks.end(), child_breaks.begin(),
> std::bind2nd()std::plus<int>, offset));
> Needs indentation
> 
> Could you write this using a lambda instead of bind2nd and plus? I think
that'd
> be more readable.
> 
> This involves a fair amount of copying, an alternative that comes to mind is
to
> use a helper that takes an offset argument, then you can do it all in one
pass.
> 
> // Recursively finds line breaks in this subtree and appends them to the
passed
> vector, adding |offset| to each one.
> void BrowserAccessibility::AppendLineBreaksWithOffset(int offset,
> std::vector<int>* line_breaks) const {
> }
> 
> std::vector<int> BrowserAccessibility::ComputeLineBreaks() const {
>   std::vector<int> result;
>   AppendLineBreaksWithOffset(0, &result);
>   return result;
> }

[chromium-reviews, 2016-09-02 17:52:06]

On 9/2/2016 12:01 PM, David Tseng wrote:
> Drive by.
>
> How was performance measured? Please run through try bots. ChromeVox 
> uses line breaks quite heavily.

We got a report from a user and the trace showed computing line breaks 
taking a long time. Dominic knows more about this. I'll assign the bug 
with the discussion; I haven't forgotten.
ChromeVox was the next thing for me to check. I am not going to submit 
anything that creates trouble for ChromeVox.

-- 
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-09-07 16:22:48]

I don't think ChromeVox uses the AX_LINE_BREAKS attribute, though, so
I think we're fine.

[nektarios, 2016-09-13 01:00:16]

I completely re-wrote this.
Should work for all editable fields, including content editable.
But only tested with input and textareas for now.
Caches the result for performance reasons, since bug was due to performance
issues.
Can work on any AXNode.
Should work from automation and other platforms.
Still working on more tests.

[nektarios, 2016-09-13 01:02:43]

Description was changed from

==========
Get rid of AX_LINE_BREAKS attribute to improve performance.

R=dmazzoni@chromium.org
BUG=642337
==========

to

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
==========

[nektarios, 2016-09-13 01:02:43]

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

[dmazzoni, 2016-09-13 17:29:38]

Overall looks great, my only concern is the new test.
If it's not working right on page load, let me know
and we can try to figure out why.

https://codereview.chromium.org/2301833005/diff/20001/chrome/browser/accessib...
File chrome/browser/accessibility/accessibility_extension_api.cc (left):

https://codereview.chromium.org/2301833005/diff/20001/chrome/browser/accessib...
chrome/browser/accessibility/accessibility_extension_api.cc:13: #include
"chrome/browser/accessibility/accessibility_extension_api.h"
Don't remove this include

https://codereview.chromium.org/2301833005/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/automation/tests/tabs/line_start_offsets.js
(right):

https://codereview.chromium.org/2301833005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/line_start_offsets.js:7:
var input = rootNode.firstChild.firstChild;
This is fragile, anytime we change what nodes are ignored the test will have to
be updated.

Do something like this instead:

  var input = rootNode.find({ role: 'textField' });

You can also find by name or almost any other attribute so you can find both

https://codereview.chromium.org/2301833005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/automation/tests/tabs/line_start_offsets.js:9:
EventType.childrenChanged, function(e) {
Why are you running this on childrenChanged? Is the test failing if you run it
on page load? I'm surprised if so.

It seems to me like this test will probably be flaky because there's no
guarantee you'll get a childrenChanged every time this page loads

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:420: line_starts,
Maybe it'd be more clear to call it cached_line_starts?

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.cc (left):

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.cc:695: case AX_ATTR_LINE_BREAKS:
I'd prefer if AXNodeData dumped the new cached attribute too, just so when we do
dump it we really do see the whole structure. It could call it cachedLineBreaks=

[David Tseng, 2016-09-13 17:41:27]

Thanks for the re-write.

You still need to get rid of lineBreaks under 
chrome/browser/resources/chromeos/chromevox/*.

Also, wrt the bug, did someone patch this cl in and verify the performance
improvement?

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_nod...
ui/accessibility/ax_node.cc:70: int* end_offset) const {
Is this meant to work on any node (e.g.
<div aria-label="hello"></div>
)?

What does line start offsets mean?

https://codereview.chromium.org/2301833005/diff/20001/ui/accessibility/ax_nod...
ui/accessibility/ax_node.cc:81: *end_offset += static_cast<int>(text.length());
Just make end_offset a size_t

[dmazzoni, 2016-09-13 17:50:44]

On Tue, Sep 13, 2016 at 10:41 AM <dtseng@chromium.org> wrote:

> Thanks for the re-write.
>
> You still need to get rid of lineBreaks under
> chrome/browser/resources/chromeos/chromevox/*.
>
> Also, wrt the bug, did someone patch this cl in and verify the performance
> improvement?
>

In a sense I did, I verified that commenting out the computation of
lineBreaks on the renderer side fixed the bug.

There's still room for more improvement if you're editing a large
contentEditable, but this patch definitely solves the problem of the page
slowing down when a contentEditable is changing when you're not even
editing it.

- Dominic

-- 
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, 2016-09-13 18:09:59]

On Tue, Sep 13, 2016 at 10:50 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Tue, Sep 13, 2016 at 10:41 AM <dtseng@chromium.org> wrote:
>
>> Thanks for the re-write.
>>
>> You still need to get rid of lineBreaks under
>> chrome/browser/resources/chromeos/chromevox/*.
>>
>> Also, wrt the bug, did someone patch this cl in and verify the performance
>> improvement?
>>
>
> In a sense I did, I verified that commenting out the computation of
> lineBreaks on the renderer side fixed the bug.
>
> browser side results in the expected performance gains?



> There's still room for more improvement if you're editing a large
> contentEditable, but this patch definitely solves the problem of the page
> slowing down when a contentEditable is changing when you're not even
> editing it.
>
> Yup; I can see why it would be better with this change making the line
offsets computed on demand but would like to see the actual time
improvement with this cl vs without.




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

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

[chromium-reviews, 2016-09-13 18:13:08]

Okay, I'll try and do some performance measurements and also modify 
ChromeVox to use this change.


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

[chromium-reviews, 2016-09-16 20:08:13]

>
https://codereview.chromium.org/2301833005/diff/20001/chrome/browser/accessib...
> File chrome/browser/accessibility/accessibility_extension_api.cc (left):
>
>
https://codereview.chromium.org/2301833005/diff/20001/chrome/browser/accessib...
> chrome/browser/accessibility/accessibility_extension_api.cc:13: #include
> "chrome/browser/accessibility/accessibility_extension_api.h"
> Don't remove this include

I removed it because it was included twice. Do you still want me to add 
it back?

-- 
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-09-16 22:08:16]

On Fri, Sep 16, 2016 at 1:08 PM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> chrome/browser/accessibility/accessibility_extension_api.cc:13: #include
> "chrome/browser/accessibility/accessibility_extension_api.h"
> Don't remove this include
>
> I removed it because it was included twice. Do you still want me to add it
> back?
>

Nevermind, didn't realize it was above too. All good.

-- 
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-09-17 00:28:31]

Description was changed from

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
==========

to

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[nektarios, 2016-09-17 00:28:37]

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

[commit-bot: I haz the power, 2016-09-17 00:28:56]

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-09-17 00:41:10]

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

[commit-bot: I haz the power, 2016-09-17 00:41:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nektarios, 2016-09-19 15:26:29]

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

[commit-bot: I haz the power, 2016-09-19 15:26:42]

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

[nektarios, 2016-09-19 15:27:04]

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

[commit-bot: I haz the power, 2016-09-19 15:27:14]

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

[nektarios, 2016-09-19 15:28:53]

Description was changed from

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
TESTED=Manually with Voiceover ChromeVox Jaws and NVDA, existing Windows browser
tests, new Automation API tests, existing ChromeVox tests
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[nektarios, 2016-09-19 15:46:58]

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

[nektarios, 2016-09-19 15:47:04]

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

[commit-bot: I haz the power, 2016-09-19 15:47: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, 2016-09-19 16:25:16]

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

[commit-bot: I haz the power, 2016-09-19 16:25:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nektarios, 2016-09-19 16:38:50]

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

[commit-bot: I haz the power, 2016-09-19 16:39:01]

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-09-19 17:05:36]

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

[commit-bot: I haz the power, 2016-09-19 17:05:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nektarios, 2016-09-19 18:52:33]

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

[commit-bot: I haz the power, 2016-09-19 18:53:31]

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

[nektarios, 2016-09-19 19:03:32]

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

[commit-bot: I haz the power, 2016-09-19 19:04:12]

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-09-19 20:07:08]

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

[commit-bot: I haz the power, 2016-09-19 20:07:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nektarios, 2016-09-19 21:02:26]

I did some rough measurements with DLOG statements. I logged the start and end
times of the old and the new functions for calculating line offsets, from the
time I started Chrome until I shut it down.
I created a textarea and pasted in more than 5 pages of text.
I moved to the top and started moving down line by line until I read the first
section of the document, around 30 lines.
The old code ran 74 times. The new code, due to caching, ran 36 times, which is
around half as many. I assume that most of the runs were not in the text area.
The new code took less than one millisecond on all except one of the runs, when
it took one millisecond. In contrast, the old code took one millisecond in 7 out
of the 74 runs. DLOG doesn't give me sub millisecond accuracy.
So, it appears that the new code is somewhat faster, but most importantly runs
less times.

Data for old code:
121903to121903
121905to121905
121905to121905
121935to121935
121936to121936
121936to121936
121936to121936
121937to121937
121937to121937
121938to121938
121938to121938
121939to121939
121939to121940
121940to121940
121941to121941
121941to121941
121942to121942
121942to121942
121943to121943
121943to121943
121944to121944
121944to121944
121945to121945
121945to121945
121945to121945
121945to121946
121946to121946
121946to121946
121946to121947
121947to121947
121947to121947
121947to121947
121948to121948
121948to121948
121948to121948
121948to121948
121949to121949
121949to121949
121949to121950
121950to121950
121950to121950
121950to121950
121951to121951
121951to121951
121951to121951
121952to121952
121952to121952
121952to121952
121953to121953
121953to121953
121953to121953
121953to121953
121954to121954
121954to121954
121954to121954
121954to121954
121955to121955
121955to121955
121956to121956
121956to121956
121957to121957
121957to121957
121957to121958
121958to121958
122000to122000
122000to122000
122000to122000
122000to122001
122001to122001
122001to122001
122001to122001
122001to122002
122002to122002
122002to122002


Data for new code:
163553to163553
163557to163557
163557to163557
163558to163558
163558to163558
163559to163559
163600to163600
163600to163600
163600to163600
163601to163601
163601to163601
163601to163602
163602to163602
163602to163602
163603to163603
163603to163603
163604to163604
163604to163604
163605to163605
163605to163605
163606to163606
163606to163606
163606to163606
163607to163607
163607to163607
163608to163608
163609to163609
163609to163609
163609to163609
163609to163609
163610to163610
163610to163610
163611to163611
163611to163611
163612to163612
163612to163612

[nektarios, 2016-09-19 21:12:13]

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

[commit-bot: I haz the power, 2016-09-19 21:13:12]

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

[nektarios, 2016-09-19 21:14:47]

Ready for another look.

[commit-bot: I haz the power, 2016-09-19 21:56:38]

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

[commit-bot: I haz the power, 2016-09-19 21:56:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nektarios, 2016-09-19 22:18:03]

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

[commit-bot: I haz the power, 2016-09-19 22:18:39]

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-09-19 23:16:21]

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

[commit-bot: I haz the power, 2016-09-19 23:16:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-09-21 18:47:38]

lgtm

https://codereview.chromium.org/2301833005/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/automation/sites/line_start_offsets.html
(right):

https://codereview.chromium.org/2301833005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/automation/sites/line_start_offsets.html:17:
Line thre.</textarea>
Nit: thre -> three

https://codereview.chromium.org/2301833005/diff/180001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2301833005/diff/180001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:418: // For all objects with text content. A list
of the start offset of each
"all objects with text content" could be confusing, I'd say "all editable text
controls and contenteditable roots" or something like that.

[nektarios, 2016-09-21 21:18:42]

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

[commit-bot: I haz the power, 2016-09-21 21:19:07]

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-09-21 22:21:49]

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

[commit-bot: I haz the power, 2016-09-21 22:21:50]

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

[chromium-reviews, 2016-09-22 15:54:03]

https://codereview.chromium.org/2301833005/diff/180001/ui/accessibility/ax_en...
> File ui/accessibility/ax_enums.idl (right):
>
>
https://codereview.chromium.org/2301833005/diff/180001/ui/accessibility/ax_en...
> ui/accessibility/ax_enums.idl:418: // For all objects with text content.
> A list of the start offset of each
> "all objects with text content" could be confusing, I'd say "all
> editable text controls and contenteditable roots" or something like
> that.

In order to enable the Jaws Cursor (i.e. the ability to move the mouse) 
on a webpage we need to return line boundaries for all objects that 
contain text. For example, if there is a Wikipedia article with several 
<p> elements and a user enables the Jaws Cursor and tries to read one of 
those paragraphs line by line, Jaws will ask us to provide the line 
boundaries in that paragraph.

-- 
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-09-22 16:17:47]

On Thu, Sep 22, 2016 at 8:53 AM Nektarios Paisios <nektar@google.com> wrote:

> In order to enable the Jaws Cursor (i.e. the ability to move the mouse) on
> a webpage we need to return line boundaries for all objects that contain
> text. For example, if there is a Wikipedia article with several <p>
> elements and a user enables the Jaws Cursor and tries to read one of those
> paragraphs line by line, Jaws will ask us to provide the line boundaries in
> that paragraph.
>

But we don't use the line breaks array for that, do we? We didn't before -
before this change we only computed the line breaks for editable text nodes.

Computing line breaks for every node in the tree would be at least O(n^2)
complexity.

-- 
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-09-22 21:55:37]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-09-22 21:55:37]

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/2301833005/#ps240001
(title: "Fixed test.")

[nektarios, 2016-09-22 21:56:12]

The CQ bit was unchecked by nektar@chromium.org

[commit-bot: I haz the power, 2016-09-22 21:56:19]

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

[nektarios, 2016-09-22 22:07:17]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-09-22 22:07:17]

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/2301833005/#ps260001
(title: "Re-worded comment.")

[commit-bot: I haz the power, 2016-09-22 22:08:02]

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-09-22 22:18:22]

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

[commit-bot: I haz the power, 2016-09-22 22:18:23]

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

[nektarios, 2016-09-23 00:25:23]

David also needs to approve.

[David Tseng, 2016-09-23 01:43:58]

lgtm

Thanks for doing the measurements. I guess I'm still not seeing the referenced
O(n^2) growth and the original profiling stated on the bug isn't your setup.
It's lots of binding code changes for what may be difficult to explain
performance gains.

Taken as a pure refactoring change, the new line breaks calculation isn't the
same as the one in Blink. Not sure if we care, but a screen reader might given a
rich document where it's important to move by the same underlying Blink
position/selection and not based on the AX accessible name lengths.

Finally, you probably also want to remove lineBreaks from AXLayoutObject; can do
that now or in a subsequent change.

I don't think the change is great for the above reasons, but also don't see any
good reason to block the change for fixing the Mac issue which I know was the
impetus to get this out so quickly.

https://codereview.chromium.org/2301833005/diff/260001/ui/accessibility/ax_no...
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2301833005/diff/260001/ui/accessibility/ax_no...
ui/accessibility/ax_node.cc:80: base::string16 text =
child->data().GetString16Attribute(ui::AX_ATTR_NAME);
This isn't equivalent with the way line breaks currently work in Blink. Is that
intended? Most significantly, the name here includes non-rendered content like
aria labels, images, etc. See my previous comment regarding inline textboxes.
Not sure if line breaks in Blink worked as we wanted either...

[David Tseng, 2016-09-23 12:43:03]

https://codereview.chromium.org/2301833005/diff/260001/ui/accessibility/ax_no...
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2301833005/diff/260001/ui/accessibility/ax_no...
ui/accessibility/ax_node.cc:76: child->ComputeLineStartOffsets(line_offsets,
end_offset);
Also, before I forget, if performance is a concern here, you could just visit
mostly only the leaves (pseudocode):
leaf = start.firstChild
while (leaf) {
  while (leaf.firstChild)
    leaf = leaf.firstChild
  visit(leaf)
  leaf = leaf.nextSibling || leaf.parent
  if (leaf == start)
    return
}

rather than visiting the entire tree recursively.

(and this leads to having possibly an incremental api, getNextLineBreakStart,
depending on re-writing the callers).

Just a note to remind us if needed.

[chromium-reviews, 2016-09-23 16:14:16]

ui/accessibility/ax_node.cc:76:
> child->ComputeLineStartOffsets(line_offsets, end_offset);
> Also, before I forget, if performance is a concern here, you could just
> visit mostly only the leaves (pseudocode):


Indeed. This is what I currently do in 
BrowserAccessibilityManager::Next/PreviousTextOnlyObject functions. But 
it needs to be moved to AXPosition.

-- 
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-09-24 00:15:26]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2016-09-24 00:16:01]

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-09-24 01:25:18]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-09-24 01:28:21]

Description was changed from

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
TESTED=Manually with Voiceover ChromeVox Jaws and NVDA, existing Windows browser
tests, new Automation API tests, existing ChromeVox tests
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Get rid of AX_LINE_BREAKS attribute to improve performance and clarity.

1. Attribute renamed to line_start_offsets.
2. Now computed on browser side and cached.
3. Attribute should not be used directly but line start offsets should be
retrieved via |AXNode::GetOrComputeLineStartOffsets|.
4. Should work on all kinds of text fields and both in automation and other
platforms.
TESTED=Manually with Voiceover ChromeVox Jaws and NVDA, existing Windows browser
tests, new Automation API tests, existing ChromeVox tests
R=dmazzoni@chromium.org, dtseng@chromium.org
BUG=642337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/91926613c70561f2ddeb86896496e7306518c747
Cr-Commit-Position: refs/heads/master@{#420807}
==========

[commit-bot: I haz the power, 2016-09-24 01:28:23]

Patchset 14 (id:??) landed as
https://crrev.com/91926613c70561f2ddeb86896496e7306518c747
Cr-Commit-Position: refs/heads/master@{#420807}

[David Tseng, 2016-10-06 20:44:20]

Sorry guys; this cl regressed text area/content editable behaviors e.g.
crbug.com/652059. We're now returning the wrong line break information. If no
one has extreme objections, I'm going to revert.

[David Tseng, 2016-10-06 21:28:03]

I've worked around the issue commented on here in:
https://codereview.chromium.org/2395293002/

but this will probably bite other screen readers similarly to varying degrees.

https://codereview.chromium.org/2301833005/diff/260001/ui/accessibility/ax_no...
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2301833005/diff/260001/ui/accessibility/ax_no...
ui/accessibility/ax_node.cc:83: line_offsets->push_back(*end_offset);
This is sometimes wrong when you're on the last leaf node of a textarea or
content editable. You can't just assume there's a line break start here.

[dmazzoni, 2016-10-10 05:07:24]

Nektarios's change was a step in the right direction. AX_LINE_BREAKS was
horrible from a performance perspective. I'm not surprised that there are
some corner cases that the new code got wrong, but I think the right
solution is to fix them, not revert. Write up a unit test that fails and
I'll happily help fix it.

We can't be afraid to make big changes that cause some regressions if it
improves the code overall.

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

[chromium-reviews, 2016-10-10 13:50:20]

I didn't handle the situation at the end of the object correctly. There 
might or there might not be a line break at the end of an object. It 
would have been better to never return anything.
Similarly, I should handle the possibility of a line break at the 
beginning of an object. Now, if there is a start of a line at the 
beginning, I don't return 0 as an offset.

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

[chromium-reviews, 2016-10-10 15:43:15]

Dominic,
usually, when regressions occur, reverting is what I've seen happen to
reduce confusion especially considering now that a merge is required making
a bit more work for everyone. The particular failing case was discussed
offline. We can enough to fix without introducing more bugs.


Nektarios, if you are fine with fixing this as a beta block, then I guess
we can leave the cl in.

Also, can you rename line start offsets? It doesn't really fit since the
first line is not covered.

 Sorry for pushing on this, but as an api, it should be given more scrutiny
and not rushed through for other reasons. Thanks!

On Mon, Oct 10, 2016 at 6:50 AM, 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> I didn't handle the situation at the end of the object correctly. There
> might or there might not be a line break at the end of an object. It would
> have been better to never return anything.
> Similarly, I should handle the possibility of a line break at the
> beginning of an object. Now, if there is a start of a line at the
> beginning, I don't return 0 as an offset.
>
>
> --
> 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.
>
>

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

[chromium-reviews, 2016-10-10 16:46:12]

Please see bug and include me on the fix
https://bugs.chromium.org/p/chromium/issues/detail?id=652059

Assigning to nektar@ for underlying regression.

Line breaks should from at least ChromeVox's perspective and probably most
other screen readers work as follows:
- consider leaves that are *text* nodes. Text nodes are nodes who's
accessible name is text from the *DOM*. Thus, nodes with roles of static
text, line break should be included. However, nodes of other types who's
names from are not from contents, should not be considered like divs with
an raia label and that are also leaves.
- line breaks, as calculated by the AX module in ui/, should:
provide the index where a line break occurs. If it is a hard line break,
the index should contain a new line character. If it is a soft line break,
the index should fall on the start of the next line and the client should
use affinity to resolve where the caret actually is.


On Mon, Oct 10, 2016 at 8:43 AM, David Tseng <dtseng@google.com> wrote:

> Dominic,
> usually, when regressions occur, reverting is what I've seen happen to
> reduce confusion especially considering now that a merge is required making
> a bit more work for everyone. The particular failing case was discussed
> offline. We can enough to fix without introducing more bugs.
>
>
> Nektarios, if you are fine with fixing this as a beta block, then I guess
> we can leave the cl in.
>
> Also, can you rename line start offsets? It doesn't really fit since the
> first line is not covered.
>
>  Sorry for pushing on this, but as an api, it should be given more
> scrutiny and not rushed through for other reasons. Thanks!
>
> On Mon, Oct 10, 2016 at 6:50 AM, 'Nektarios Paisios' via Chromium-reviews
> <chromium-reviews@chromium.org> wrote:
>
>> I didn't handle the situation at the end of the object correctly. There
>> might or there might not be a line break at the end of an object. It would
>> have been better to never return anything.
>> Similarly, I should handle the possibility of a line break at the
>> beginning of an object. Now, if there is a start of a line at the
>> beginning, I don't return 0 as an offset.
>>
>>
>> --
>> 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.
>>
>>
>

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