Fixed line start offsets.

Fixed line start offsets.

Now they indeed refer to the offset at the start of each line. This means that if e.g. the first text node starts a new line, the first offset returned should be 0. Further, the offset equivalent to the text's length should never be returned.
I chose to stick with line start offsets instead of line breaks because what exactly is a line break? The beginning of the line, the end of the previous line or the line break character in between two lines when it exists? And what happens if there is no line break character, i.e. a soft line break?
Line start offsets are less ambiguous.
BUG=652059
R=dmazzoni@chromium.org, dtseng@chromium.org
TESTED=automation browsertests, manually with ChromeVox on ChromeOS

Committed: https://crrev.com/6be580cbb1b4947179978c1c0f2dd9c680a99863
Cr-Commit-Position: refs/heads/master@{#426636}

[nektarios, 2016-10-19 16:53:27]

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

[commit-bot: I haz the power, 2016-10-19 16:53:55]

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

[dmazzoni, 2016-10-19 17:34:28]

I'm totally fine with the definition of line start offsets,
but the previous line breaks array never had 0 in the array.
For a single line of text it'd be an empty array, and now
it looks like the first element in the array is always 0.

I'm concerned that this might lead to different behavior
where it thinks there's an empty line of text before
the first line

[David Tseng, 2016-10-19 17:39:42]

Sorry, this is still wrong.

https://codereview.chromium.org/2437473003/diff/1/ui/accessibility/ax_node.cc
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2437473003/diff/1/ui/accessibility/ax_node.cc...
ui/accessibility/ax_node.cc:80: if
(!child->data().HasIntAttribute(ui::AX_ATTR_PREVIOUS_ON_LINE_ID))
This is still wrong...

- this would include 0; there's *never* a line break at the beginning of the
content (i.e. before the first character in a content editable)
- this would include leaf nodes that don't have any accessible text like a div
with an empty aria label.
- this would not handle the case where the last character is a hard line break

I think you really just want inline textbox nodes.

Also, the confusion over start and end and break: I would think of the line
index as the index in-between characters that involve a line break (either a
hard or soft line break). This would allow us to then talk about a line break
between two characters, before the first character, after the last character,
etc.

[chromium-reviews, 2016-10-19 18:24:23]

On 10/19/2016 1:34 PM, dmazzoni@chromium.org wrote:
> I'm totally fine with the definition of line start offsets,
> but the previous line breaks array never had 0 in the array.
> For a single line of text it'd be an empty array, and now
> it looks like the first element in the array is always 0.
>
> I'm concerned that this might lead to different behavior
> where it thinks there's an empty line of text before
>
Having 0 in there is correct. There are cases where it could matter.
For example, having an embedded object like a link that spans multiple 
lines. Having 0 means that the link itself starts at the beginning of 
the line that is in, whilst not having a 0 would mean that the link 
starts in the middle of a line.
However, since this usecase hasn't been tested, I removed the 0.

-- 
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-10-19 18:31:35]

Is there any case where we use the line start offsets for anything other
than the root node of an editable text control? There shouldn't be.

This function is only supposed to be temporary until AXPosition can replace
it. Imagine a contenteditable with 1000 lines. It shouldn't be necessary to
compute all of the line start offsets just to jump to the beginning of the
current line.

On Wed, Oct 19, 2016 at 11:24 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> On 10/19/2016 1:34 PM, dmazzoni@chromium.org wrote:
>
> I'm totally fine with the definition of line start offsets,
> but the previous line breaks array never had 0 in the array.
> For a single line of text it'd be an empty array, and now
> it looks like the first element in the array is always 0.
>
> I'm concerned that this might lead to different behavior
> where it thinks there's an empty line of text before
>
> Having 0 in there is correct. There are cases where it could matter.
> For example, having an embedded object like a link that spans multiple
> lines. Having 0 means that the link itself starts at the beginning of the
> line that is in, whilst not having a 0 would mean that the link starts in
> the middle of a line.
> However, since this usecase hasn't been tested, I removed the 0.
>
> --
> 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-19 18:35:31]

(!child->data().HasIntAttribute(ui::AX_ATTR_PREVIOUS_ON_LINE_ID))
> This is still wrong...
>
> - this would include 0; there's *never* a line break at the beginning of
> the content (i.e. before the first character in a content editable)

Not every node that we might need to call this function on would be a 
content editable.
There might be a new line before the start of an object or not, e.g. a 
link object that starts at the beginning of a line or in the middle of a 
line.
However, I removed 0 for now to prevent confusion.

> - this would include leaf nodes that don't have any accessible text like
> a div with an empty aria label.

I don't understand. If the label is empty how could it affect the offset 
calculation?

> - this would not handle the case where the last character is a hard line
> break
>
Why? There should be an empty inline text box after the hard line break 
in which the caret would be placed. This would cause the algorithm to 
return the character offset of the empty inline text box as the 
beginning of a new line.


> I think you really just want inline textbox nodes.
>
Not all accessible text is an inline text box. I know it's not 
selectable text using the browser's commands, but still it could start a 
line and the accessibility API should report that, because Jaws Cursor 
relies on this.
We should have a different way of reporting if the caret could traverse 
some offsets or not.

> Also, the confusion over start and end and break: I would think of the
> line index as the index in-between characters that involve a line break
> (either a hard or soft line break). This would allow us to then talk
> about a line break between two characters, before the first character,
> after the last character, etc.

This is confusing and has bitten me at least once in the past. Character 
offsets don't go between characters and when you respond to an 
accessibility API request it's a character offset that you are after not 
an index.
Conceptually though, what's the difference?
If there is a line break between character offsets 4 and 3, then we 
would define the line break index to be equal to 4, and also a new line 
would start at character offset 4.
So, the values are identical. The naming is less confusing.

-- 
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-10-19 18:56:38]

On Wed, Oct 19, 2016 at 11:35 AM, 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> (!child->data().HasIntAttribute(ui::AX_ATTR_PREVIOUS_ON_LINE_ID))
>
> This is still wrong...
>
> - this would include 0; there's *never* a line break at the beginning of
> the content (i.e. before the first character in a content editable)
>
>
> Not every node that we might need to call this function on would be a
> content editable.
> There might be a new line before the start of an object or not, e.g. a
> link object that starts at the beginning of a line or in the middle of a
> line.
> However, I removed 0 for now to prevent confusion.
>
> It's fine to include 0; you'll just have to fix  automation where we do
expect a line break offset and not a start of line offset

- this would include leaf nodes that don't have any accessible text like
> a div with an empty aria label.
>
>
> I don't understand. If the label is empty how could it affect the offset
> calculation?
>
> If I have multiple divs of this type each on a new line, you would add
that many lines at the same offset.



> - this would not handle the case where the last character is a hard line
> break
>
> Why? There should be an empty inline text box after the hard line break in
> which the caret would be placed. This would cause the algorithm to return
> the character offset of the empty inline text box as the beginning of a new
> line.
>
> Nope, not what I'm seeing:
<p>this is<br></p>
results in a static text node followed by a line break node. These two
nodes are on the same line. There's no other inline text boxes.



>
> I think you really just want inline textbox nodes.
>
> Not all accessible text is an inline text box. I know it's not selectable
> text using the browser's commands, but still it could start a line and the
> accessibility API should report that, because Jaws Cursor relies on this.
>

This is wrong imo. What does it mean for non-rendered text to be in a line.
A line is closely tied to visual rendering.


> We should have a different way of reporting if the caret could traverse
> some offsets or not.
>
> Also, the confusion over start and end and break: I would think of the
> line index as the index in-between characters that involve a line break
> (either a hard or soft line break). This would allow us to then talk
> about a line break between two characters, before the first character,
> after the last character, etc.
>
>
> This is confusing and has bitten me at least once in the past. Character
> offsets don't go between characters and when you respond to an
> accessibility API request it's a character offset that you are after not an
> index.
> Conceptually though, what's the difference?
> If there is a line break between character offsets 4 and 3, then we would
> define the line break index to be equal to 4, and also a new line would
> start at character offset 4.
> So, the values are identical. The naming is less confusing.
>
> --
> 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.

[dmazzoni, 2016-10-19 19:16:16]

On Wed, Oct 19, 2016 at 11:56 AM David Tseng <dtseng@chromium.org> wrote:

> However, I removed 0 for now to prevent confusion.
>
> It's fine to include 0; you'll just have to fix  automation where we do
> expect a line break offset and not a start of line offset
>

Not unless you fix the Mac and Windows code that also uses this array and
doesn't expect a 0 at the front. I thought the whole point of this change
was just to do exactly the same calculation we had as a very temporary
performance improvement. As soon as possible we should get away from any
function that computes a really large array of offsets because it's
wasteful.


> Nope, not what I'm seeing:
> <p>this is<br></p>
> results in a static text node followed by a line break node. These two
> nodes are on the same line. There's no other inline text boxes.
>

Agreed, that's what I see too. However, I can't find any case where this
fails in the middle of a contenteditable, only at the end. In the middle,
every blank line gets its own inline text box. At the very end, there's
not. It looks like if you see an inline text box consisting of only a
newline, at the very end, you should add a line break offset after that one.

Not all accessible text is an inline text box. I know it's not selectable
> text using the browser's commands, but still it could start a line and the
> accessibility API should report that, because Jaws Cursor relies on this.
>
>
> This is wrong imo. What does it mean for non-rendered text to be in a
> line. A line is closely tied to visual rendering.
>

So what about this case: you have a line of text with an inline image with
alt text in the middle. The image doesn't have an inline text box, but
because the image is inline, it's on the same line as the text before and
after the image.

-- 
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-10-19 19:28:17]

Sure, but what does it mean to include the image's alt text in the
offset calculation? The whole point of having an offset is so that a
client can in return make calls to our api for setting selection,
retrieving selection, figuring out the position in the accessibility
tree, etc. If we put selection after the image, then we'll have to
translate a position from Blink to account for the name offset. If a
client selects part of the image's alt text, then we'll have to
account for that case as well. The issue with this is if the text is
really unselectable as it will be with aria label, then we'll never
get an event back from Blink when a client attempts to select it.

Again, this whole discussion goes back to a need for a good spec to
guide the decisions we're trying to make before trying to dive deep
into code level reviews. I don't know the requirements/constraints for
Jaws/NVDA, but I can argue for the right behavior for ChromeVox which
in some sense is a pure implementation over our accessibility data.

As for the <p>this is <br></p> snippet above, it's a counter example
to making the attribute named line start offset because the last line
would not really be a start offset, but an end offset which is why I
think you're missing that particular case.

-- 
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-19 19:43:13]

As for the <p>this is <br></p> snippet above, it's a counter example to making
the attribute named line start offset because the last line would not really be
a start offset, but an end offset which is why I think you're missing that
particular case.


A start offset equivalent to the text's length means that the text ends 
at the end of a line. That's what I could do.
But that's what I did before and it would always return the text length, 
regardless if there was a line break at the end or not.
I think this should be fixed on the Blink side. There should be an empty 
inline text box if the caret is on an empty line after the text. Because 
otherwise I would have to rely on looking for hard line breaks manually, 
which is what we used to do before and it wasn't working for soft line 
breaks.
Now we have a uniform API, GetNextOnLine and GetPreviousOnLine, we 
should fix that instead of looking for "\n" characters.
But again, this is for a followup change. I don't think it should block 
the beta.

-- 
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-10-19 19:54:25]

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

[commit-bot: I haz the power, 2016-10-19 19:55:06]

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

[chromium-reviews, 2016-10-19 19:56:43]

> It's fine to include 0; you'll just have to fix  automation where we 
> do expect a line break offset and not a start of line offset
>
Excluding 0 is the safest option for this temporary change.

>>     - this would include leaf nodes that don't have any accessible
>>     text like
>>     a div with an empty aria label.
>
>     I don't understand. If the label is empty how could it affect the
>     offset calculation?
>
> If I have multiple divs of this type each on a new line, you would add 
> that many lines at the same offset.
>
>
Okay, I took care of that too.

-- 
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-10-19 20:01:19]

lgtm

[commit-bot: I haz the power, 2016-10-19 20:46:56]

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

[commit-bot: I haz the power, 2016-10-19 20:46:56]

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, 2016-10-20 16:03:14]

lgtm

Final line break case is still broken, but let's fix content editables in
general and get this merged. Thanks.

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

https://codereview.chromium.org/2437473003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node.cc:80: // Don't report if the first piece of text
starts a new line or not.
When is end of previous sline ever not equal to start of next line? That's why
the wording of start is confusing.

https://codereview.chromium.org/2437473003/diff/40001/ui/accessibility/ax_nod...
ui/accessibility/ax_node.cc:85: if (line_offsets->empty() ||
line_offsets->back() != *start_offset) {
nit: no curlies

[nektarios, 2016-10-20 21:45:01]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-10-20 21:45:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2437473003/#ps60001
(title: "Fixed tests and nits.")

[commit-bot: I haz the power, 2016-10-20 21:45:41]

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

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

https://codereview.chromium.org/2437473003/diff/40001/ui/accessibility/ax_nod...
> File ui/accessibility/ax_node.cc (right):
>
>
https://codereview.chromium.org/2437473003/diff/40001/ui/accessibility/ax_nod...
> ui/accessibility/ax_node.cc:80: // Don't report if the first piece of
> text starts a new line or not.
> When is end of previous sline ever not equal to start of next line?
> That's why the wording of start is confusing.
>
When there is a hard line break in between the lines. I think we'll need 
this distinction on the Mac. So, NextLineEndPosition might be different 
from NextLineStartPosition.
Let's continue the discussion in the updated AXPosition design Doc I'll 
send out.
Line break index has confused me in the past because when you have a 
hard line break does the index point to the line break itself or in 
between the hard line break and the next character?
After you explained to me that indices point between characters, I got 
it, but it's still confusing in my opinion because I never worked with 
indices that point between characters. If you have a string for example, 
indices point to characters, not between characters.




>
https://codereview.chromium.org/2437473003/diff/40001/ui/accessibility/ax_nod...
> ui/accessibility/ax_node.cc:85: if (line_offsets->empty() ||
> line_offsets->back() != *start_offset) {
> nit: no curlies
>
Done.

-- 
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-10-20 22:15:13]

Description was changed from

==========
Fixed line start offsets.

Now they indeed refer to the offset at the start of each line. This means that
if e.g. the first text node starts a new line, the first offset returned should
be 0. Further, the offset equivalent to the text's length should never be
returned.
I chose to stick with line start offsets instead of line breaks because what
exactly is a line break? The beginning of the line, the end of the previous line
or the line break character in between two lines when it exists? And what
happens if there is no line break character, i.e. a soft line break?
Line start offsets are less ambiguous.
BUG=652059
R=dmazzoni@chromium.org, dtseng@chromium.org
TESTED=automation browsertests
==========

to

==========
Fixed line start offsets.

Now they indeed refer to the offset at the start of each line. This means that
if e.g. the first text node starts a new line, the first offset returned should
be 0. Further, the offset equivalent to the text's length should never be
returned.
I chose to stick with line start offsets instead of line breaks because what
exactly is a line break? The beginning of the line, the end of the previous line
or the line break character in between two lines when it exists? And what
happens if there is no line break character, i.e. a soft line break?
Line start offsets are less ambiguous.
BUG=652059
R=dmazzoni@chromium.org, dtseng@chromium.org
TESTED=automation browsertests, manually with ChromeVox on ChromeOS
==========

[commit-bot: I haz the power, 2016-10-20 23:08:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:24:08]

Description was changed from

==========
Fixed line start offsets.

Now they indeed refer to the offset at the start of each line. This means that
if e.g. the first text node starts a new line, the first offset returned should
be 0. Further, the offset equivalent to the text's length should never be
returned.
I chose to stick with line start offsets instead of line breaks because what
exactly is a line break? The beginning of the line, the end of the previous line
or the line break character in between two lines when it exists? And what
happens if there is no line break character, i.e. a soft line break?
Line start offsets are less ambiguous.
BUG=652059
R=dmazzoni@chromium.org, dtseng@chromium.org
TESTED=automation browsertests, manually with ChromeVox on ChromeOS
==========

to

==========
Fixed line start offsets.

Now they indeed refer to the offset at the start of each line. This means that
if e.g. the first text node starts a new line, the first offset returned should
be 0. Further, the offset equivalent to the text's length should never be
returned.
I chose to stick with line start offsets instead of line breaks because what
exactly is a line break? The beginning of the line, the end of the previous line
or the line break character in between two lines when it exists? And what
happens if there is no line break character, i.e. a soft line break?
Line start offsets are less ambiguous.
BUG=652059
R=dmazzoni@chromium.org, dtseng@chromium.org
TESTED=automation browsertests, manually with ChromeVox on ChromeOS

Committed: https://crrev.com/6be580cbb1b4947179978c1c0f2dd9c680a99863
Cr-Commit-Position: refs/heads/master@{#426636}
==========

[commit-bot: I haz the power, 2016-10-21 13:24:09]

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