Skip to content

Fix Excel cell formatting reporting#15100

Merged
seanbudd merged 3 commits into
nvaccess:masterfrom
LeonarddeR:fixExcelFormatting
Jul 9, 2023
Merged

Fix Excel cell formatting reporting#15100
seanbudd merged 3 commits into
nvaccess:masterfrom
LeonarddeR:fixExcelFormatting

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Jul 4, 2023

Copy link
Copy Markdown
Collaborator

Link to issue number:

Fixes #15091

Summary of the issue:

Pr #14984 broke Excel cell formatting reporting in such a way that it would always report all formatting, regardless of whether the formatting changed between cells.

Description of user facing changes

Ensure that cell formatting isn't repeated needlessly.

Description of development approach

When getting the selection in script_changeSelection, override the parent of the selection with self. This ensures that the format field cache on the work sheet will persist when moving through cells.

Testing strategy:

Test the str from #15091

Known issues with pull request:

None known

Change log entries:

None needed, issue regressed after 2023.1.

Code Review Checklist:

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

@LeonarddeR LeonarddeR requested a review from a team as a code owner July 4, 2023 16:27
@LeonarddeR LeonarddeR requested a review from michaelDCurran July 4, 2023 16:27
@LeonarddeR LeonarddeR added this to the 2023.2 milestone Jul 4, 2023
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@CyrilleB79, could you please have a test?

@CyrilleB79

Copy link
Copy Markdown
Contributor

Tested from source (commit eeb7e49) with one property having default value (underline) and one property having no specific default value (font size). I confirm that the reported issue is fixed, i.e. moving from cell to cell reports the property only when it changes.

However I can see a non critical change of behaviour with the following STR:

  • Type "The cell's content" in a cell
  • Format it to underline
  • Press Alt; the ribbon is focused
  • Press alt again; the cell is focused again.

At last step, with NVDA 2023.1, we can hear: "Sheet2 table 11.0 pt underline The cell's content".

With this PR instead, we can hear: "Sheet2 table The cell's content", i.e. formatting is not reported again.

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@CyrilleB79 Thanks for testing. I think that finding, though it is a change, is consistent with how this happens in Word. For example:

  1. Enable font attribute reporting
  2. Open Word
  3. Type "hello"
  4. Select + underline
  5. Open and close the ribbon.
  6. Observe that NVDA will only report Underline after pressing down arrow, for example. It doesn't on first occasion.

@CyrilleB79

Copy link
Copy Markdown
Contributor

@CyrilleB79 Thanks for testing. I think that finding, though it is a change, is consistent with how this happens in Word. For example:

  1. Enable font attribute reporting
  2. Open Word
  3. Type "hello"
  4. Select + underline
  5. Open and close the ribbon.
  6. Observe that NVDA will only report Underline after pressing down arrow, for example. It doesn't on first occasion.

No, I disagree.

With your STR, in Word, it does not report immediately when leaving the ribbon. But if you ensure that the word "Hello" is not selected, e.g. just put the caret between the two l's of "Hello", you will have "underline" reported as soon as you leave the ribbon.
The same way, in Excel with NVDA 2023.1, if you have only one cell selected, the attributes are reported as soon as you leave the ribbon. But if you have a multi-cell selection, the attributes are not reported until you turn back to a single cell selection.
This has probably to do with the fact that NVDA does not report formatting when dealing with selection.

@LeonarddeR

LeonarddeR commented Jul 5, 2023 via email

Copy link
Copy Markdown
Collaborator Author

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author
* Type "The cell's content" in a cell

* Format it to underline

* Press Alt; the ribbon is focused

* Press alt again; the cell is focused again.

At last step, with NVDA 2023.1, we can hear: "Sheet2 table 11.0 pt underline The cell's content".

With this PR instead, we can hear: "Sheet2 table The cell's content", i.e. formatting is not reported again.

I actually can't reproduce this. For me the behavior is that underline is reported properly.

  1. Focus a1
  2. Type hello
  3. press enter, focus is at a2
  4. move up to A1
  5. Press ctrl + u to underline
  6. press alt twice

NVDA ssays: sheet 1 table underlined hello A1.

@CyrilleB79

Copy link
Copy Markdown
Contributor

Sorry, my STR was not complete enough.

If you take your STR, add the following steps to reproduce the issue:
7. Press again alt two times

Or alternatively:
Step 5.5 (between 5. and 6.): press rightArrow, then leftArrow

Comment thread source/NVDAObjects/window/excel.py Outdated
@seanbudd seanbudd marked this pull request as draft July 6, 2023 02:41
@LeonarddeR LeonarddeR force-pushed the fixExcelFormatting branch from eeb7e49 to 7c6b1db Compare July 7, 2023 05:34
@LeonarddeR LeonarddeR force-pushed the fixExcelFormatting branch from 7c6b1db to be1222f Compare July 7, 2023 05:51
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

Never mind, the approach I used was way to complex. I changed this pr with a two lines fix that should work as expected.

@CyrilleB79

Copy link
Copy Markdown
Contributor

Never mind, the approach I used was way to complex. I changed this pr with a two lines fix that should work as expected.
I have tested it and all goes well. Nice fix!

Setting oldSelection.parent to self when oldSelection.parent == self is not very intuitive when reading the code. Maybe you could add a comment to explain this lines of code? It also appears later in the function with oldSelection.parent==newSelection.parent.

@LeonarddeR LeonarddeR marked this pull request as ready for review July 7, 2023 16:20

@CyrilleB79 CyrilleB79 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good job! Seems all fine to me.

@seanbudd seanbudd self-requested a review July 9, 2023 23:31
Comment thread source/NVDAObjects/window/excel.py Outdated
@seanbudd seanbudd merged commit c4d63c9 into nvaccess:master Jul 9, 2023
@LeonarddeR LeonarddeR deleted the fixExcelFormatting branch August 23, 2025 06:27
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.

In Excel, cell format is reported on each move

3 participants