1951573 - Unify the accessibility code to set the caret

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

There are two very different accessibility code paths for setting the caret: HyperTextAccessible::SetCaretOffset (used by IA2 and ATK) and TextLeafRange::SetSelection (used by Mac and UIA). The former performs some additional tasks which the latter does not: focusing the control, scrolling the selection into view and updating DOM nsFocusmanager. I think we should unify these so that we have a single code path, with SetCaretOffset being mostly a wrapper around SetSelection.

I started working on this in bug 1950785, thinking this solved a real bug. It turns out that my testing was incorrect and the bug I thought I spotted actually doesn't exist. 😳 However, I still think it makes sense to do this and I suspect it will make weird bugs in this area easier to manage in future.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1951573 part 1: Make it possible to remove all existing selections when calling TextLeafRange::SetSelection.

Previously, a caller had to first retrieve all selection ranges and then remove them individually.
This makes it awkward to perform a fairly common operation.

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1951573 part 2: Make TextLeafRange::SetSelection perform additional tasks to ensure the selection is set correctly.

HyperTextAccessible::SetSelectionRange (used by SetCaretOffset) performed these tasks, but TextLeafRange::SetSelection didn't.
This meant that the behaviour when setting the caret or selection was different for different OS APIs: IA2 and ATK use HyperTextAccessible::SetCaretOffset, while Mac and UIA use TextLeafRange::SetSelection.
While we aren't aware of any specific bugs impacting users, this is confusing at best and might result in obscure bugs in future.
See the code comments for details.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1951573 part 3: Implement HyperTextAccessibleBase::SetCaretOffset using TextLeafRange::SetSelection.

TextLeafRange::SetSelection should now be the one place where all text selections are set.
Previously, we had a separate local implementation for SetCaretOffset which differed slightly from using TextLeafRange::SetSelection.
Now, these should be equivalent, so there's no need for this separate local implementation.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1951573 part 4: Remove HyperTextAccessible::SetSelectionRange.

There were still some callers of SetSelectionRange, but these can now all be trivially converted to use SetSelectionBoundsAt (which calls TextLeafRange::SetSelection).

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1951573 part 5: Add an additional Select() test for UIA which tests setting the caret in content which isn't editable.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f30edc52101d
part 1: Make it possible to remove all existing selections when calling TextLeafRange::SetSelection. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/270f08d9691e
part 2: Make TextLeafRange::SetSelection perform additional tasks to ensure the selection is set correctly. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/9e306ea56c79
part 3: Implement HyperTextAccessibleBase::SetCaretOffset using TextLeafRange::SetSelection. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/5f90dc08ec62
part 4: Remove HyperTextAccessible::SetSelectionRange. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/23a8b787c70c
part 5: Add an additional Select() test for UIA which tests setting the caret in content which isn't editable. r=eeejay

Comment 7

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f30edc52101d
https://hg.mozilla.org/mozilla-central/rev/270f08d9691e
https://hg.mozilla.org/mozilla-central/rev/9e306ea56c79
https://hg.mozilla.org/mozilla-central/rev/5f90dc08ec62
https://hg.mozilla.org/mozilla-central/rev/23a8b787c70c