The accessibility engine currently uses PeekOffset to determine line boundaries. Particularly as we work to cache the entire a11y tree in the parent process, this is no longer suitable for various reasons; e.g. we can't restrict the query to a single Accessible, PeekOffset has a lot of quirks which impact a11y but don't really impact other consumers, etc. We'd like to instead do most of the boundary calculation ourselves, but we still need to know where layout split content across lines.

I have verified that nsIFrame::IsLogicallyAtLineEdge serves our purposes nicely. However, it is currently private. It also depends on nsIFrame::GetLineNumber, which is also private.

I discussed this on matrix and mats noted:

IIRC, one reason we didn't want to expose them is that figuring out the line number for a specific frame is pretty slow - our internal representation doesn't have that info so we have to iterate and count them. Jamie, it would be good if you could specify your use case with some detail in the bug, to make sure we take that into account if we decide to redesign this at some point.

For now, I'm going to submit a patch to simply make these two methods public. However, if there's a better (more performant?) way to determine the line edge without using line iterators, that's great. Another option might be going with this in the interim and coming up with a better solution later, which would also be fine.

IsLogicallyAtLineEdge and GetLineNumber are needed by accessibility to calculate line edges for text navigation.
Accessibility doesn't need IsVisuallyAtLineEdge, but it seemed odd to expose one without the other.

Ug. IsLogicallyAtLineEdge doesn't work for us in cases like this:
data:text/html,a<br><span></span>b
The empty span is stripped from the a11y tree, but not the frame tree. Since the empty span begins the line, IsLogicallyAtLineEdge returns false for the "b" text frame.

We have a similar issue with this:
data:text/html,<span>a<br>%0a</span>b
The white space (%0a) here seems to impact the frame tree somehow. Interestingly, this one breaks PeekOffset as well.

Anyway, it looks like we're going to have to compare two frames (this one and the frame for the previous leaf Accessible) and check whether they're on the same line. Currently, I'm doing this by calling GetLineNumber on both. Is there a better (more performant) way?

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

...and check whether they're on the same line. Currently, I'm doing this by calling GetLineNumber on both. Is there a better (more performant) way?

I think a somewhat more performant approach, at least, would be to first find the containing-block ancestor of each frame; if they're different, then you're done already. If the containing block is the same, then get a LineIterator from it, and use that to find the line for both frames, instead of two separate GetLineNumber calls which each independently have to get and then discard a LineIterator.

(In reply to Jonathan Kew (:jfkthame) from comment #3)

I think a somewhat more performant approach, at least, would be to first find the containing-block ancestor of each frame; if they're different, then you're done already.

That's fair. However, GetLineNumber has a bunch of code to get the containing block; it's not as simple as "get the parent and check if it's block", but must also deal with placeholder frames, etc. (I confess I don't understand any of that.) Is there a simple public way for me to get the containing block or do I need to duplicate this code in a11y?

I think my inclination would be to factor out that functionality from GetLineNumber into a (public) helper like GetContainingBlockForLine, which would return the nsBlockFrame or nsTableRowGroupFrame ancestor (if any), with the appropriate handling for placeholders etc.

This method would not actually get the nsILineIterator, as that's potentially expensive for an nsBlockFrame with lots of lines, and might not be needed. Instead, we could have a new nsIFrame method bool CanProvideLineIterator() that just returns false by default; the classes (only nsBlockFrame and nsTableRowGroupFrame, afaics) that override GetLineIterator would also override this to return true, so that GetContainingBlockForLine can cheaply tell when it has reached the right ancestor.

Then GetLineNumber would use this helper to find the ancestor frame and get the nsILineIterator it wants; and you could use it from a11y to do the comparison above in the somewhat more efficient way, without needing to get two individual nsILineIterators.

Does that sound workable?

That made sense. However, I hit a snag trying to implement it.

if I'm reading the code right, GetLineNumber doesn't just pass this to nsILineIterator::GetContainingBlockForLine. Rather, it passes a frame which is a direct child of the containing block. That means the proposed GetContainingBlockForLine helper needs to return two frames: the containing block and a direct child. That does make this a bit less clean than originally thought. I'll submit a patch with that and see where we land.

The other concern is that nsBlockFrame::GetLineIterator can return null in some cases. I'm not sure whether that ever happens in reality, but there are code paths for it. In those cases, CanProvideLineIterator will still return true, leaving us with a discrepancy.

Previously, to check whether two frames were on the same line, you had to call nsIFrame::GetLineNumber on both of them.
If the lines were in a different containing block, this wastefully created line iterators.
With this new method, you can get the containing blocks for both frames and only create a single line iterator if the blocks are the same.
To achieve this, nsIFrame::CanProvideLineIterator was also added and is used by GetContainingBlockForLine.
Accessibility will use this new functionality in its new text boundary implementation.

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

That made sense. However, I hit a snag trying to implement it.

if I'm reading the code right, GetLineNumber doesn't just pass this to nsILineIterator::GetContainingBlockForLine. Rather, it passes a frame which is a direct child of the containing block. That means the proposed GetContainingBlockForLine helper needs to return two frames: the containing block and a direct child. That does make this a bit less clean than originally thought. I'll submit a patch with that and see where we land.

Right - on looking a bit more closely, I noticed this too. It turns out, though, that we have a couple other call-sites that can also convert to use this, because the way they currently use GetLineNumber is wasteful; I'll file a followup to handle that. So I think this worth doing for the benefit of several cases.

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

The other concern is that nsBlockFrame::GetLineIterator can return null in some cases. I'm not sure whether that ever happens in reality, but there are code paths for it. In those cases, CanProvideLineIterator will still return true, leaving us with a discrepancy.

The error checks there are obsolete, since we converted to infallible malloc/new allocators; I propose to remove them and make it clear that it's infallible.

See bug 1732268 for removal of the (unreachable) nsLineIterator failure codepaths.

https://hg.mozilla.org/mozilla-central/rev/d28f451789d7