Fixed word and line navigation in multi-line text fields.

Fixed word and line navigation in multi-line text fields.

This might also fix the line break issue where lines appear joined together.
This CL should be submitted only after the Blink change removing a spurious line break at the end of all text fields is rolled into Chromium.
BUG=102231
R=dmazzoni

Committed: https://crrev.com/28f5f6444f311879cff2e7688011a54bf40a463b
Cr-Commit-Position: refs/heads/master@{#327629}

[dmazzoni, 2015-04-29 05:50:15]

Looks fantastic!

Give me some time to do a thorough review of this but overall I really like what
I see.

[dmazzoni, 2015-04-29 21:49:50]

Don't forget to add a bug number.

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

https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:329:
DCHECK_EQ(child->GetRole(), ui::AX_ROLE_INLINE_TEXT_BOX);
I think there's a bug that could trip this: I just stumbled upon a place in
Blink where we return a role of static text for something other than an actual
text node, so then it's possible it has a child that's not an inline text box.
Consider doing a real test for this role with a warning, not a debug check:

if (child->GetRole() != ui::AX_ROLE_INLINE_TEXT_BOX) {
  LOG(WARNING) << "..."
  continue;
}

https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:353: word_start =
child_start + *(iter - 1);
Is this safe if iter points to the first element in word_starts?

https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:377: return
unknown_offset;
I think it'd be more clear to use something like "end_of_text_length" here
rather than unknown_offset. You could define two variables at the top of the
function since they have somewhat different purposes even though they happen to
be the same value.

https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:383: return
unknown_offset;
What happens if you have an element with a lot of static text children but no
words? Does it make sense to return the end offset like this?

https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:396: if ((word_start !=
unknown_offset &&
nit: indentation

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

https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win_unittest.cc:381: /*
Commented out on purpose? Do these assertions no longer pass?

[chromium-reviews, 2015-04-29 23:08:35]

Don't forget to add a bug number.


> I couldn't find the number of my old word navigation bug quickly 
> that's why I didn't have a number originally.
Used related bug instead.

>
>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

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

>
> content/browser/accessibility/browser_accessibility.cc:329:
> DCHECK_EQ(child->GetRole(), ui::AX_ROLE_INLINE_TEXT_BOX);
> I think there's a bug that could trip this: I just stumbled upon a place
> in Blink where we return a role of static text for something other than
> an actual text node, so then it's possible it has a child that's not an
> inline text box. Consider doing a real test for this role with a
> warning, not a debug check:
>
> if (child->GetRole() != ui::AX_ROLE_INLINE_TEXT_BOX) {
>   LOG(WARNING) << "..."
>   continue;
> }
>
Done but used DLOG instead of LOG.

>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility.cc:353: word_start =
> child_start + *(iter - 1);
> Is this safe if iter points to the first element in word_starts?
>

I think because I say:
if (iter != word_starts.end()) {
...
if (iter == word_starts.begin()) {
...
} else {
Do the above.
}
...
}
So, it's not == end() and not equals begin(), so *(iter - 1) should be 
valid?


>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility.cc:377: return
> unknown_offset;
> I think it'd be more clear to use something like "end_of_text_length"
> here rather than unknown_offset. You could define two variables at the
> top of the function since they have somewhat different purposes even
> though they happen to be the same value.
>
I don't like end_of_text_length because it doesn't say what it stands 
for. Actually, as I say in the header file text length is a special 
offset that is returned when the word boundary has not been found.
So, I renamed the variable from unknown_offset to word_start_not_found 
to make it clear what this constant stands for.

>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility.cc:383: return
> unknown_offset;
> What happens if you have an element with a lot of static text children
> but no words? Does it make sense to return the end offset like this?
>
It's the end offset but it stands for word_start_not_found according to 
the header file. This function should behave the same regardless if the 
caller is another function or the function itself.


>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility.cc:396: if
> ((word_start != unknown_offset &&
> nit: indentation
>
Fixed.

>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

>
> File content/browser/accessibility/browser_accessibility_win_unittest.cc
> (right):
>
>
https://codereview.chromium.org/1111163002/diff/1/content/browser/accessibili...

>
> content/browser/accessibility/browser_accessibility_win_unittest.cc:381:
> /*
> Commented out on purpose? Do these assertions no longer pass?


Yes, I had to add code with the inline text boxes. Fixed now.
Also, changed all ASSERT_FOO to EXPECT_FOO, except where it didn't make 
sense.
Please review the whole function.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-04-29 23:12:50]

lgtm

I don't see the rename to word_start_not_found but everything else sounds good,
thanks.

[nektarios, 2015-04-30 00:27:21]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-04-30 00:27:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1111163002/#ps40001
(title: "Addressed first bunch of comments.")

[commit-bot: I haz the power, 2015-04-30 00:27:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1111163002/40001

[commit-bot: I haz the power, 2015-04-30 00:31:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-30 00:32:14]

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