Skip to content

Reduce verbosity when reporting Excel cell formatting#15560

Merged
seanbudd merged 5 commits into
nvaccess:masterfrom
CyrilleB79:noNegFmtReport
Oct 10, 2023
Merged

Reduce verbosity when reporting Excel cell formatting#15560
seanbudd merged 5 commits into
nvaccess:masterfrom
CyrilleB79:noNegFmtReport

Conversation

@CyrilleB79

@CyrilleB79 CyrilleB79 commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Link to issue number:

None

Summary of the issue:

When requesting for cell formatting information in Excel (NVDA+F), there is over-verbose information:

  • "no border lines" is often reported, since Excel files with default borders are quite common
  • "background pattern none" is almost always reported since cells with other background patterns are very rare

Description of user facing changes

When formatting information is requested with NVDA+F, we will not report anymore a lack of specific formatting for border lines or for background pattern. This formatting will still be reported during navigation if the corresponding option is enabled in document formatting settings (respectively "cell borders" and "color").

In that sense, we follow the logic used for other formatting attributes that have a negative state (lack of such formatting) such as bold/italic/etc. E.g.: "no bold" is not formatted upon NVDA+F request, but it is still reported while navigating from bold to no bold if the corresponding doc formatting option is enabled.

Description of development approach

Do not provide info on background pattern in formatting info fields and changed the logic in speech.py to handle this.

For UIA, nothing is done since NVDA does not report borders or background pattern for Excel cells when UIA is enabled.

Additional note

_getFormatFieldAndOffsets is private but it's probably called by getTextWithFields, thus changing its result.
Thus should I indicate something in API breaking changes though?

Testing strategy:

Manual test: Tested NVDA+F and navigation with option "color" and "cell borders" enabled.

Known issues with pull request:

None

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.

@CyrilleB79 CyrilleB79 changed the title No neg fmt report Reduce verbosity when reporting Excel cell formatting Oct 9, 2023
@CyrilleB79 CyrilleB79 marked this pull request as ready for review October 9, 2023 14:16
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner October 9, 2023 14:16
@CyrilleB79 CyrilleB79 requested a review from seanbudd October 9, 2023 14:16
@lukaszgo1

Copy link
Copy Markdown
Contributor

Excel UIA not supported.

Have you tried to implement the same fix for UIA and encountered some difficulties, or is that because when accessing Excel via UIA this problem does not occur?

@CyrilleB79 CyrilleB79 marked this pull request as draft October 9, 2023 19:47
@CyrilleB79

Copy link
Copy Markdown
Contributor Author

Excel UIA not supported.

Have you tried to implement the same fix for UIA and encountered some difficulties, or is that because when accessing Excel via UIA this problem does not occur?

The problem of such verbosity is not present with UIA. But with UIA, there seem to be an error with this build when requesting formatting info; this error does not occur in alpha build.

Converting to draft to clarify this.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review October 9, 2023 21:05
@CyrilleB79

Copy link
Copy Markdown
Contributor Author

The problem of such verbosity is not present with UIA.

Initial description updated now.

But with UIA, there seem to be an error with this build when requesting formatting info; this error does not occur in alpha build.

There was actually no error; tests probably not executed correctly the first time.
So I'am setting this PR as ready again.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Oct 9, 2023

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @CyrilleB79

Comment thread source/speech/speech.py Outdated
@seanbudd seanbudd merged commit 74476eb into nvaccess:master Oct 10, 2023
@nvaccessAuto nvaccessAuto added this to the 2024.1 milestone Oct 10, 2023
@CyrilleB79 CyrilleB79 deleted the noNegFmtReport branch October 10, 2023 07:25
Comment thread source/speech/speech.py
backgroundPattern=attrs.get("background-pattern")
oldBackgroundPattern=attrsCache.get("background-pattern") if attrsCache is not None else None
if (backgroundPattern or oldBackgroundPattern is not None)and backgroundPattern != oldBackgroundPattern:
if (backgroundPattern or oldBackgroundPattern is not None) and backgroundPattern != oldBackgroundPattern:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the linting. But why hasn't it been detected by appVeyor check?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume a flake8 bug

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

Development

Successfully merging this pull request may close these issues.

4 participants