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

Several fixes to native selection mode #16129

Merged
merged 4 commits into from Feb 6, 2024
Merged

Several fixes to native selection mode #16129

merged 4 commits into from Feb 6, 2024

Conversation

michaelDCurran
Copy link
Member

Link to issue number:

Fixes #16064
Fixes #16097

Summary of the issue:

Description of user facing changes

  • In firefox, If NVDA fails to update the native selection when turning on native selection mode, it is now left off, and the user is
    notified that native selection mode is not supported. this stops errors when moving with the arrow keys in Thunderbird after turning on native selection mode.
  • NVDA no longer incorrectly alerts the user that native selection mode is not supported in Microsoft Word. Rather, the message state that it canot be turned off.
  • when copying text with control+c in Microsoft Word with Browse mode on, formatting is now also copied, fulfilling the expectation of a native selection mode.

Description of development approach

  • Gecko virtualBuffer's updateAppSelection method: if the selection is collapsed, don't try fetching information for the selection, instead just clear the app selection. This error was previously silently ignored.
  • In UIA Browse mode documents, _nativeAppSelectionMode is now set to True, as this correctly reflects that UIA browse mode documents move the native selection.
  • BrowseMode document's toggleNativeSelectionMode script:
    • Tailor the message reported if native selection mode is not supported (by looking at _nativeSelectionMode). If _nativeSelectionMode is True, then the message is changed to state it can't be turned off.
    • If updateappSelection fails when turning on native selection mode, log the error, turn it off, and alert the user that it is not supported.
  • In MS word, suppress the "copy" UIA notification, if in MS word Browse mode. Otherwise there would be double speaking along with the message in cursorManager's copyToclipboard script when doing a native copy.

Testing strategy:

  • Opened a page in Firefox. Turned on native selection mode. Selected some text, copied, and pasted in to MS Word to confirm that the text was formatted.
  • Tried togling native sleection mode in MS word browse mode. Confirmed that NvDA reported that it could not be toggled off.
  • Temporarily caused Gecko virtualbuffer's updateAppSelection method to raise NotImplementedError, Opened a page in Firefox, and tried turning on native selection mode. Confirmed that NVDA reported it was not supported.

Known issues with pull request:

None known.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

…ing:

* In firefox, If NVDA fails to update the native selection when turning on native selection mode, it is now left off, and the user is notified that native selection mode is not supported. this stops errors when moving with the arrow keys in Thunderbird after turning on native selection mode.
* Exceptions are logged when turning on and off native selection mode.
* If native selection mode is not supported when trying to turn it on, NVDA honors the _nativeAppSelectionMode boolean to tailor the message to state that it cannot be turned off, if it is on (though not supported).
* UIA browse mode documents (E.g. MS word) set _nativeSelectionMode to True, though it cannot be turned off. This not only provides a better message to the user if they try and toggle it, but it also has the advantage that copying with control+c from MS word browse mode, will now also copy with formatting.
@michaelDCurran michaelDCurran requested a review from a team as a code owner February 5, 2024 03:31
@michaelDCurran michaelDCurran requested review from SaschaCowley and removed request for a team February 5, 2024 03:31
@seanbudd seanbudd added this to the 2024.1 milestone Feb 5, 2024
@AppVeyorBot
Copy link

See test results for failed build of commit e81027670d

seanbudd
seanbudd previously approved these changes Feb 5, 2024
source/browseMode.py Outdated Show resolved Hide resolved
…, as the methods already convert ot to NotImplementedError.
Co-authored-by: Sean Budd <sean@nvaccess.org>
Copy link
Collaborator

@CyrilleB79 CyrilleB79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, also update the change log to indicate that native copy (i.e. with formatting) is now done in Word Browse Mode. Before, in Word Browse Mode, formatting was copied when using Object Model, but not when using UIA.
Thanks.

Comment on lines +2040 to +2044
# Translators: the message when native selection mode is not available in this browse mode document.
ui.message(_("Native selection mode unsupported in this browse mode document"))
else:
# Translators: the message when native selection mode cannot be turned off in this browse mode document.
ui.message(_("Native selection mode cannot be turned off in this browse mode document"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"browse mode document" -> "document"

My feeling is that, for users and translators, there is no need to specify here that the document is a browse mode document (no possible ambiguity). This way, the messages are lighter. Feel free to ignore this suggestion if you disagree with me though.

Suggested change
# Translators: the message when native selection mode is not available in this browse mode document.
ui.message(_("Native selection mode unsupported in this browse mode document"))
else:
# Translators: the message when native selection mode cannot be turned off in this browse mode document.
ui.message(_("Native selection mode cannot be turned off in this browse mode document"))
# Translators: the message when native selection mode is not available in this document.
ui.message(_("Native selection mode unsupported in this document"))
else:
# Translators: the message when native selection mode cannot be turned off in this document.
ui.message(_("Native selection mode cannot be turned off in this document"))

@seanbudd seanbudd merged commit 29c0d72 into beta Feb 6, 2024
3 checks passed
@seanbudd seanbudd deleted the i16064 branch February 6, 2024 00:24
seanbudd pushed a commit that referenced this pull request Feb 9, 2024
…ult a and cannot be turned off, so ensure the toggle message reflects this. (#16151)

Follow up from #16129
Fixes #16097

Summary of the issue:
Selecting text in Browse mode in Microsoft Word moves the physical selection. Thus it is the equivalent to native selection mode. Pr #16129 ensured that Browse mode in MS word with UI Automation took this into account and made the toggle message report that native selection mode could not be turned off. However this has not been done for MS Word object model, and NVDA states that native selection mode is unsupported.

Description of user facing changes
When trying to toggle native selection mode in Browse mode in MS Word when not using UI Automation, NvDA will correctly state that native selection mode cannot be turned off.
When copying text with control+c in browse mode in MS Word when not using UI Automation, formatting is now also copied.
Description of development approach
Set browse mode for MS Word object model's _nativeAppSelectionMode to True.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Fixes nvaccess#16064
Fixes nvaccess#16097

Summary of the issue:
Native selection mode can be enabled in unsupported Gecko, such as Thunderbird 115.x nvaccess#16064: turning on native selection mode in thunderbird 115 would cause errors when moving with the arrow keys as IAccessibleTextSelectionContainer is unavailable.
Wrong message reported when toggling native selection in Word nvaccess#16097: NVDA is misleading in MS Word browse mode when it says that native selection mode is not supported when trying to toggle it on, as MS word Browse mode does move the caret / selection, thus it has always used native selection mode.. It is just you can't toggle it off.
Description of user facing changes
In firefox, If NVDA fails to update the native selection when turning on native selection mode, it is now left off, and the user is
notified that native selection mode is not supported. this stops errors when moving with the arrow keys in Thunderbird after turning on native selection mode.
NVDA no longer incorrectly alerts the user that native selection mode is not supported in Microsoft Word. Rather, the message state that it canot be turned off.
when copying text with control+c in Microsoft Word with Browse mode on, formatting is now also copied, fulfilling the expectation of a native selection mode.
Description of development approach
Gecko virtualBuffer's updateAppSelection method: if the selection is collapsed, don't try fetching information for the selection, instead just clear the app selection. This error was previously silently ignored.
In UIA Browse mode documents, _nativeAppSelectionMode is now set to True, as this correctly reflects that UIA browse mode documents move the native selection.
BrowseMode document's toggleNativeSelectionMode script:
Tailor the message reported if native selection mode is not supported (by looking at _nativeSelectionMode). If _nativeSelectionMode is True, then the message is changed to state it can't be turned off.
If updateappSelection fails when turning on native selection mode, log the error, turn it off, and alert the user that it is not supported.
In MS word, suppress the "copy" UIA notification, if in MS word Browse mode. Otherwise there would be double speaking along with the message in cursorManager's copyToclipboard script when doing a native copy.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
…ult a and cannot be turned off, so ensure the toggle message reflects this. (nvaccess#16151)

Follow up from nvaccess#16129
Fixes nvaccess#16097

Summary of the issue:
Selecting text in Browse mode in Microsoft Word moves the physical selection. Thus it is the equivalent to native selection mode. Pr nvaccess#16129 ensured that Browse mode in MS word with UI Automation took this into account and made the toggle message report that native selection mode could not be turned off. However this has not been done for MS Word object model, and NVDA states that native selection mode is unsupported.

Description of user facing changes
When trying to toggle native selection mode in Browse mode in MS Word when not using UI Automation, NvDA will correctly state that native selection mode cannot be turned off.
When copying text with control+c in browse mode in MS Word when not using UI Automation, formatting is now also copied.
Description of development approach
Set browse mode for MS Word object model's _nativeAppSelectionMode to True.
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.

None yet

4 participants