Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize review by collapsing before expanding #12825

Closed
wants to merge 1 commit into from

Conversation

codeofdusk
Copy link
Contributor

Link to issue number:

Closes #12808.

Summary of the issue:

Due to differences in behaviour between UIA and MSAA text ranges, some UIA text ranges never report "bottom" when moving past the last line of text.

Description of how this pull request fixes the issue:

In review, normalize text ranges by collapsing to start before expanding by the unit being reviewed (i.e. move an expanded range, not a collapsed one).

Testing strategy:

Tested Microsoft Word with UIA enabled, an upcoming UIA Windows Console, and Notepad (both MSAA and UIA proxied MSAA) by moving through multiline text. Verified that "bottom" is properly reported when moving by line in all cases.

Known issues with pull request:

I'm not 100% set on this approach, mostly because I don't fully understand this code. This may introduce additional bugs.

I've opened this PR as a possible fix and to start a discussion on resolving this issue as it will continue affecting word (even after UIA) and will soon affect consoles.

Change log entries:

== Bug Fixes ==

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • API is compatible with existing addons.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@codeofdusk codeofdusk requested a review from a team as a code owner September 9, 2021 14:33
@seanbudd seanbudd self-assigned this Sep 10, 2021
@LeonarddeR
Copy link
Collaborator

Note that this changes behavior in two ways:

  1. It collapses before movement
  2. It no longer collapses after movement. I think that's still necessary to ensure the cursor is at the start of the new unit.

Shouldn't we still collapse after movement? For UIA and OffsetsTextInfo, I believe after movement, the range is still collapsed, but I don't think that's the case for every single textInfo.

Also note that it's not only IAccessible and UIA we have to consider. There's DisplayModelTextInfo, Java Access Bridge, Word object model, etc. The default move method on OffsetsTextInfo also collapses beforehand when no endpoint is defined.

I think the following questions should be answered for every TextInfo implementation in core before continuing:

  1. Does it collapse before a move automatically (OffsetsTextInfo) or does it need an additional collapse (UIA)?
  2. Does it collapse after a move automatically (OffsetsTextInfo) or not, or is the collapsed state dependent on the state before movement?

We first need to uniformize how move behaves before changing any review command logic.

@codeofdusk
Copy link
Contributor Author

Closing in favour of a better approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some UIA text ranges have no apparent bottom
3 participants