1905021 - HyperTextAccessible::IsCaretAtEndOfLine returns true when it shouldn't

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

STR:

1. Open this test case:
   data:text/html,<div contenteditable>a<span>b
2. Focus the editable text.
3. Press right arrow.
4. Open the NVDA Python console with NVDA+control+z.
5. Enter this command:
   nav.IAccessibleTextObject.textAtOffset(-2, 0)
   • Note: -2 is caret. 0 is character.
   • Expected: (1, 2, 'b')
   • Actual: (1, 1, None)

Impact: This breaks caret exposure when moving the cursor forward to the start of a new node. This bug has existed for a long time, but it's more problematic now that we're implementing UIA text and now that NVDA will start using IA2_TEXT_OFFSET_CARET.

This occurs because HyperTextAccessible::IsCaretAtEndOfLine() returns true here, but it shouldn't; we're definitely not at the end of a line. That function assumes that CaretAssociationHint::Before always means that the caret is at the end of a line, which is a reasonable assumption given the code comments but doesn't seem to be true. It can also mean that the caret is positioned before the start of a node. This behaviour seems to be true going as far back as 2013 (bug 880159 when a11y first started using this), probably much further. We need to detect that case and return false. That's probably going to involve checking whether the previous character is on a different line if mOffset is 0. (If mOffset > 0, this is not a node boundary, so this case doesn't apply.)

Comment 1

[James Teh [:Jamie]]

Emilio, do you agree that this code comment looks wrong? The comment says:

Hint to tell if the selection is at the end of this line or beginning of next.

IN reality, the hint can also be Before if the caret is moved forward to the start of a new node.

Is there some way (in addition to the caret association hint) to differentiate between this node crossing case and the end of wrapped line case?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Yeah I agree, that hint is not about lines necessarily, it's also about frames and nodes. To know whether the caret offset is at the beginning of the line you need to look at the caret position as well and confirm it's a line boundary, or something along those lines.

Comment 3

[James Teh [:Jamie]]

This nearly works, but for some reason, when you first focus a contentEditable, you get CaretAssociationHint::Before. I can't tell the difference between that case and the case that you're at the start of a node which wrapped onto a new line.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

You get CaretAssociationHint::Before, with which caret position? Is the contenteditable node empty? Hard to say off-hand without that. I assume the behavior might be different with a <span contenteditable> vs a <div contenteditable> too?

Comment 5

[James Teh [:Jamie]]

The contentEditable is not empty. Caret position 0 in the first text node.

I think I can work around this by checking if this is the start of a block, but... this is definitely going to be ugly. :)

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1905021: Don't incorrectly treat the start of a node inside a contentEditable as the end of a line.

Previously, we assumed that CaretAssociationHint::Before always meant the caret was at the insertion point at the end of a line.
However, it can also mean that the caret is before the start of a node in the middle of a line.
To fix this, we need to check for line and paragraph boundaries.
See the code comments for details.

I moved this functionality completely out of HyperTextAccessible and into TextLeafPoint.
First, it was easiest to do these checks with private functions already available to TextLeafPoint.
Second, this ideally belongs in TextLeafPoint anyway; its existence in HyperTextAccessible was vestigial.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5ff95602119
Don't incorrectly treat the start of a node inside a contentEditable as the end of a line. r=nlapre

Comment 8

[Atila Butkovits]

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

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1905021: Don't incorrectly treat the start of a node inside a contentEditable as the end of a line.

Previously, we assumed that CaretAssociationHint::Before always meant the caret was at the insertion point at the end of a line.
However, it can also mean that the caret is before the start of a node in the middle of a line.
To fix this, we need to check for line and paragraph boundaries.
See the code comments for details.

I moved this functionality completely out of HyperTextAccessible and into TextLeafPoint.
First, it was easiest to do these checks with private functions already available to TextLeafPoint.
Second, this ideally belongs in TextLeafPoint anyway; its existence in HyperTextAccessible was vestigial.

Original Revision: https://phabricator.services.mozilla.com/D215089

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This causes frequent accessibility crashes. The change will be backed out from the next Nightly which starts to build at 10pm UTC.

Comment 11

[Norisz Fay [:noriszfay]]

Backed out as requested for causing accessibility crashes related to selection
Backout link: https://hg.mozilla.org/mozilla-central/rev/48a7ce6fe0888f9be96810dcf74829c995f29764

Comment 12

[James Teh [:Jamie]]

Attachment: Bug 1905021: Don't incorrectly treat the start of a node inside a contentEditable as the end of a line.

Previously, we assumed that CaretAssociationHint::Before always meant the caret was at the insertion point at the end of a line.
However, it can also mean that the caret is before the start of a node in the middle of a line.
To fix this, we need to check for line and paragraph boundaries.
See the code comments for details.

Comment 13

[James Teh [:Jamie]]

The crash occurs when ToTextLeafPoint on the HyperTextAccessible with the caret offset returns an invalid point. I don't understand how that could happen, though. That should only happen if an invalid HyperText offset is provided, but the caret offset retrieved from HyperTextAccessible::CaretOffset should never be invalid. I've added an assertion to try to catch this with a graceful return for non-debug builds.

I did take a look at some crash dumps, but unfortunately, all of the variables I need for better diagnosis in the TextLeafPoint::GetCaret stack frame aren't included in any of the dumps.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ab575b044cf
Don't incorrectly treat the start of a node inside a contentEditable as the end of a line. r=nlapre

Comment 15

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/7ab575b044cf

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9410374 [details]
Bug 1905021: Don't incorrectly treat the start of a node inside a contentEditable as the end of a line.

I'm going to sit on this request for now as we're in RC week and it only re-landed on Nightly today and it seems this could use some bake time. We can re-evaluate for the mid-cycle scheduled dot release.

Comment 17

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: Some screen readers will fail to read when navigating by character in rich text editors in some cases.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: NA
• Risk associated with taking this patch: low
• Explanation of risk level: Isolated to a single function with multiple error checks. Covered by automated tests. This is a simpler version of the patch that landed on central.
• String changes made/needed: none
• Is Android affected?: no

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Did you mean to set ESR115 to affected, Jamie?

Comment 19

[James Teh [:Jamie]]

Yes, that was deliberate. It turns out that this bug goes back a long way, and recent fixes in the NVDA screen reader are likely to expose this bug such that it impacts users.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/eae21a8c4caa

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9410374 [details]
Bug 1905021: Don't incorrectly treat the start of a node inside a contentEditable as the end of a line.

Approved for 128.1esr.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/f1220475a545

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This'll need a rebased patch for ESR115 if you wanted to uplift there also.

Comment 24

[James Teh [:Jamie]]

This might have been worthwhile if we could take the beta patch, but requiring yet another patch probably isn't worth it given the time we have left until ESR 115 is EOL. It also just occurred to me that if folks haven't updated to ESR 128 yet, they'll probably also be behind on NVDA updates, in which case this won't impact them.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Added to the 128.0.2 relnotes:

Fixed an issue causing some screen readers to fail to read when navigating by character in rich text editors.