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

Enhance Microsoft Word Comment Command: Press Twice to Present Comment Content in Browsable message. #16800

Merged
merged 12 commits into from
Jul 12, 2024

Conversation

cary-rowen
Copy link
Contributor

@cary-rowen cary-rowen commented Jul 3, 2024

Link to issue number:

Partially fixed #14628
Replaces abandoned #15987

Thanks to @tseykovets for most of the work.

Summary of the issue:

Sometimes the text of comment is too long, in order to understand, we may need to navigate by sentence or by word.

Description of user facing changes

This pull request adds the ability to present the text of a comment in browse mode by pressing command twice (NVDA+Alt+C).

Description of development approach

A handler for the number of presses has been added to the script (if repeats).
If pressed twice, presents the information in browse mode (ui.browseableMessage).

Testing strategy:

Tested manually in Microsoft Word.

Known issues with pull request:

Unknown

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.

Summary by CodeRabbit

  • New Features
    • Enhanced script to report text at the system caret location and provide additional information when pressed twice in browse mode.
    • Improved message handling in comments, presenting different messages based on script repeats.

@cary-rowen cary-rowen requested a review from a team as a code owner July 3, 2024 02:22
Copy link
Contributor

coderabbitai bot commented Jul 3, 2024

Walkthrough

The changes in the NVDA screen reader's codebase enhance the functionality of scripts in Microsoft Word documents. Specifically, the updates enable better interaction with the system caret and comments, allowing users to open comments in a browsable dialog on a repeated key press and providing improved message handling based on script repetitions.

Changes

Files Change Summary
source/NVDAObjects/IAccessible/winword.py Updated script descriptions and functionalities to handle caret position and comments, allowing for presentation in browse mode.
source/NVDAObjects/UIA/wordDocument.py Added scriptHandler import and modified the script_reportCurrentComment to handle repeated key presses for browse mode.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant NVDA
    participant WordDocument

    User->>+NVDA: Press NVDA + Alt + C
    NVDA->>+WordDocument: Fetch comment text where caret is located
    WordDocument-->>-NVDA: Return comment text
    NVDA->>-User: Report comment text

    User->>+NVDA: Press NVDA + Alt + C again
    NVDA->>+WordDocument: Fetch comment text where caret is located
    WordDocument-->>-NVDA: Return comment text
    NVDA->>+User: Open browsable dialog with comment text
Loading

Assessment against linked issues

Objective Addressed Explanation
Support opening comment text where the system caret is located in a browsable dialog (#14628)
Handle repeated key presses for comments to show different types of messages (#14628)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

source/NVDAObjects/IAccessible/winword.py Outdated Show resolved Hide resolved
source/NVDAObjects/UIA/wordDocument.py Outdated Show resolved Hide resolved
@cary-rowen cary-rowen requested a review from a team as a code owner July 3, 2024 02:26
user_docs/en/changes.md Outdated Show resolved Hide resolved
source/NVDAObjects/UIA/wordDocument.py Outdated Show resolved Hide resolved
source/NVDAObjects/UIA/wordDocument.py Outdated Show resolved Hide resolved
source/NVDAObjects/UIA/wordDocument.py Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
@seanbudd seanbudd marked this pull request as draft July 3, 2024 02:41
@cary-rowen cary-rowen requested a review from seanbudd July 3, 2024 03:18
@cary-rowen cary-rowen marked this pull request as ready for review July 3, 2024 03:19
user_docs/en/changes.md Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

seanbudd commented Jul 3, 2024

Thanks @cary-rowen

@seanbudd
Copy link
Member

seanbudd commented Jul 3, 2024

@cary-rowen can you confirm you've tested the latest changes you've pushed?

@CyrilleB79
Copy link
Collaborator

I wonder if we cannot propose another UX here, e.g.:

  • First press reports the comment
  • Second press jumps in the comment area

This would allow to navigate in the comment, but also in the answers of the comment.

Also, it's worth noting that there are various issues related to Word comments (#14985, #9685, #4336). The use case of comments with replies is much less common than single comments. But should we really continue to ignore it totally in the UX.

Of course, a third path can be to merge this PR as is and to modify again the UX when the other issues are handled, with the risk though that users used to the new UX provided in this PR (the browseable message) be disappointed if it is removed in the future.

It should be NV Access decision (@seanbudd, @Qchristensen), but I just write this here to have all the related issues in mind for everyone.

@cary-rowen
Copy link
Contributor Author

cc @seanbudd

can you confirm you've tested the latest changes you've pushed?
Yes I tested locally using Microsoft 365MSO (Version 2406 Build 16.0.17726.20078) 64 Bit.

@cary-rowen
Copy link
Contributor Author

Hi @CyrilleB79

First press reports the comment
Second press jumps in the comment area

I just to confirm, by "comment area" you mentioned here, do you mean "sidetrack" which can be focused with Alt+F12?

Also, it's worth noting that there are various issues related to Word comments

Yes, I'm aware of these reported issues as well, especially #14985 which deserves to be fixed in a separate PR.

Regarding the UX considerations you mentioned, I'd also like to hear NV Access's thoughts. Especially if Alt+F12 can focus the sidetrack.

@AppVeyorBot
Copy link

See test results for failed build of commit 4fdf7880a8

@seanbudd seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Jul 4, 2024
@seanbudd seanbudd self-requested a review July 4, 2024 06:03
@CyrilleB79
Copy link
Collaborator

I just to confirm, by "comment area" you mentioned here, do you mean "sidetrack" which can be focused with Alt+F12?

On my end, alt+F12 is not working. Though, there is an important difference between modern comments and classical ones. Modern ones seem to be available only with Word 365; I have Word 2016 here.

@cary-rowen
Copy link
Contributor Author

Hi @CyrilleB79

Due to the differences between Office versions, I'm not quite sure what we can do in this PR to unify the UX.

Whether it is Office365 or 2016, Office seems to have native shortcut keys for focusing on the comment area.

NVDA users will not be unfamiliar with browsable dialog. For NVDA, such browsable message are used very widely, such as report formatting information.
As for other issues related to report comments, they are definitely worth fixing in separate PRs.

@seanbudd Any idea?
Thanks

@seanbudd seanbudd added conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. and removed blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. labels Jul 4, 2024
@seanbudd
Copy link
Member

seanbudd commented Jul 4, 2024

we are happy to accept this as-is. In the future we can consider performing alt+f12 on triple press

@AppVeyorBot
Copy link

See test results for failed build of commit 01ff017f55

@CyrilleB79
Copy link
Collaborator

@cary-rowen wrote:

Due to the differences between Office versions, I'm not quite sure what we can do in this PR to unify the UX.

Whether it is Office365 or 2016, Office seems to have native shortcut keys for focusing on the comment area.

Actually, I didn't know that there were a native shortcut to edit a comment in Word 365. I do not know of an equivalent shortcut in Word 2016, except of course the sequential one corresponding to what is found in the ribbon.

@cary-rowen also wrote:

NVDA users will not be unfamiliar with browsable dialog. For NVDA, such browsable message are used very widely, such as report formatting information. As for other issues related to report comments, they are definitely worth fixing in separate PRs.

I agree that the browseable message is very familiar for NVDA users and is probably easier to navigate than Word's native comment area.
Since I am not able to suggest something to improve the UX in Word's native comment area, nor to suggest a way to reach a unified experience between 365 and 2016, you can just ignore my previous suggestion.

If possible, a second step (in a separate PR) could be to add the replies in the browseable message, in HTML, with separators between each one.

@seanbudd
Copy link
Member

Please disregard previous comment

Can you please fix the lint @cary-rowen - I'm not sure what's wrong with the lint artifacts, can you run runlint.bat locally and report the results?

@cary-rowen
Copy link
Contributor Author

Hi @seanbudd
I executed runlint.bat and the following information was output. Can I commit these changes?

D:\git\nvda>runlint.bat
Ensuring NVDA Python virtual environment
call ruff check --fix
All checks passed!
Deactivating NVDA Python virtual environment
Ensuring NVDA Python virtual environment
call ruff format
557 files reformatted, 15 files left unchanged
Deactivating NVDA Python virtual environment

@cary-rowen
Copy link
Contributor Author

Hi @CyrilleB79

Thank you for your previous thoughts in bringing these existing issues to the attention of the community.

If possible, a second step (in a separate PR) could be to add the replies in the browseable message, in HTML, with separators between each one.

This is a good idea. I find it very useful.

@seanbudd
Copy link
Member

@cary-rowen - is that after merging the latest master? There really shouldn't be that many changes.

@AppVeyorBot
Copy link

See test results for failed build of commit 5a6bf0ea46

@seanbudd
Copy link
Member

@cary-rowen - see the lint results here: https://ci.appveyor.com/project/NVAccess/nvda/builds/50189139#L5378
please add the trailing commas

@hwf1324
Copy link
Contributor

hwf1324 commented Jul 11, 2024

@cary-rowen - is that after merging the latest master? There really shouldn't be that many changes.

For Git for Windows, it seems that even if you merge the latest master, the local files don't update to the end of the LF line. It also doesn't seem to create new commits after committing those changes though?

My git settings should be checkout as is.

@seanbudd seanbudd marked this pull request as draft July 12, 2024 00:16
@seanbudd seanbudd marked this pull request as ready for review July 12, 2024 02:33
@seanbudd seanbudd merged commit 41418d7 into nvaccess:master Jul 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
5 participants