Expose InlineTextBoxes in the accessibility tree.

Expose InlineTextBoxes in the accessibility tree.

The goal here is for the serialized accessibility tree exposed by
Blink to have enough information that the embedder can compute the
bounding rectangle for an arbitrary text range, and determine the
word that any given character offset belongs to. Supporting text with
rotations or 3-D transforms is a non-goal.

This will enable us to complete the implementation of native accessibility
APIs that request the bounding box of text ranges, and make it possible to
enable software like ZoomText to highlight individual words on a webpage as
they're spoken.

Previously, accessibility objects with a role of StaticText would be
leaf nodes. Now we expose all of the InlineTextBoxes within that object
using a new type, AccessibilityInlineTextBox, and furthermore we expose
the pixel offset of each character within the inline text box, and the
start and end position of all words within each inline text box. Every
InlineTextBox has a single direction - right, left, up, or down.

This change also makes it more efficient to update the accessibility tree
when the user is editing a large textarea or contenteditable. Previously,
the entire contents had to be updated every time. Now typically only the
InlineTextBoxes corresponding to the current paragraph need to change.

Making this change would break some Chromium tests, so we add a new flag
to enable this new functionality. Once Chromium has been updated to depend
on having inline text boxes in the accessibility tree, we can remove the
flag.

BUG=98977
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160020

[dmazzoni, 2013-09-05 07:08:45]

aboxhall: primary review
esprehn: could you please help review this and make sure my use of InlineTextBox
and the changes to it are sane?
dglazkov: public/

[dglazkov, 2013-09-05 17:23:26]

lgtm public/ stuff.

[aboxhall, 2013-09-06 17:19:20]

This is great!

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/linu...
File
LayoutTests/platform/linux/accessibility/inline-text-word-boundaries-expected.txt
(right):

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/linu...
LayoutTests/platform/linux/accessibility/inline-text-word-boundaries-expected.txt:47:
Character 38: 'x' word=(38, 39): 'x'
Why is this x on its own?

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/mac/...
File
LayoutTests/platform/mac/accessibility/inline-text-word-boundaries-expected.txt
(right):

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/mac/...
LayoutTests/platform/mac/accessibility/inline-text-word-boundaries-expected.txt:45:
Character 36: 'x' word=(36, 39): 'xxx'
Weirdness happening here as well. Does each platform have a maximum word length?

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:124: pos =
textBreakNext(iterator);
Out of curiosity, how does this deal with spaces? Does textBreakPrevious return
the start of the previous word, while textBreakNext returns the end of the
current word?

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:142: if (Node* node =
renderText->node()) {
Why is this a special case?

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityNodeObject.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityNodeObject.cpp:1384: if
(!axObjectCache()->inlineTextBoxAccessibility())
return axObjectCache()->inlineTextBoxAccessibility() ? Or is it desirable to
fall through to the default here?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
File Source/testing/runner/AccessibilityUIElementChromium.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:365:
WEBKIT_ASSERT(characterOffsets.size() > 0 && characterOffsets.size() ==
child.stringValue().length());
Is checking characterOffsets.size() > 0 redundant?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:366: if
(characterOffsets[characterOffsets.size() - 1] < 0) {
Why last offset rather than first?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:371: return
WebRect(right + characterOffsets[index], childRect.y, characterOffsets[index -
1] - characterOffsets[index], childRect.height);
This is breaking my brain... would it be a problem to pull it out something like
int left = right + characterOffsets[index];
int width = characterOffsets[index - 1] - characterOffsets[index];
return WebRect(left, childRect.y, width, childRect.height);
...? All the double negatives are pretty hard to follow when you're also keeping
track of what argument you're up to.

(It might also be helpful to do something similar in the if(index) case for LTR
below as well, just for comparison.)

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:402: // If there are no
words, use the InlineTextBox boundaries.
Why would there be no words?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:409: // Look for a
character within any word other than the last.
Why not the last?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:411: if (characterIndex
<= ends[j]) {
Should this be index?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:1143: void
AccessibilityUIElement::wordStartCallback(const CppArgumentList& arguments,
CppVariant* result)
Are we likely to mostly be calling this and wordEnd() in rapid succession? Is
there any way to cache the result of getWordBoundaries() for a given character
so we don't need to recalculate it if that is the case? Alternatively, is there
any way we could combine these two methods and return a start, end pair instead?

[dmazzoni, 2013-09-06 20:25:26]

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/linu...
File
LayoutTests/platform/linux/accessibility/inline-text-word-boundaries-expected.txt
(right):

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/linu...
LayoutTests/platform/linux/accessibility/inline-text-word-boundaries-expected.txt:47:
Character 38: 'x' word=(38, 39): 'x'
On 2013/09/06 17:19:21, aboxhall wrote:
> Why is this x on its own?

That's on purpose. The style of the element is set to "width: 8em; word-wrap:
break-word;" and there are enough x's that it wraps to the next line. This tests
that when a word is split between multiple lines it's treated as two separate
words from the perspective of this API - which I think is correct behavior given
how word APIs are used by assistive technology. Otherwise the user wouldn't
realize the word wrapped.

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/mac/...
File
LayoutTests/platform/mac/accessibility/inline-text-word-boundaries-expected.txt
(right):

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/mac/...
LayoutTests/platform/mac/accessibility/inline-text-word-boundaries-expected.txt:45:
Character 36: 'x' word=(36, 39): 'xxx'
On 2013/09/06 17:19:21, aboxhall wrote:
> Weirdness happening here as well. Does each platform have a maximum word
length?

Font differences. Hard to avoid in a test that's specifically all about text
layout.

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:124: pos =
textBreakNext(iterator);
On 2013/09/06 17:19:21, aboxhall wrote:
> Out of curiosity, how does this deal with spaces? Does textBreakPrevious
return
> the start of the previous word, while textBreakNext returns the end of the
> current word?

That's what it seems to do, yes. It's a bit confusing to me, we need someone
who knows the text iterators to review this and make sure this is the right
way to do it.

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:142: if (Node* node =
renderText->node()) {
On 2013/09/06 17:19:21, aboxhall wrote:
> Why is this a special case?

This is the common case. What we want is the "plain text" rendering of the text,
for example if the DOM contains literally <span>    Hello,   world  </span> then
we want just "Hello, world" as the string value, since that's what's actually
rendered. This does the right thing with regards to implicit spaces too.

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:149: return
renderText->text().substring(start, len);
This is the uncommon case. We only reach this for cases like CSS-inserted text.

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityNodeObject.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityNodeObject.cpp:1384: if
(!axObjectCache()->inlineTextBoxAccessibility())
On 2013/09/06 17:19:21, aboxhall wrote:
> return axObjectCache()->inlineTextBoxAccessibility() ? Or is it desirable to
> fall through to the default here?

I find it slightly more clear to have it fall through - i.e. read the code as
"StaticTextRole should not have children, unless the inlineTextBoxAccessibility
flag is on".

I would find "return axObjectCache()->inlineTextBoxAccessibility()" less clear,
given the comment at the top of the switch. But I could move the test outside of
the switch with its own comment if you think that would be more clear.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
File Source/testing/runner/AccessibilityUIElementChromium.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:365:
WEBKIT_ASSERT(characterOffsets.size() > 0 && characterOffsets.size() ==
child.stringValue().length());
On 2013/09/06 17:19:21, aboxhall wrote:
> Is checking characterOffsets.size() > 0 redundant?

I want to assert that they're both not zero. It'd be just as surprising if
child.stringValue() was empty.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:366: if
(characterOffsets[characterOffsets.size() - 1] < 0) {
On 2013/09/06 17:19:21, aboxhall wrote:
> Why last offset rather than first?

I was worried about a corner case where the first character's width happens to
be zero (especially since it's rounded). Since the offsets are accumulative,
checking the last offset is better.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:402: // If there are no
words, use the InlineTextBox boundaries.
On 2013/09/06 17:19:21, aboxhall wrote:
> Why would there be no words? 

An inline text box could have just whitespace, for example. The word break
iterator would detect no complete words.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:409: // Look for a
character within any word other than the last.
On 2013/09/06 17:19:21, aboxhall wrote:
> Why not the last?

Only because that case is handled below - any character past the last word is
treated as part of the last word.

Remember, this code is ONLY to enable layout tests. This implementation is
somewhat arbitrary.

The equivalent Mac and Win code on the Chromium side will
have to do the right thing, but I think those APIs are
more specific, too.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:411: if (characterIndex
<= ends[j]) {
On 2013/09/06 17:19:21, aboxhall wrote:
> Should this be index?

You're totally right. Fixed.

I renamed it inlineBoxRelativeIndex to be totally clear.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:1143: void
AccessibilityUIElement::wordStartCallback(const CppArgumentList& arguments,
CppVariant* result)
On 2013/09/06 17:19:21, aboxhall wrote:
> Are we likely to mostly be calling this and wordEnd() in rapid succession? Is
> there any way to cache the result of getWordBoundaries() for a given character
> so we don't need to recalculate it if that is the case? Alternatively, is
there
> any way we could combine these two methods and return a start, end pair
instead?

This code is only for enabling layout tests, so this seemed simpler. For the
'actual' implementation of this method on the Chromium side
we should worry about efficiency.

[aboxhall, 2013-09-06 20:51:12]

lgtm

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/linu...
File
LayoutTests/platform/linux/accessibility/inline-text-word-boundaries-expected.txt
(right):

https://codereview.chromium.org/23983002/diff/10001/LayoutTests/platform/linu...
LayoutTests/platform/linux/accessibility/inline-text-word-boundaries-expected.txt:47:
Character 38: 'x' word=(38, 39): 'x'
On 2013/09/06 20:25:27, Dominic Mazzoni wrote:
> On 2013/09/06 17:19:21, aboxhall wrote:
> > Why is this x on its own?
> 
> That's on purpose. The style of the element is set to "width: 8em; word-wrap:
> break-word;" and there are enough x's that it wraps to the next line. This
tests
> that when a word is split between multiple lines it's treated as two separate
> words from the perspective of this API - which I think is correct behavior
given
> how word APIs are used by assistive technology. Otherwise the user wouldn't
> realize the word wrapped.

Ahh got it. How about replacing the x's with "averylongwordthatwillwrap" - or is
the word break detection smart enough to detect the words?

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityNodeObject.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/core/accessibility...
Source/core/accessibility/AccessibilityNodeObject.cpp:1384: if
(!axObjectCache()->inlineTextBoxAccessibility())
On 2013/09/06 20:25:27, Dominic Mazzoni wrote:
> On 2013/09/06 17:19:21, aboxhall wrote:
> > return axObjectCache()->inlineTextBoxAccessibility() ? Or is it desirable to
> > fall through to the default here?
> 
> I find it slightly more clear to have it fall through - i.e. read the code as
> "StaticTextRole should not have children, unless the
inlineTextBoxAccessibility
> flag is on".
> 
> I would find "return axObjectCache()->inlineTextBoxAccessibility()" less
clear,
> given the comment at the top of the switch. But I could move the test outside
of
> the switch with its own comment if you think that would be more clear.

This is fine. I tend to prefer not to do things like "if (!condition) return
false else return true" but I agree it makes sense semantically here.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
File Source/testing/runner/AccessibilityUIElementChromium.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:371: return
WebRect(right + characterOffsets[index], childRect.y, characterOffsets[index -
1] - characterOffsets[index], childRect.height);
On 2013/09/06 17:19:21, aboxhall wrote:
> This is breaking my brain... would it be a problem to pull it out something
like
> int left = right + characterOffsets[index];
> int width = characterOffsets[index - 1] - characterOffsets[index];
> return WebRect(left, childRect.y, width, childRect.height);
> ...? All the double negatives are pretty hard to follow when you're also
keeping
> track of what argument you're up to.
> 
> (It might also be helpful to do something similar in the if(index) case for
LTR
> below as well, just for comparison.)

Ping? No worries if this isn't worth the effort for layout test-only code.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:411: if (characterIndex
<= ends[j]) {
On 2013/09/06 20:25:27, Dominic Mazzoni wrote:
> On 2013/09/06 17:19:21, aboxhall wrote:
> > Should this be index?
> 
> You're totally right. Fixed.
> 
> I renamed it inlineBoxRelativeIndex to be totally clear.

I'm curious why this wasn't caught by the layout test, I would have thought the
multi-line case would be broken by this.

BTW, what about renaming index -> inlineBoxRelativeIndex in the
boundsForCharacter method too?

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:1143: void
AccessibilityUIElement::wordStartCallback(const CppArgumentList& arguments,
CppVariant* result)
On 2013/09/06 20:25:27, Dominic Mazzoni wrote:
> On 2013/09/06 17:19:21, aboxhall wrote:
> > Are we likely to mostly be calling this and wordEnd() in rapid succession?
Is
> > there any way to cache the result of getWordBoundaries() for a given
character
> > so we don't need to recalculate it if that is the case? Alternatively, is
> there
> > any way we could combine these two methods and return a start, end pair
> instead?
> 
> This code is only for enabling layout tests, so this seemed simpler. For the
> 'actual' implementation of this method on the Chromium side
> we should worry about efficiency.

Makes sense.

[esprehn, 2013-09-08 19:05:14]

Is this code so hot that you can't use a Vector<float> ? :)

https://codereview.chromium.org/23983002/diff/24001/Source/core/rendering/Inl...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/24001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1489: void
InlineTextBox::characterWidths(float widths[]) const
Why isn't this a Vector<float> ? This code is scary.

https://codereview.chromium.org/23983002/diff/24001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1507: widths[i] = -widths[i];
*= -1; ?

[dmazzoni, 2013-09-09 07:03:13]

On 2013/09/08 19:05:14, esprehn wrote:
> Is this code so hot that you can't use a Vector<float> ? :)

Well, I want to run it on the whole page, so it is hot...but yeah, I agree I was
over-optimizing there. :)

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
File Source/testing/runner/AccessibilityUIElementChromium.cpp (right):

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:371: return
WebRect(right + characterOffsets[index], childRect.y, characterOffsets[index -
1] - characterOffsets[index], childRect.height);
On 2013/09/06 20:51:12, aboxhall wrote:
> On 2013/09/06 17:19:21, aboxhall wrote:
> > This is breaking my brain... would it be a problem to pull it out something
> like
> > int left = right + characterOffsets[index];
> > int width = characterOffsets[index - 1] - characterOffsets[index];
> > return WebRect(left, childRect.y, width, childRect.height);
> > ...? All the double negatives are pretty hard to follow when you're also
> keeping
> > track of what argument you're up to.
> > 
> > (It might also be helpful to do something similar in the if(index) case for
> LTR
> > below as well, just for comparison.)
> 
> Ping? No worries if this isn't worth the effort for layout test-only code.

Sorry, meant to do this. Done - I like your idea, it's hopefully more readable
now.

https://codereview.chromium.org/23983002/diff/14001/Source/testing/runner/Acc...
Source/testing/runner/AccessibilityUIElementChromium.cpp:411: if (characterIndex
<= ends[j]) {
On 2013/09/06 20:51:12, aboxhall wrote:
> On 2013/09/06 20:25:27, Dominic Mazzoni wrote:
> > On 2013/09/06 17:19:21, aboxhall wrote:
> > > Should this be index?
> > 
> > You're totally right. Fixed.
> > 
> > I renamed it inlineBoxRelativeIndex to be totally clear.
> 
> I'm curious why this wasn't caught by the layout test, I would have thought
the
> multi-line case would be broken by this.

Totally fair point. It wasn't caught because I didn't have an actual "word" on
the
second line of text. I fixed the test so that there's a word on the second line
and now the test fails without this fix.

> BTW, what about renaming index -> inlineBoxRelativeIndex in the
> boundsForCharacter method too?

Done. How about "localIndex" instead - it's basically the same index, but in the
local coordinates of that inline box.

https://codereview.chromium.org/23983002/diff/24001/Source/core/rendering/Inl...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/24001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1489: void
InlineTextBox::characterWidths(float widths[]) const
On 2013/09/08 19:05:15, esprehn wrote:
> Why isn't this a Vector<float> ? This code is scary.

Switched to Vector<float>.

What else is scary?

https://codereview.chromium.org/23983002/diff/24001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1507: widths[i] = -widths[i];
On 2013/09/08 19:05:15, esprehn wrote:
> *= -1; ?

Done.

[eseidel, 2013-09-10 22:22:22]

https://codereview.chromium.org/23983002/diff/31001/Source/core/rendering/Inl...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/31001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1489: void
InlineTextBox::characterWidths(Vector<float>& widths) const
This function seems OK to me.  I'm surprised it's needed (that we can't get this
information some other way w/o adding another method to InlineTextBox".  But I'm
OK with this part of the change.

[dmazzoni, 2013-09-10 22:26:38]

On 2013/09/10 22:22:22, eseidel wrote:
>
https://codereview.chromium.org/23983002/diff/31001/Source/core/rendering/Inl...
> File Source/core/rendering/InlineTextBox.cpp (right):
> 
>
https://codereview.chromium.org/23983002/diff/31001/Source/core/rendering/Inl...
> Source/core/rendering/InlineTextBox.cpp:1489: void
> InlineTextBox::characterWidths(Vector<float>& widths) const
> This function seems OK to me.  I'm surprised it's needed (that we can't get
this
> information some other way w/o adding another method to InlineTextBox".  But
I'm
> OK with this part of the change.

Thanks.

Yes, it would be possible to get this some other way, but it'd be O(n^2) to get
all character widths at once (and we need all of them). I'm pretty sure there
wasn't previously a need to get the bounding box of more than one range at a
time before.

[eseidel, 2013-09-10 22:53:13]

You mentioned offline that this was for highilghting word bounds via AX
software.

Would knowing word-widths be sufficient?  Should we expose a higher level API
than char widths (which can get tricky with ligatures?)  Would you want to just
know line-offsets of word-breaks?  Do you want the space (or other non-rendered
characters) to appear inside or outside your boxes?

Again, I'm OK with this.  But it feels a bit odd to be exposing some of this to
AX.  These are pretty low-level bits of text rendering.

[eseidel, 2013-09-10 22:54:19]

Another oddity here is that AX will have to be text-direction aware with this
setup.  If we exposed "word" boundary rects up to AX from the linebox tree then
we could abstract it away from having to care?

[dmazzoni, 2013-09-10 23:07:00]

On 2013/09/10 22:53:13, eseidel wrote:
> You mentioned offline that this was for highilghting word bounds via AX
> software.
> 
> Would knowing word-widths be sufficient?  Should we expose a higher level API
> than char widths (which can get tricky with ligatures?)

In theory knowing word-widths would be sufficient, but I'm not convinced
that would actually be any simpler or any faster (it'd be faster by a constant
factor at best, and it'd still be O(n^2) if we didn't add a custom API to
InlineTextBox like this.

Also, the accessibility software may decide on its own what a "word" is,
and if we disagree it'd be a worse user experience. So I'd rather focus
on trying to just store the character offsets, and we can revisit if we
need something smaller or faster, etc.

> Would you want to just know line-offsets of word-breaks?

Actually that's what we're doing now, essentially - we're storing the
character offset of each line break. It was sufficient for almost
everything except features that want to highlight individual words.

> Do you want the space (or other non-rendered
> characters) to appear inside or outside your boxes?

That's a good question. I'm not 100% sure, and we may need to tweak this
once it's fully implemented on the Chromium side and we can test it out.

At this point, I want to start with something that's complete enough to
implement the APIs we need, no more complicated than necessary, and
safe (i.e. won't crash, won't return garbage).

[dmazzoni, 2013-09-10 23:10:16]

On 2013/09/10 22:54:19, eseidel wrote:
> Another oddity here is that AX will have to be text-direction aware with this
> setup.  If we exposed "word" boundary rects up to AX from the linebox tree
then
> we could abstract it away from having to care?

That's a fair point.

Luckily the directionality is relatively simple since each InlineTextBox
is either LTR or RTL. As long as we have a good unit test for each case on
the Chromium side and abstract it from the higher-level functions, I
still prefer it this way.

[eseidel, 2013-09-11 15:00:25]

I also meant writing-direction, i.e. vertical text (which may itself be LTR or
RTL in combination with being top to bottom or bottom to top).

The more I think about this, the less I'm comfortable with AX reaching into the
line-box tree.

Most parts of the engine (and certainly the DOM) do not know there even is a
linebox tree, let alone it's layout. :)

The goal here is to cache all the information about all the positions of all the
glyphs on screen so that this can be sent to the browser process and used to
work backwards from into the DOM?

[dmazzoni, 2013-09-11 16:39:08]

Eric and I just had an offline conversation. To summarize, let's have the
renderer module provide a more abstract interface to accessibility code rather
than having accessibility code working with InlineTextBoxes directly.

[dmazzoni, 2013-10-01 07:44:57]

@leviw, @eseidel: Ready for another look.

1. I've abstracted InlineTextBox so that the accessibility module no longer
depends on InlineTextBox directly.
2. There's a test that makes sure references to InlineTextBoxes don't persist
when they're destroyed.
3. The direction of the text is now explicit, and a test now covers
vertically-oriented text, in addition to LTR and RTL.

[igoroliveira, 2013-10-03 03:11:52]

https://codereview.chromium.org/23983002/diff/63001/Source/core/rendering/Inl...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/63001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1503: bool InlineTextBox::isRTL() const
InlineBox::direction() is not enough here?

[dmazzoni, 2013-10-03 03:22:33]

https://codereview.chromium.org/23983002/diff/63001/Source/core/rendering/Inl...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/63001/Source/core/rendering/Inl...
Source/core/rendering/InlineTextBox.cpp:1503: bool InlineTextBox::isRTL() const
On 2013/10/03 03:11:55, igoroliveira wrote:
> InlineBox::direction() is not enough here?

Thanks, that's obviously what I was looking for.

[eseidel, 2013-10-03 20:36:54]

Thank you for taking the time to go over this patch again.

Unfortunately I fear that my concerns of limiting the invasive-ness of AX code
into the Rendering Tree are still not well addressed by this change.

My goals are twofold:
1. To make AX safe.  Make it hard for AX programmers to accidentally crash by
other parts of the code not being aware of how AX may or may not interface with
the rendering tree.
2. Keep rendering unencumbered.  There is a lot of change going on in rendering
right now.  As part of that change we're realizing that we made mistakes years
and years ago by not better abstracting rendering from the rest of the system.

Since this is the first time (to my knowledge) that any system is reaching in
(and holding onto) bits of the linebox tree from outside of rendering my
concerns are doubled here that rendering hackers are going to either break AX
(crashing or functionally) or be unable to make rendering more awesome due to
established hooks from other parts of the system.

Part of that is I suspect that I'm asking you to do something both nebulous and
difficult.  We don't really have very good APIs into Rendering and I'm asking
you to create the first one w/o any good examples to model after.

Part of that is also that I'm not sure that I (and probably you) don't 100%
remember our VC fully from 3 weeks ago and we probably need to sit down and
re-sync (I'm happy to come to MTV to do that).

One solution may be to just wait 6 months on this change.  Rendering will look
very different then and maybe we'll have better API patterns for you to hook
into.

Another solution is for you and I, or you and Levi or Elliot to sit down and go
over this change in person.  esprehn sits right next to you in MTV so maybe you
could grab him in person for a quick walk-through?  Otherwise I'm happy to come
down to MTV this week or next where you and I can work through this in person.

https://codereview.chromium.org/23983002/diff/77001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityInlineTextBox.h (right):

https://codereview.chromium.org/23983002/diff/77001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.h:61:
RenderText::AbstractInlineTextBox* m_inlineTextBox;
So this is still holding a raw pointer into InlineTextBox.  I see that you've
added a callback from ~InlineTextBox(), but this is still rather scary. :)

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
File Source/core/rendering/RenderText.h (right):

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
Source/core/rendering/RenderText.h:146: typedef InlineTextBox
AbstractInlineTextBox;
So this actually isn't going to do anything. :)  The compiler won't stop you
from calling any method you like on these, despite the fact that in the source
they're called AbstractInlineTextBox.  We'd have to actually static_cast these
pointers to ints or something before the compiler would stop you from using
them.

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
Source/core/rendering/RenderText.h:158: AbstractInlineTextBox*
firstInlineTextBox() const;
I might have moved this all out of RenderText into a separate object and make
that object a friend class of RenderText if necessary.

[leviw_travelin_and_unemployed, 2013-10-03 20:42:06]

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
File Source/core/rendering/RenderText.h (right):

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
Source/core/rendering/RenderText.h:158: AbstractInlineTextBox*
firstInlineTextBox() const;
On 2013/10/03 20:36:55, eseidel wrote:
> I might have moved this all out of RenderText into a separate object and make
> that object a friend class of RenderText if necessary.

This description is what I was expecting after the conversation we'd had.

[dmazzoni, 2013-10-04 07:56:09]

OK, here's an approach that's a lot more decoupled. Is this moving in the right
direction?

https://codereview.chromium.org/23983002/diff/77001/Source/core/accessibility...
File Source/core/accessibility/AccessibilityInlineTextBox.h (right):

https://codereview.chromium.org/23983002/diff/77001/Source/core/accessibility...
Source/core/accessibility/AccessibilityInlineTextBox.h:61:
RenderText::AbstractInlineTextBox* m_inlineTextBox;
On 2013/10/03 20:36:55, eseidel wrote:
> So this is still holding a raw pointer into InlineTextBox.  I see that you've
> added a callback from ~InlineTextBox(), but this is still rather scary. :)

Well, we're doing that with RenderObject already. Scary. A WeakPtr would be a
bit better, but wouldn't totally solve the problem, and the cost would be a
WeakPtrFactory for every RenderObject and Node, which seems too expensive. Any
ideas?

What I can do in this case to make it a lot less scary is to make this a RefPtr
to a ref-counted AbstractInlineTextBox. Then we just need an InlineTextBox to
detach its associated AbstractInlineTextBox when it's deleted, which we can do
with a HashMap. That way there's no added cost when AbstractInlineTextBox isn't
used.

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
File Source/core/rendering/RenderText.h (right):

https://codereview.chromium.org/23983002/diff/77001/Source/core/rendering/Ren...
Source/core/rendering/RenderText.h:158: AbstractInlineTextBox*
firstInlineTextBox() const;
On 2013/10/03 20:42:07, Levi wrote:
> On 2013/10/03 20:36:55, eseidel wrote:
> > I might have moved this all out of RenderText into a separate object and
make
> > that object a friend class of RenderText if necessary.
> 
> This description is what I was expecting after the conversation we'd had.

Got it. That way we can really decouple it.

[dmazzoni, 2013-10-07 19:22:37]

Uploaded for real this time.

[eseidel, 2013-10-17 18:43:26]

lgtm.

I recommend getting alice or someone more familiar with the AX-Web bits to sign
off on those.

Thanks for taking the time to make this maintainable for us long term!

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:328: return
AccessibilityInlineTextBox::create(RefPtr<AbstractInlineTextBox>(inlineTextBox));
I don't think the RefPtr here is needed?

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:431: getAXID(newObj.get());
Is this to populate the cache?  I'm surprised to see a get call which isn't
assigned to something?

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:435: newObj->init();
init() must be an AXObject concept?

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:47: m_axObjectCache =
renderText->document().axObjectCache();
Interesting that all of these objects each hold a separate pointer to the
AXObjectCAche.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:52: if (m_axObjectCache
&& m_inlineTextBox.get())
You should be able to just write && m_inlineTextBox and the bool operator will
do the right thing.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityInlineTextBox.h (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.h:37: class
AccessibilityInlineTextBox : public AccessibilityObject {
So these are smart-pointers of sorts?  Like String() holds a RefPtr<StringImpl>,
this is a RefPtr around some other object with some wrapper methods.

But these are held in a global cache?  The lifetime of these objects is not
immediately obvious to me.  Presumably they're notified when the thing they're
holding goes away?  In this case, it's a RefPtr, so we keep it alive, so my
mental model must not quite be right?

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.h:63: virtual bool
computeAccessibilityIsIgnored() const;
OVERRIDE?

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityRenderObject.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityRenderObject.cpp:1835: for
(RefPtr<AbstractInlineTextBox> box = renderText->firstAbstractInlineTextBox();
box.get(); box = box->nextInlineTextBox()) {
This may be alot of mallocs. :)  But this looks reasonable.

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/Ab...
File Source/core/rendering/AbstractInlineTextBox.h (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.h:45:
AbstractInlineTextBoxDirectionLR,
AbstractLeftToRight,
AbstractRightToLeft,
AbstractTopToBottom,
maybe?

Donno. Seems like we're spending a lot of characters on the prefix and not much
on the content. :)  But I'm not wedded to any particular naming scheme.

I'm also OK with this just using existing enums if they fit.  The isolation
layer is more about isolating AX from object liftime troubles than it is from
keeping ABI compatibility, etc.

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.h:88: RenderText* m_renderText;
I might add a comment like:
// These are weak pointers.  ~InlineTextBox() notifies us of its destruction.
// We assume that RenderText can never be destroyed *before* its InlineTextBoxes
// and depend on ~InlineTextBox for clearing the RenderText* too.

The exact wording is up to you, just to make the lifetime assumptions clear.

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:74:
AbstractInlineTextBox::willDestroy(this);
If you have perf trouble, we may just have to make this function inline in
AbstractInlineTextBox.h.

Given that mallocs() normally dominate perf samples, I doubt this function call
will show up.

[dmazzoni, 2013-10-17 20:39:19]

Thanks! Eric, all of your feedback should be addressed.

Any final comments from any other reviewers?

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:328: return
AccessibilityInlineTextBox::create(RefPtr<AbstractInlineTextBox>(inlineTextBox));
On 2013/10/17 18:43:26, eseidel wrote:
> I don't think the RefPtr here is needed?

Done.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:431: getAXID(newObj.get());
On 2013/10/17 18:43:26, eseidel wrote:
> Is this to populate the cache?  I'm surprised to see a get call which isn't
> assigned to something?

This gets a unique ID and sets it on the object. Maybe we should call it
assignNewAXID.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:435: newObj->init();
On 2013/10/17 18:43:26, eseidel wrote:
> init() must be an AXObject concept?

Yes. This needs to run after setting the id. We could refactor this a tiny bit,
but there are four "getOrCreate" functions here that all follow this same
pattern.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:47: m_axObjectCache =
renderText->document().axObjectCache();
On 2013/10/17 18:43:26, eseidel wrote:
> Interesting that all of these objects each hold a separate pointer to the
> AXObjectCAche.

That isn't normally needed. Normally we find it from the object's document - but
we need it here because the object we're referencing might be nulled.

It's safe for this to be a raw ptr because AXObjectCache always calls detach()
on every one of its AXObjects when it's destroyed.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:52: if (m_axObjectCache
&& m_inlineTextBox.get())
On 2013/10/17 18:43:26, eseidel wrote:
> You should be able to just write && m_inlineTextBox and the bool operator will
> do the right thing.

Done.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityInlineTextBox.h (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.h:37: class
AccessibilityInlineTextBox : public AccessibilityObject {
On 2013/10/17 18:43:26, eseidel wrote:
> So these are smart-pointers of sorts?  Like String() holds a
RefPtr<StringImpl>,
> this is a RefPtr around some other object with some wrapper methods.

Some AccessibilityObjects are more like wrappers with weak pointers. In this
particular case a RefPtr was possible. The idea is that we need wrappers for
Nodes, RenderObjects, Frames, and now AbstractInlineTextBoxes, but they all
implement the same interface.

> But these are held in a global cache?  The lifetime of these objects is not
> immediately obvious to me.  Presumably they're notified when the thing they're
> holding goes away?  In this case, it's a RefPtr, so we keep it alive, so my
> mental model must not quite be right?

Most AXObjects have a weak ptr, like to a RenderObject. In that case the
AXObject is nulled immediately when the RenderObject is deleted. We remove our
own references to that AXObject when the parent object notifies us that its
children have changed.

In this case, we have a RefPtr, and the object we're referencing will get nulled
when the InlineTextBox goes away. We'll remove our own ref when the
AccessibilityObject corresponding to the RenderText notices that its children
have changed.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.h:63: virtual bool
computeAccessibilityIsIgnored() const;
On 2013/10/17 18:43:26, eseidel wrote:
> OVERRIDE?

Done.

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityRenderObject.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityRenderObject.cpp:1835: for
(RefPtr<AbstractInlineTextBox> box = renderText->firstAbstractInlineTextBox();
box.get(); box = box->nextInlineTextBox()) {
On 2013/10/17 18:43:26, eseidel wrote:
> This may be alot of mallocs. :)  But this looks reasonable.

As long as it's constant in terms of the total number of (Nodes + RenderObjects
+ InlineTextBoxes) I'm happy.

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/Ab...
File Source/core/rendering/AbstractInlineTextBox.h (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.h:45:
AbstractInlineTextBoxDirectionLR,
On 2013/10/17 18:43:26, eseidel wrote:
> AbstractLeftToRight,
> AbstractRightToLeft,
> AbstractTopToBottom,
> maybe?
> 
> Donno. Seems like we're spending a lot of characters on the prefix and not
much
> on the content. :)  But I'm not wedded to any particular naming scheme.

Spelling out LeftToRight, etc. sounds good but "Abstract" as the prefix seems
unclear. How about I move it inside the class? That will make it a lot cleaner.

> I'm also OK with this just using existing enums if they fit.  The isolation
> layer is more about isolating AX from object liftime troubles than it is from
> keeping ABI compatibility, etc.

I can't find an existing enum that expresses this semantically - normally LTR
and RTL are a different enum than TB vs BT.

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.h:88: RenderText* m_renderText;
On 2013/10/17 18:43:26, eseidel wrote:
> I might add a comment like:
> // These are weak pointers.  ~InlineTextBox() notifies us of its destruction.
> // We assume that RenderText can never be destroyed *before* its
InlineTextBoxes
> // and depend on ~InlineTextBox for clearing the RenderText* too.
> 
> The exact wording is up to you, just to make the lifetime assumptions clear.

Done.

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/104001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:74:
AbstractInlineTextBox::willDestroy(this);
On 2013/10/17 18:43:26, eseidel wrote:
> If you have perf trouble, we may just have to make this function inline in
> AbstractInlineTextBox.h.
> 
> Given that mallocs() normally dominate perf samples, I doubt this function
call
> will show up.

Hopefully it won't show up.

[aboxhall, 2013-10-17 22:27:53]

lgtm

Basically looks good with some nits.

https://codereview.chromium.org/23983002/diff/112001/LayoutTests/accessibilit...
File LayoutTests/accessibility/inline-text-word-boundaries.html (right):

https://codereview.chromium.org/23983002/diff/112001/LayoutTests/accessibilit...
LayoutTests/accessibility/inline-text-word-boundaries.html:10: Ace ten
twenty-one thirty-five. xxxxxxxxxxxxxxxxxxx
Can we comment that the xxxxx... is intended to wrap somehow?

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:102: offsets[i] =
LayoutUnit::fromFloatRound(widthSoFar);
So offsets[i] is the position of the end of character i?

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityRenderObject.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityRenderObject.cpp:1749:
AccessibilityNodeObject::textChanged();
Why does this come last?

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
File Source/core/rendering/AbstractInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:40: HashMap<InlineTextBox*,
RefPtr<AbstractInlineTextBox> >*
AbstractInlineTextBox::gAbstractInlineTextBoxMap = 0;
What's the g for, out of curiosity?

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:42:
PassRefPtr<AbstractInlineTextBox> AbstractInlineTextBox::getOrCreate(RenderText*
renderText, InlineTextBox* inlineTextBox)
Do we comment static methods with // static ?

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:50: HashMap<InlineTextBox*,
RefPtr<AbstractInlineTextBox> >::const_iterator it =
gAbstractInlineTextBoxMap->find(inlineTextBox);
Would it make sense to typedef this iterator type (or the hashmap type even)?

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:159: return
text().substring(start, len);
What are these two cases (node and !node) for? Does text() not work if the
RenderObject is not anonymous?

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:1496: it.advance(i + 1, &glyphBuffer);
Could you do the advance() in the for statement, i.e.
for (unsigned i = 0; i < m_len && it.advance(i + 1, &glyphbuffer); i++) ?
Or is that less readable?

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
File Source/testing/runner/AccessibilityUIElementChromium.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:355: WebAXObject child
= object.childAt(i);
s/child/textBox/g ?

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:368: if (localIndex) {
I know it's not WebKit/Blink style, but I think explicitly testing localIndex >
0 here would be more comprehensible. It could technically be < 0 (not in
practice, but technically) after all.

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:412: void
getWordBoundaries(const WebAXObject& object, int characterIndex, int& wordStart,
int& wordEnd)
This method name confuses me a bit - it's similar to
AbstractInlineTextBox::wordBoundaries() but does something quite different.
Maybe getBoundariesForWord or similar?

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:416: WebAXObject child
= object.childAt(i);
similarly, s/child/textBox/g ?

[dmazzoni, 2013-10-19 06:48:06]

https://codereview.chromium.org/23983002/diff/112001/LayoutTests/accessibilit...
File LayoutTests/accessibility/inline-text-word-boundaries.html (right):

https://codereview.chromium.org/23983002/diff/112001/LayoutTests/accessibilit...
LayoutTests/accessibility/inline-text-word-boundaries.html:10: Ace ten
twenty-one thirty-five. xxxxxxxxxxxxxxxxxxx
On 2013/10/17 22:27:55, aboxhall wrote:
> Can we comment that the xxxxx... is intended to wrap somehow?

Done.

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityInlineTextBox.cpp:102: offsets[i] =
LayoutUnit::fromFloatRound(widthSoFar);
On 2013/10/17 22:27:55, aboxhall wrote:
> So offsets[i] is the position of the end of character i?

Yes. It's arbitrary, it could be the beginning, I guess.

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
File Source/core/accessibility/AccessibilityRenderObject.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/accessibilit...
Source/core/accessibility/AccessibilityRenderObject.cpp:1749:
AccessibilityNodeObject::textChanged();
On 2013/10/17 22:27:55, aboxhall wrote:
> Why does this come last?

AccessibilityNodeObject::textChanged handles live region announcements. We
should update the tree before sending live region events.

Added a comment.

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
File Source/core/rendering/AbstractInlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:40: HashMap<InlineTextBox*,
RefPtr<AbstractInlineTextBox> >*
AbstractInlineTextBox::gAbstractInlineTextBoxMap = 0;
On 2013/10/17 22:27:55, aboxhall wrote:
> What's the g for, out of curiosity?

Global.

That seems to be the standard Blink style for both globals and static class
members.

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:42:
PassRefPtr<AbstractInlineTextBox> AbstractInlineTextBox::getOrCreate(RenderText*
renderText, InlineTextBox* inlineTextBox)
On 2013/10/17 22:27:55, aboxhall wrote:
> Do we comment static methods with // static ?

I don't think so, I haven't seen it in Blink code.

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:50: HashMap<InlineTextBox*,
RefPtr<AbstractInlineTextBox> >::const_iterator it =
gAbstractInlineTextBoxMap->find(inlineTextBox);
On 2013/10/17 22:27:55, aboxhall wrote:
> Would it make sense to typedef this iterator type (or the hashmap type even)?

Sure, that helps readability a bit. Done.

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/Ab...
Source/core/rendering/AbstractInlineTextBox.cpp:159: return
text().substring(start, len);
On 2013/10/17 22:27:55, aboxhall wrote:
> What are these two cases (node and !node) for? Does text() not work if the
> RenderObject is not anonymous?

It's for generated text, like list item numbers or bullets, or the css :before
and :after, for example.

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:1496: it.advance(i + 1, &glyphBuffer);
On 2013/10/17 22:27:55, aboxhall wrote:
> Could you do the advance() in the for statement, i.e.
> for (unsigned i = 0; i < m_len && it.advance(i + 1, &glyphbuffer); i++) ?
> Or is that less readable?

I find it less readable because the iterator advance doesn't control the loop at
all - it doesn't return success/failure, etc.

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
File Source/testing/runner/AccessibilityUIElementChromium.cpp (right):

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:355: WebAXObject child
= object.childAt(i);
On 2013/10/17 22:27:55, aboxhall wrote:
> s/child/textBox/g ?

Done.

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:368: if (localIndex) {
On 2013/10/17 22:27:55, aboxhall wrote:
> I know it's not WebKit/Blink style, but I think explicitly testing localIndex
>
> 0 here would be more comprehensible. It could technically be < 0 (not in
> practice, but technically) after all.

I agree with you, but the style checker actually doesn't allow this! :(

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:412: void
getWordBoundaries(const WebAXObject& object, int characterIndex, int& wordStart,
int& wordEnd)
On 2013/10/17 22:27:55, aboxhall wrote:
> This method name confuses me a bit - it's similar to
> AbstractInlineTextBox::wordBoundaries() but does something quite different.
> Maybe getBoundariesForWord or similar?

Done.

https://codereview.chromium.org/23983002/diff/112001/Source/testing/runner/Ac...
Source/testing/runner/AccessibilityUIElementChromium.cpp:416: WebAXObject child
= object.childAt(i);
On 2013/10/17 22:27:55, aboxhall wrote:
> similarly, s/child/textBox/g ?

Done.

[commit-bot: I haz the power, 2013-10-19 21:04:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23983002/153001

[commit-bot: I haz the power, 2013-10-19 22:55:44]

Change committed as 160020