Bugzilla

(In reply to James Teh [:Jamie] from comment #4)
> (In reply to James Teh [:Jamie] from comment #0)
> > My current thinking is that whenever line boundaries change, the bounds should change too.
> 
> Ug. That doesn't always hold. Consider this:
> 
> `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>ac</span>def`
> 
> That gives you this:
> 
> ```
> ac
> de
> f
> ```
> 
> The second text leaf (starting at d) spans two lines. Even though there is only a single character f on the last line, the rect has to cover e, so the rect is the width of two characters.

When you talk about "line boundaries" I assume you mean both the info we're caching in lineStarts and the corresponding "lineEnd", right?
So, for your example above, something like `[[0,2], [2, 4], [4, 5]]`?

> 
> Now, if you insert b between a and c, you get this:
> 
> ```
> ab
> cd
> ef
> ```
> 
> The second text leaf reflows. It only includes one character on the second line now (d), but because it includes e, the rect's left coordinate is the first character. Because it includes f, the width is two characters. Unfortunately, that means the rect doesn't change... and the text hasn't changed either. :(

So here, that lineBoundaries array would change from `[..., [4, 5]]` to `[... [4, 6]]` right? And your point is the bounds for the final text leaf wouldn't change, so we can't rely on that as an indicator to update our lineBoundaries array? Just wanna make sure I have this right 😀

> I don't know what to do about this. Sending cached lines every time we reflow seems bad for the same reason that sending bounds every time we reflow is bad. (That's why we cache bounds in LocalAccessible.) But caching lines in LocalAccessible as well seems really horrible.
> 
> I guess we could get the points for the first and last characters of the leaf and compare those. That's perhaps better than caching all the line starts, but it still means caching additional stuff in LocalAccessible.

Yeah this is immediately where my mind went. That, or caching the "empty" space within the bounds and checking if that changes? That's still (potentially) quite a lot of bookkeeping, and something we don't currently have logic for. 

> Any thoughts, Morgan? I'm not asking you to come up with a solution here, more just to check my thinking in case I've missed something or if anything comes to mind without digging too much. Can we do polygons? :)

You joke ;) but yeah, technically we could -- [we've got a class for that](https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/gfx/2d/Polygon.h#155). That class also has a nice `BoundingBox()` function, meaning if we decide to use Polygons internally, we can still easily send clients the rects they expect. Looks like for things that are actually rects (ie. text bounds that aren't multiline, and individual char bounds) we can just call `Polygon::FromRect`. Right now, we rely on the underlying `nsTextFrame` of a TextLeafAccessible to create its (cached) parent-relative bounds. If we're moving towards polygons, we'd want to override `LocalAccessible::ParentRelativeBounds` in `TextLeafAccessible` and do something similar to what I'm doing in the [`while/for` loop in `HyperTextAccessibleBase::TextBounds` ](https://phabricator.services.mozilla.com/D143527?id=571654). Instead of creating a line rect, and then unioning that into `result`, we'd create a line rect, turn it into a polygon with `FromRect()`, and then (create and) call a union function. There's probably a way to avoid duplicating too much of the code, like I mentioned in the patch, that `TextBounds` loop should ultimately live in TextLeafRange, probably. 

Famous last words but, seems doable 😀

(In reply to James Teh [:Jamie] from comment #4)
> (In reply to James Teh [:Jamie] from comment #0)
> > My current thinking is that whenever line boundaries change, the bounds should change too.
> 
> Ug. That doesn't always hold. Consider this:
> 
> `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>ac</span>def`
> 
> That gives you this:
> 
> ```
> ac
> de
> f
> ```
> 
> The second text leaf (starting at d) spans two lines. Even though there is only a single character f on the last line, the rect has to cover e, so the rect is the width of two characters.

So lineStarts would look like: `[0]` for the first leaf and `[0, 2]` for the second leaf

> 
> Now, if you insert b between a and c, you get this:
> 
> ```
> ab
> cd
> ef
> ```
> 
> The second text leaf reflows. It only includes one character on the second line now (d), but because it includes e, the rect's left coordinate is the first character. Because it includes f, the width is two characters. Unfortunately, that means the rect doesn't change... and the text hasn't changed either. :(

... And now lineStarts is `[0, 2]` and `[1]` for the leaves respectively.

So your point is the bounds for the final text leaf wouldn't change, so we can't rely on that as an indicator to update our lineStarts array? Just wanna make sure I have this right 😀 I wonder if you could use text length to help with this. It seems like lineStarts would only be affected (a) in the leaf an insertion or deletion occurred in and (b) any leaves after that in the reading order that don't force a line break themselves. Is that fair to say?

If you have something like `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>abc</span><br>def` for example, inserting "b" in the same fashion doesn't change the lineStarts of the last leaf because the <br> means it isn't dependent on the leaf before.

It isn't super efficient, but if you're monitoring text-length changes, you at least have a narrowed range of leaves that may be affected. And, if we keep track of where "hard" linebreaks occur, that could narrow the range further. 

> I don't know what to do about this. Sending cached lines every time we reflow seems bad for the same reason that sending bounds every time we reflow is bad. (That's why we cache bounds in LocalAccessible.) But caching lines in LocalAccessible as well seems really horrible.
> 
> I guess we could get the points for the first and last characters of the leaf and compare those. That's perhaps better than caching all the line starts, but it still means caching additional stuff in LocalAccessible.

Yeah this is immediately where my mind went. That, or caching the "empty" space within the bounds and checking if that changes? That's still (potentially) quite a lot of bookkeeping, and something we don't currently have logic for. 

> Any thoughts, Morgan? I'm not asking you to come up with a solution here, more just to check my thinking in case I've missed something or if anything comes to mind without digging too much. Can we do polygons? :)

You joke ;) but yeah, technically we could -- [we've got a class for that](https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/gfx/2d/Polygon.h#155). That class also has a nice `BoundingBox()` function, meaning if we decide to use Polygons internally, we can still easily send clients the rects they expect. Looks like for things that are actually rects (ie. text bounds that aren't multiline, and individual char bounds) we can just call `Polygon::FromRect`. Right now, we rely on the underlying `nsTextFrame` of a TextLeafAccessible to create its (cached) parent-relative bounds. If we're moving towards polygons, we'd want to override `LocalAccessible::ParentRelativeBounds` in `TextLeafAccessible` and do something similar to what I'm doing in the [`while/for` loop in `HyperTextAccessibleBase::TextBounds` ](https://phabricator.services.mozilla.com/D143527?id=571654). Instead of creating a line rect, and then unioning that into `result`, we'd create a line rect, turn it into a polygon with `FromRect()`, and then (create and) call a union function. There's probably a way to avoid duplicating too much of the code, like I mentioned in the patch, that `TextBounds` loop should ultimately live in TextLeafRange, probably. 

Famous last words but, seems doable if that's the route that makes the most sense 😀

(In reply to James Teh [:Jamie] from comment #4)
> (In reply to James Teh [:Jamie] from comment #0)
> > My current thinking is that whenever line boundaries change, the bounds should change too.
> 
> Ug. That doesn't always hold. Consider this:
> 
> `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>ac</span>def`
> 
> That gives you this:
> 
> ```
> ac
> de
> f
> ```
> 
> The second text leaf (starting at d) spans two lines. Even though there is only a single character f on the last line, the rect has to cover e, so the rect is the width of two characters.

So lineStarts would look like: `[0]` for the first leaf and `[0, 2]` for the second leaf

> 
> Now, if you insert b between a and c, you get this:
> 
> ```
> ab
> cd
> ef
> ```
> 
> The second text leaf reflows. It only includes one character on the second line now (d), but because it includes e, the rect's left coordinate is the first character. Because it includes f, the width is two characters. Unfortunately, that means the rect doesn't change... and the text hasn't changed either. :(

... And now lineStarts is `[0, 2]` and `[1]` for the leaves respectively.

So your point is the bounds (er, bounding rect, what we get from `Bounds()`) for the final text leaf wouldn't change, so we can't rely on that as an indicator to update our lineStarts array? Just wanna make sure I have this right 😀 I wonder if you could use text length to help with this. It seems like lineStarts would only be affected (a) in the leaf an insertion or deletion occurred in and (b) any leaves after that in the reading order that don't force a line break themselves. Is that fair to say?

If you have something like `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>abc</span><br>def` for example, inserting "b" in the same fashion doesn't change the lineStarts of the last leaf because the <br> means it isn't dependent on the leaf before.

It isn't super efficient, but if you're monitoring text-length changes, you at least have a narrowed range of leaves that may be affected. And, if we keep track of where "hard" linebreaks occur, that could narrow the range further. 

> I don't know what to do about this. Sending cached lines every time we reflow seems bad for the same reason that sending bounds every time we reflow is bad. (That's why we cache bounds in LocalAccessible.) But caching lines in LocalAccessible as well seems really horrible.
> 
> I guess we could get the points for the first and last characters of the leaf and compare those. That's perhaps better than caching all the line starts, but it still means caching additional stuff in LocalAccessible.

Yeah this is immediately where my mind went. That, or caching the "empty" space within the bounds and checking if that changes? That's still (potentially) quite a lot of bookkeeping, and something we don't currently have logic for. 

> Any thoughts, Morgan? I'm not asking you to come up with a solution here, more just to check my thinking in case I've missed something or if anything comes to mind without digging too much. Can we do polygons? :)

You joke ;) but yeah, technically we could -- [we've got a class for that](https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/gfx/2d/Polygon.h#155). That class also has a nice `BoundingBox()` function, meaning if we decide to use Polygons internally, we can still easily send clients the rects they expect. Looks like for things that are actually rects (ie. text bounds that aren't multiline, and individual char bounds) we can just call `Polygon::FromRect`. Right now, we rely on the underlying `nsTextFrame` of a TextLeafAccessible to create its (cached) parent-relative bounds. If we're moving towards polygons, we'd want to override `LocalAccessible::ParentRelativeBounds` in `TextLeafAccessible` and do something similar to what I'm doing in the [`while/for` loop in `HyperTextAccessibleBase::TextBounds` ](https://phabricator.services.mozilla.com/D143527?id=571654). Instead of creating a line rect, and then unioning that into `result`, we'd create a line rect, turn it into a polygon with `FromRect()`, and then (create and) call a union function. There's probably a way to avoid duplicating too much of the code, like I mentioned in the patch, that `TextBounds` loop should ultimately live in TextLeafRange, probably. 

Famous last words but, seems doable if that's the route that makes the most sense 😀

(In reply to James Teh [:Jamie] from comment #4)
> (In reply to James Teh [:Jamie] from comment #0)
> > My current thinking is that whenever line boundaries change, the bounds should change too.
> 
> Ug. That doesn't always hold. Consider this:
> 
> `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>ac</span>def`
> 
> That gives you this:
> 
> ```
> ac
> de
> f
> ```
> 
> The second text leaf (starting at d) spans two lines. Even though there is only a single character f on the last line, the rect has to cover e, so the rect is the width of two characters.

So lineStarts would look like: `[0]` for the first leaf and `[0, 2]` for the second leaf

> 
> Now, if you insert b between a and c, you get this:
> 
> ```
> ab
> cd
> ef
> ```
> 
> The second text leaf reflows. It only includes one character on the second line now (d), but because it includes e, the rect's left coordinate is the first character. Because it includes f, the width is two characters. Unfortunately, that means the rect doesn't change... and the text hasn't changed either. :(

... And now lineStarts is `[0, 2]` and `[1]` for the leaves respectively.

So your point is the bounds (er, bounding rect, what we get from `Bounds()`) for the final text leaf wouldn't change, so we can't rely on that as an indicator to update our lineStarts array? Just wanna make sure I have this right 😀 I wonder if you could use text length to help with this. It seems like lineStarts would only be affected (a) in the leaf an insertion or deletion occurred in and (b) any leaves after that in the reading order that don't force a line break themselves. Is that fair to say?

If you have something like `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>abc</span><br>def` for example, inserting "b" in the same fashion doesn't change the lineStarts of the last leaf because the <br> means it isn't dependent on the leaf before.

It isn't super efficient, but if you're monitoring text-length changes, you at least have a narrowed range of leaves that may be affected. And, if we keep track of where "hard" linebreaks occur, that could narrow the range further. We'd have to do a traversal. This would also mean changes to acc A queue cache updates for accs B, C, D ... instead of our existing paradigm where changes to acc A queue updates for acc A only. Maybe we've already diverged from that though I dunno. 

> I don't know what to do about this. Sending cached lines every time we reflow seems bad for the same reason that sending bounds every time we reflow is bad. (That's why we cache bounds in LocalAccessible.) But caching lines in LocalAccessible as well seems really horrible.
> 
> I guess we could get the points for the first and last characters of the leaf and compare those. That's perhaps better than caching all the line starts, but it still means caching additional stuff in LocalAccessible.

Yeah this is immediately where my mind went. That, or caching the "empty" space within the bounds and checking if that changes? That's still (potentially) quite a lot of bookkeeping, and something we don't currently have logic for. 

> Any thoughts, Morgan? I'm not asking you to come up with a solution here, more just to check my thinking in case I've missed something or if anything comes to mind without digging too much. Can we do polygons? :)

You joke ;) but yeah, technically we could -- [we've got a class for that](https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/gfx/2d/Polygon.h#155). That class also has a nice `BoundingBox()` function, meaning if we decide to use Polygons internally, we can still easily send clients the rects they expect. Looks like for things that are actually rects (ie. text bounds that aren't multiline, and individual char bounds) we can just call `Polygon::FromRect`. Right now, we rely on the underlying `nsTextFrame` of a TextLeafAccessible to create its (cached) parent-relative bounds. If we're moving towards polygons, we'd want to override `LocalAccessible::ParentRelativeBounds` in `TextLeafAccessible` and do something similar to what I'm doing in the [`while/for` loop in `HyperTextAccessibleBase::TextBounds` ](https://phabricator.services.mozilla.com/D143527?id=571654). Instead of creating a line rect, and then unioning that into `result`, we'd create a line rect, turn it into a polygon with `FromRect()`, and then (create and) call a union function. There's probably a way to avoid duplicating too much of the code, like I mentioned in the patch, that `TextBounds` loop should ultimately live in TextLeafRange, probably. 

Famous last words but, seems doable if that's the route that makes the most sense 😀

(In reply to James Teh [:Jamie] from comment #4)
> (In reply to James Teh [:Jamie] from comment #0)
> > My current thinking is that whenever line boundaries change, the bounds should change too.
> 
> Ug. That doesn't always hold. Consider this:
> 
> `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>ac</span>def`
> 
> That gives you this:
> 
> ```
> ac
> de
> f
> ```
> 
> The second text leaf (starting at d) spans two lines. Even though there is only a single character f on the last line, the rect has to cover e, so the rect is the width of two characters.

So lineStarts would look like: `[0]` for the first leaf and `[0, 2]` for the second leaf

> 
> Now, if you insert b between a and c, you get this:
> 
> ```
> ab
> cd
> ef
> ```
> 
> The second text leaf reflows. It only includes one character on the second line now (d), but because it includes e, the rect's left coordinate is the first character. Because it includes f, the width is two characters. Unfortunately, that means the rect doesn't change... and the text hasn't changed either. :(

... And now lineStarts is `[0, 2]` and `[1]` for the leaves respectively.

So your point is the bounds (er, bounding rect, what we get from `Bounds()`) for the final text leaf wouldn't change, so we can't rely on that as an indicator to update our lineStarts array? Just wanna make sure I have this right 😀 I wonder if you could use text length to help with this. It seems like lineStarts would only be affected (a) in the leaf an insertion or deletion occurred in and (b) any leaves after that in the reading order that don't force a line break themselves. Is that fair to say?

If you have something like `data:text/html,<div contenteditable style="word-break: break-word; width: 2ch;"><span>abc</span><br>def` for example, inserting "b" in the same fashion doesn't change the lineStarts of the last leaf because the <br> means it isn't dependent on the leaf before.

It isn't super efficient, but if you're monitoring text-length changes, you at least have a narrowed range of leaves that may be affected. And, if we keep track of where "hard" linebreaks occur, that could narrow the range further. We'd have to do a traversal, and potentially reconcile/coalesce updates if multiple changes happened at once. This would also mean changes to acc A queue cache updates for accs B, C, D ... instead of our existing paradigm where changes to acc A queue updates for acc A only. Maybe we've already diverged from that though I dunno. 

> I don't know what to do about this. Sending cached lines every time we reflow seems bad for the same reason that sending bounds every time we reflow is bad. (That's why we cache bounds in LocalAccessible.) But caching lines in LocalAccessible as well seems really horrible.
> 
> I guess we could get the points for the first and last characters of the leaf and compare those. That's perhaps better than caching all the line starts, but it still means caching additional stuff in LocalAccessible.

Yeah this is immediately where my mind went. That, or caching the "empty" space within the bounds and checking if that changes? That's still (potentially) quite a lot of bookkeeping, and something we don't currently have logic for. 

> Any thoughts, Morgan? I'm not asking you to come up with a solution here, more just to check my thinking in case I've missed something or if anything comes to mind without digging too much. Can we do polygons? :)

You joke ;) but yeah, technically we could -- [we've got a class for that](https://searchfox.org/mozilla-central/rev/dc09246dfbfd8dafeb6d55ebee18a6294d525443/gfx/2d/Polygon.h#155). That class also has a nice `BoundingBox()` function, meaning if we decide to use Polygons internally, we can still easily send clients the rects they expect. Looks like for things that are actually rects (ie. text bounds that aren't multiline, and individual char bounds) we can just call `Polygon::FromRect`. Right now, we rely on the underlying `nsTextFrame` of a TextLeafAccessible to create its (cached) parent-relative bounds. If we're moving towards polygons, we'd want to override `LocalAccessible::ParentRelativeBounds` in `TextLeafAccessible` and do something similar to what I'm doing in the [`while/for` loop in `HyperTextAccessibleBase::TextBounds` ](https://phabricator.services.mozilla.com/D143527?id=571654). Instead of creating a line rect, and then unioning that into `result`, we'd create a line rect, turn it into a polygon with `FromRect()`, and then (create and) call a union function. There's probably a way to avoid duplicating too much of the code, like I mentioned in the patch, that `TextBounds` loop should ultimately live in TextLeafRange, probably. 

Famous last words but, seems doable if that's the route that makes the most sense 😀