2006816 - Fix bugs in CaretLineNumber

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Spun off bug 1972457 comment 35. I'm not sure whether fixing these problems will fix that bug, but in the absence of clarity, correctness is a good starting point.

1. CaretLineNumber calls TextLeafPoint::GetCaret. For LocalAccessible, GetCaret calls HyperTextAccessible::CaretOffset, then HyperTextAccessibleBase::ToTextLeafPoint. However, that will return the wrong point if the caret is more than a single Accessible deep (i.e. grandchild or deeper) beneath the origin and not immediately at the start of all the descendants. The reason is that ToTextLeafPoint will traverse descendants using offset 0. We need to use SelectionManager::AccessibleWithCaret, and it that fails, use HyperTextAccessible::CaretOffset and keep traversing using CaretOffset on each descendant until we get a leaf. This bug causes us to report the same line number on every line of a descendant container.
2. CaretLineNumber gets the caret, gets the start of the origin container and walks back by line until it hits the start of the origin container, increasing the line count as it goes. This means that if you're somewhere after the first character on the line, the line count is off by 1, since the first line you hit is actually the start of the line under the caret. This could be fixed by including the origin when querying the first line. Alternatively, it's probably easier to walk forward instead of backward, in which case you don't need to treat the first differently.

Walking forward by line is also a more tested code path than walking backward, since we walk forward to build the RemoteAccessible cache. Obviously, we should fix bugs in both, but if we're going to change it for other reasons anyway, that might be more reliable.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 2006816 part 1: Add more tests for HyperTextAccessibleBase::CaretLineNumber via the line-number object attribute.

Some of these are marked as todo because they currently fail.
The subsequent patches will fix that.

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 2006816 part 2: Fix CaretLineNumber returning a number 1 larger than it should when the caret is not on the first character of a line.

Previously, it walked back by line start from the caret until it reached the start of the container.
If the caret was after the first character on a line, the first line start it encountered was the start of the current line.
To fix this, walk forward from the start of the container to the caret instead.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 2006816 part 3: Fix TextLeafPoint::GetCaret for LocalAccessible when the caret is deeper than a direct child of the origin container.

Previously, it used HyperTextAccessible::CaretOffset on the origin container, then ToTextLeafPoint on that offset.
However, offsets can only refer to a direct child.
This meant that ToTextLeafPoint was descending into the direct child using offset 0; i.e. returning the start of the child's content.
This was completely incorrect when the caret wasn't at the start of the child's content.
To fix this, first try using the caret cached by SelectionManager, as that avoids redundant transformation of the caret for each descendant.
If that fails, traverse using CaretOffset on each descendant until we can't go any further.

Comment 4

[James Teh [:Jamie]]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=169829

Comment 5

[James Teh [:Jamie]]

Strangely, these patches actually make it easier to reproduce bug 1972457! 🤯 I still think we should land these, but we can't land them until we track the other bug down.

Comment 6

[James Teh [:Jamie]]

Turns out I introduced a new version of the same bug. That's fixed now, with tests.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c61290c87180
https://hg.mozilla.org/integration/autoland/rev/749314a20da6
part 1: Add more tests for HyperTextAccessibleBase::CaretLineNumber via the line-number object attribute. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/0615fb365e73
https://hg.mozilla.org/integration/autoland/rev/fe77a23c14fa
part 2: Fix CaretLineNumber returning a number 1 larger than it should when the caret is not on the first character of a line. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/ea44e6826a94
https://hg.mozilla.org/integration/autoland/rev/41f14f941d61
part 3: Fix TextLeafPoint::GetCaret for LocalAccessible when the caret is deeper than a direct child of the origin container. r=eeejay

Comment 8

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/749314a20da6
https://hg.mozilla.org/mozilla-central/rev/fe77a23c14fa
https://hg.mozilla.org/mozilla-central/rev/41f14f941d61