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

Add missing formatting information in MS Excel cells (strikethrough, superscript/subscript) #12264

Merged
merged 4 commits into from Apr 20, 2021

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Apr 2, 2021

Link to issue number:

None

Summary of the issue:

Some formatting information is not reported when navigating from one cell to another in Excel, although the corresponding category is checked in Document formatting settings.

  • strikethrough is not reported even if font attributes is checked
  • superscript or subscript is not reported even if the corresponding option is checked

Description of how this pull request fixes the issue:

  • Added the missing code to handle strikethrough, superscript and subscript in _getFormatFieldAndOffsets
  • In reportFocus, speak the format field sequence not only if the current cell has format field info to speak but also if the previous cell has it. This is done to report 'baseline' when transitioning from a cell with superscript to a cell without formatting since 'baseline', being the default, is not stored in the cell's format fields.

Note

This PR allows to have strikethrough and superscript/subscript reported if the whole cell is formatted with these attributes.
If having a whole cell formatted as superscript or subscript is probably not a real-life use case, strikethrough may be quite common.

Testing strategy:

Manual test:

  1. Created an Excel sheet with some cells with various formatting: strikethrough, underline, bold, superscript, subscript. No cell has mixed formatting, i.e. formatting applied to only some characters of the text of a cell.
  2. Set NVDA to report font attributes and not superscript/subscript.
  3. Moved the active cell with arrows to perform each possible transition on the sheet.
  4. Re-test step 3 with the following options:
    • font attributes OFF / superscript and subscript ON
    • font attributes OFF / superscript and subscript OFF
    • font attributes ON / superscript and subscript ON

No specific unit or system test for this PR.

Known issues with pull request:

This PR does not address the following points that are still issues with NVDA:

Change log entry:

Section: Bug fixes
Strikethrough, superscript and subscript are now reported when navigating through Excel cells if the corresponding option is enabled (#12264)

Code Review Checklist:

This checklist is a reminder of things commonly forgotten in a new PR.
Authors, please do a self-review and confirm you have considered the following items.
Mark items you have considered by checking them.
You can do this when editing the Pull request description with an x: [ ] becomes [x].
You can also check the checkboxes after the PR is created.

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@AppVeyorBot
Copy link

See test results for failed build of commit 3ea39bcef6

@CyrilleB79
Copy link
Collaborator Author

The failing test is Robot.chromeTests.ARIA treegrid
Seems totally unrelated to this PR.

@CyrilleB79 CyrilleB79 closed this Apr 4, 2021
@CyrilleB79 CyrilleB79 reopened this Apr 4, 2021
@AppVeyorBot
Copy link

See test results for failed build of commit ac7c06ef12

@CyrilleB79
Copy link
Collaborator Author

Oh, I have closed this PR by mistake and then re-opened it. This seems to trigger a new AppVeyor build. Good to know...

Now, another system test is failing: Robot.chromeTests.i7562.
Still unrelated to this PR.

@seanbudd seanbudd self-assigned this Apr 20, 2021
@AppVeyorBot
Copy link

See test results for failed build of commit ccae80cdb0

@AppVeyorBot
Copy link

See test results for failed build of commit 23be5e7cbe

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @CyrilleB79 for this, and for your patience dealing with our randomly failing system tests.

Here's the excel file I used to test with
test-12264.xlsx

@seanbudd seanbudd merged commit 5de73c1 into nvaccess:master Apr 20, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Apr 20, 2021
@CyrilleB79 CyrilleB79 deleted the excelFormatting branch April 20, 2021 14:34
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