Descend into children for dialog text for option pane (Fixes #11687) by michaelweghorn · Pull Request #12801 · nvaccess/nvda

michaelweghorn · 2021-09-02T11:37:54Z

Link to issue number:

Summary of the issue:

Text in LibreOffice message dialogs is not announced by NVDA.

Description of how this pull request fixes the issue:

This commit makes sure that the children of objects of role 'controlTypes.Role.OPTIONPANE' are taken into account when determining the text to announce for a dialog, as is done for objects of role 'controlTypes.Role.PANE'.

For LibreOffice (message) dialogs, the dialog text are objects of role 'controlTypes.Role.STATICTEXT' that are children of an object of type 'controlTypes.Role.OPTIONPANE'.
Those were previously not considered when determining the dialog text to announce. This change ensures that they are now and that the actual dialog text is thus announced by NVDA, just as is done (and has been the case) for objects of role 'controlTypes.Role.PANE'.

Note: In addition to this change in NVDA, it also requires
the following commit recently committed to LibreOffice [1]
(which will be contained in LibreOffice 7.3):

commit e29f80efc826fdc8a8e4b7cbdd3becf2f074cce4
Author: Michael Weghorn <m.weghorn@posteo.de>
Date:   Wed Sep 1 16:25:01 2021 +0200

    tdf#135588 wina11y: Map AccessibleRole::STATIC to ROLE_SYSTEM_STATICTEXT

Its commit message has some more background information.

[1] https://git.libreoffice.org/core/+/e29f80efc826fdc8a8e4b7cbdd3becf2f074cce4%5E%21

Testing strategy:

download and set up a recent LibreOffice daily build from https://dev-builds.libreoffice.org/daily/master/current.html (needs to be from 2021-09-02 or newer, since the above mentioned LibreOffice commit is needed as well)
start NVDA and LibreOffice Writer
type some text
press the "Close" button in LibreOffice
check that the Text in the "Save document?" dialog is announced completely
kill the LibreOffice process using Task manager
start LibreOffice again
follow description in LibreOffice Writer: in Confirm dialog in Document restore is not announced when using screenreaders #11687 and ensure that the text in the document recovery dialog is announced

Known issues with pull request:

none

Change log entries:

Changes
Dialog text from an object of role 'OPTIONPANE' is now retrieved by descending into the objects children. (#11687)

Code Review Checklist:

Pull Request description is up to date.
Unit tests.
System (end to end) tests.
Manual testing.
User Documentation.
Change log entry.
Context sensitive help for GUI changes.
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers

michaelweghorn · 2021-09-02T11:44:11Z

This is the first time I'm trying to contribute to NVDA. I went through the documentation for contributing. For those items I haven't marked checked in the code review checklist, it's unclear to me whether those are relevant in this case, so please let me know in case anything is missing.

AppVeyorBot · 2021-09-02T11:55:05Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/xhvpgbfpibrfkyek/artifacts/output/nvda_snapshot_pr12801-23669,1283bb46.exe

See test results for failed build of commit 1283bb46de

michaelweghorn · 2021-09-02T12:00:56Z

From what I can see, the failed flake8 check in the test is unrelated to my change, since the added line uses the same indentation as the following lines:

source/NVDAObjects/behaviors.py:92:6: ET126 (flake8-tabs) unexpected number of tabs at start of expression line (expected 4, got 5)
controlTypes.ROLE_OPTIONPANE,
^
1 ET126 (flake8-tabs) unexpected number of tabs at start of expression line (expected 4, got 5)

Should I adapt indentation for all lines in the commit or add a preparatory commit to change the indentation of the existing code and a separate commit to add the new line (to keep the diff in the "actual change" as small as possible)?

CyrilleB79 · 2021-09-02T13:37:16Z

Even if fixing only the tab number of the line you added should be enough to make the linting check pass, this makes sense to fix all the tabs for all the elements of the tuple to which you have added an element.

This addresses a flake8 warning: > 1 ET126 (flake8-tabs) unexpected number of tabs at start of expression line (expected 4, got 5) into which I ran when adding another element to the tuple in a follow-up change.

As is done for objects of role 'controlTypes.Role.PANE' also descend into children when retrieving the dialog text from an object of role 'controlTypes.Role.OPTIONPANE'. This is used e.g. in LibreOffice's message dialogs and makes sure that the text in the dialogs is read out. Note: In addition to this change in NVDA, it also requires the following commit recently committed to LibreOffice [1] (which will be contained in LibreOffice 7.3) commit e29f80efc826fdc8a8e4b7cbdd3becf2f074cce4 Author: Michael Weghorn <m.weghorn@posteo.de> Date: Wed Sep 1 16:25:01 2021 +0200 tdf#135588 wina11y: Map AccessibleRole::STATIC to ROLE_SYSTEM_STATICTEXT Its commit message has some more background information. Fixes nvaccess#11687 [1] https://git.libreoffice.org/core/+/e29f80efc826fdc8a8e4b7cbdd3becf2f074cce4%5E%21

michaelweghorn · 2021-09-02T13:57:44Z

Even if fixing only the tab number of the line you added should be enough to make the linting check pass, this makes sense to fix all the tabs for all the elements of the tuple to which you have added an element.

Thanks, I have added a preparatory commit to fix all tabs for the tuple before adding the new element in a separate commit.

seanbudd · 2021-09-02T22:51:19Z

This looks ready for review. Considering that it would require a specific application (like Libre Office) to perform system tests, this can be ignored. The other unticked checkboxes are also not necessary for this sort of change.

michaelweghorn · 2021-09-03T06:18:39Z

Thanks, I've marked this as ready for review now.

LeonarddeR

Congrats with your first pr! Looks good to me.

michaelweghorn · 2021-09-11T19:44:59Z

Thanks! Is any further action needed from my side or should I just wait for the remaining missing reviews?

AppVeyorBot · 2021-09-13T23:51:57Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/kwj2mkr0to9jt4l7/artifacts/output/nvda_snapshot_pr12801-23743,9e0602e0.exe

See test results for failed build of commit 9e0602e08a

michaelweghorn mentioned this pull request Sep 2, 2021

LibreOffice Writer: in Confirm dialog in Document restore is not announced when using screenreaders #11687

Closed

michaelweghorn added 2 commits September 2, 2021 14:50

flake8: Avoider over-indent

e253a23

This addresses a flake8 warning: > 1 ET126 (flake8-tabs) unexpected number of tabs at start of expression line (expected 4, got 5) into which I ran when adding another element to the tuple in a follow-up change.

michaelweghorn force-pushed the michaelweghorn/tdf135588_nvda11687 branch from ecfb714 to de162c9 Compare September 2, 2021 13:55

michaelweghorn marked this pull request as ready for review September 3, 2021 06:17

michaelweghorn requested a review from a team as a code owner September 3, 2021 06:17

michaelweghorn requested a review from michaelDCurran September 3, 2021 06:17

LeonarddeR approved these changes Sep 9, 2021

View reviewed changes

michaelDCurran added 2 commits September 14, 2021 09:32

Merge branch 'master' into michaelweghorn/tdf135588_nvda11687

9c47781

Update what's new

fcb324c

michaelDCurran approved these changes Sep 13, 2021

View reviewed changes

michaelDCurran merged commit efb6141 into nvaccess:master Sep 14, 2021

nvaccessAuto added this to the 2021.3 milestone Sep 14, 2021

lukaszgo1 mentioned this pull request Sep 14, 2021

Fix-up of PR 12801 - use enums in controlTypes replacing old constants #12844

Merged

9 tasks

michaelweghorn deleted the michaelweghorn/tdf135588_nvda11687 branch September 17, 2021 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Descend into children for dialog text for option pane (Fixes #11687)#12801

Descend into children for dialog text for option pane (Fixes #11687)#12801
michaelDCurran merged 4 commits into
nvaccess:masterfrom
michaelweghorn:michaelweghorn/tdf135588_nvda11687

michaelweghorn commented Sep 2, 2021

Uh oh!

michaelweghorn commented Sep 2, 2021

Uh oh!

AppVeyorBot commented Sep 2, 2021

Uh oh!

michaelweghorn commented Sep 2, 2021

Uh oh!

CyrilleB79 commented Sep 2, 2021

Uh oh!

michaelweghorn commented Sep 2, 2021

Uh oh!

seanbudd commented Sep 2, 2021 •

edited

Loading

Uh oh!

michaelweghorn commented Sep 3, 2021

Uh oh!

LeonarddeR left a comment

Uh oh!

michaelweghorn commented Sep 11, 2021

Uh oh!

AppVeyorBot commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

michaelweghorn commented Sep 2, 2021

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

michaelweghorn commented Sep 2, 2021

Uh oh!

AppVeyorBot commented Sep 2, 2021

Uh oh!

michaelweghorn commented Sep 2, 2021

Uh oh!

CyrilleB79 commented Sep 2, 2021

Uh oh!

michaelweghorn commented Sep 2, 2021

Uh oh!

seanbudd commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelweghorn commented Sep 3, 2021

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

michaelweghorn commented Sep 11, 2021

Uh oh!

AppVeyorBot commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

seanbudd commented Sep 2, 2021 •

edited

Loading