We cache text in bug 1730093. We cache screen bounds for Accessibles in bug 1726227. We need to cache screen bounds for text too.

We probably want to cache the coordinates for each character relative to the bounds of the Accessible. It'd be nice if we could cache coordinates rather than rects to save memory. It'd be nicer still if we could cache just the changing coordinate (the x coord for LTR/RTL text). However, the latter is complicated by the fact that a text leaf can contain multiple lines of text. Perhaps we can cache the line coordinate (the y coord for RTL/LTR text) for each line start, though that does involve some indirection: you have to find the line start for a character to get its line coordinate.

(In reply to James Teh [:Jamie] from comment #0)

We cache text in bug 1730093. We cache screen bounds for Accessibles in bug 1726227. We need to cache screen bounds for text too.

It'd be nice if we could cache coordinates rather than rects to save memory.

Do we not use the size of the rects from text bounds?

We discussed on Matrix, but to summarise here, we do care about the width/height, but we should be able to calculate that using the adjacent characters by assuming that the character rects are also adjacent. If char 0's x is 0 and char 1's x is 5, then char 0's width is 5.

Some other notes from our Matrix discussion:

We should cache these coordinates on text leaf Accessibles, not HyperText Accessibles. TextLeafPoint (which is the new text implementation and is used for all the caching stuff) operates with leaf nodes, not HyperTexts. The reason is that this way, we don't have to worry about embedded objects in the middle of a node, nor do we have to worry about style changing in the middle of a node. A text leaf can only contain one string of text with one set of styling attributes. It also means there's a lot less conversion between different offset spaces and thus a lot less room for error and/or loss of precision.

It possibly makes sense to start with explicitly storing both x and y coords until we're certain the calculation is correct. However, to provide some (theoretical) explanation of that possible optimisation:

1. We assume the y coordinate in a text leaf only changes when a new line begins. That is, each line of characters has one consistent y coordinate.
2. Since we already have the line starts cached, we can thus optimise by only storing each y coordinate once.
3. In the content process, we can do that either by iterating the lines (which we already do) and storing the y coordinate for each line; or iterating all the characters (regardless of line) and storing the y coordinate only when it changes. These should yield the same result, assuming that the assumption in 1) is correct.
4. One caveat is that if an Accessible begins in the middle of a line, the line starts array does not contain the first line (0). The idea is that you have to walk to the previous Accessible (or maybe walk back several Accessibles!) to find the start of the line. That doesn't fit well for char bounds.
   • Rather than doing this backwards walk, I would suggest that we would not align the y coords and line starts arrays. That is, even if line starts doesn't contain 0, the y coords array would still store the info for the first line; i.e. y coords would have one element more than line starts in this case.
5. Another caveat is vertical (top-to-bottom or bottom-to-top) text. We'd need to flip handling of the x and y coords in that case, as well as storing the vertical state.

There's also the question of how we store the coords in the arrays. For the x array, Do we store the left or right? If we store the left, the first element will always be 0, and for a character c, we'd use c + 1 to determine the width of c, with the width of the Accessible being the width of the last character. If we store the right, the last element would always be the width of the Accessible, and for a character c, we'd use c - 1 to determine the left of c, with 0 being the left of the first character. As you proposed on Matrix, I thought the latter would be easier, but I'm not really sure it makes much of a difference, since we have the width of the Accessible cached anyway.

As to what data structure to use, I'd probably suggest two simple arrays. This will work regardless of whether you do y-per-line or y-per-char. If you're doing point-per-char, I guess you could have an array of points instead.

(In reply to James Teh [:Jamie] from comment #2)

We discussed on Matrix, but to summarise here, we do care about the width/height, but we should be able to calculate that using the adjacent characters by assuming that the character rects are also adjacent. If char 0's x is 0 and char 1's x is 5, then char 0's width is 5.

How do we get the height? From the text acc's primary frame?

:Jamie I noticed we have a aCoordType argument here -- do different clients need text bounds in different coordinate systems, or is that something we use internally?

The granularity there seems to be parent-relative, screen-relative, or window-relative, and I'm a little concerned about how to deal with those different vals here (namely the last one, parent-relative should be straightforward, and screen-relative is doable with some modifications to ::Bounds())

When I was writing ::Bounds(), constructing screen coordinates meant walking up to the top-level doc boundary and adding the sum of our ancestor offsets to whatever value we got back from LocalAccessible::Bounds (ultimately involving a call to nsIFrame::GetScreenRectInAppUnits). With my working implementation of ::TextBounds() we do something similar: locate the text relative to the hypertext container, and then add that relative offset to the ::Bounds() of the containing acc. We've talked a little bit about how that probably won't work with transforms, but I'm having a hard time seeing how to modify that approach for window coordinates. It seems like the easiest way to go forward would be to get the offset between the window and the screen and subtract that from the screen coordinates I've already computed. But how do I get that information in the parent process?

As an aside, I think I can modify our ::Bounds logic to work across transforms by moving the internals into a helper function that looks something like:

LayoutDeviceIntRect BoundsWithRelativeOffset(nscoord aOffset) {
  // retrieve relative bounds for _this_ from the cache
  bounds.MoveBy(aOffset.x, aOffset.y);
  // do the normal bounds stuff, now that we've applied aOffset in the coordinate space it originates from
}

Our actual ::Bounds() function could then shift to looking something like:
return BoundsWithRelativeOffset(0);

Also, apart from accessible/tests/browser/bounds/browser_test_zoom_text.js I'm having a hard time finding tests that explicitly test text bounds. Some of our tests like this one seem to do a regular .getBounds call instead of .getRangeExtents for checking text bounds. Is that intentional/correct?

(In reply to Morgan Reschenberg [:morgan] from comment #5)

How do we get the height? From the text acc's primary frame?

Similar to x, it depends on whether we start y from top or bottom. If it's top (so first y is 0), then the height for all characters on a given line is the next larger y coordinate. If it's bottom, we have to calculate the top based on the previous smaller y coordinate. This makes me realise this is another reason to do y coordinates by line. Otherwise, we have to walk through lots of repeated y coords to find the previous smaller or next larger.

If y is based on top, then the height for the last line of characters will have to be calculated using the height of the text acc as you suggest.

This would all probably be simpler if we rendered continuations as separate nodes in the a11y tree (probably children of text nodes) so that a single node could never cross line boundaries. (This is what Chromium does.) That would also simplify line offset caching; we'd only need to cache whether a given node begins a line or not. However, that seems like a pretty massive change to the core. Still, it might be worth looking into if this is getting too complicated.

(In reply to Morgan Reschenberg [:morgan] from comment #6)

:Jamie I noticed we have a aCoordType argument here -- do different clients need text bounds in different coordinate systems, or is that something we use internally?

Ah. ATK and IA2 both support retrieving coordinates in different coordinate spaces. :( I'm not sure if any client actually uses that. NVDA doesn't, but Orca or JAWS might. I had a really quick look at the Orca code and couldn't pull up a clear answer yet.

The granularity there seems to be parent-relative, screen-relative, or window-relative, and I'm a little concerned about how to deal with those different vals here (namely the last one, parent-relative should be straightforward,

IA2 supports screen and parent. ATK has window, though.

and screen-relative is doable with some modifications to ::Bounds())

Screen is probably going to be most common.

I'm having a hard time seeing how to modify that approach for window coordinates. It seems like the easiest way to go forward would be to get the offset between the window and the screen and subtract that from the screen coordinates I've already computed.

I agree. It's not entirely clear to me what "window" really means/does. We have nsCoreUtils::GetScreenCoordsForWindow and this is what the ATK code seems to use. GetScreenCoordsForWindow gets the window from the TreeOwner for the document and uses that. The problem there is that for remote documents, we would use the browser element hosting the document, which means the TreeOwner would be the owner of the chrome, not the content document.

Is the "window" for the browser element and the window for the content document the same? I'm not sure, but I'm guessing it is these days since content documents are not hosted in their own widgets any more. (But is a widget the same as a window? Unsure.) The ATK code I linked earlier assumes the windows are equivalent, but there are other ATK code paths which punt this calculation to the content process. This inconsistency is confusing, but I've learned not to assume things like this in our code are necessarily correct.

But how do I get that information in the parent process?

My suggestion at this point is to assume that we can safely use nsCoreUtils::GetScreenCoordsForWindow on the browser element hosting remote documents and just add/subtract those coords as needed. If we need to revise that assumption later, we'll cross that bridge when we get there, especially given there is already other code assuming this.

As for how to get the browser element, I'm looking at that first chunk of ATK code and realising it's probably broken for OOP iframes! A BrowserParent for an OOP iframe doesn't have an owner element. You have to walk to the top level BrowserParent to get that. You've done something similar here, except you obviously don't want the OwnerDoc or PresContext, just the OwnerElement.

As an aside, I think I can modify our ::Bounds logic to work across transforms by moving the internals into a helper function that looks something like:

Oh nice! I was wondering whether we could avoid repeated code there. Very clean.

(In reply to Morgan Reschenberg [:morgan] from comment #7)

Also, apart from accessible/tests/browser/bounds/browser_test_zoom_text.js I'm having a hard time finding tests that explicitly test text bounds. Some of our tests like this one seem to do a regular .getBounds call instead of .getRangeExtents for checking text bounds. Is that intentional/correct?

That second test does do a regular getBounds, but only so it can calculate a comparison coordinate (the centre I think). The actual assertion happens in testOffsetAtPoint, which (calls nsIAccessibleText::getOffsetAtPoint)[https://searchfox.org/mozilla-central/rev/990afe241fd6439817555f586c07ac1feaf79538/accessible/tests/mochitest/layout.js#70]. That one is hit testing, so you probably can't/don't want to deal with that yet, but testTextPos calls getCharacterExtents and testTextPos is used in this test.

Depends on D142970

https://hg.mozilla.org/mozilla-central/rev/341232b1fa96

Not quite fixed yet. :)

Backed out for causing mochitest failures on browser_caching_text_bounds.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | accessible/tests/browser/e10s/browser_caching_text_bounds.js | Exception thrown - at chrome://mochikit/content/browser-test.js:1193 - Error: This test didn't call add_task, nor did it define a generatorTest() function, nor did it define a test() function, so we don't know how to run it.

clean try: https://treeherder.mozilla.org/jobs?repo=try&revision=2660d430b130b68cbf17203e4b9e34db1d462f0c&selectedTaskRun=FgTjnHdLSken8tD81UH2SQ.0

https://hg.mozilla.org/mozilla-central/rev/3c9e5037005c