Make font size measurements translatable by seanbudd · Pull Request #13575 · nvaccess/nvda

seanbudd · 2022-04-04T05:19:14Z

Link to issue number:

Summary of the issue:

"pt" when reporting font size measurements is not translatable.
Font size measurements are reported inconsistently, for example in excel and PowerPoint are reported as "11.0" instead of "11 pt".

Description of how this pull request fixes the issue:

Makes the font size measurement "pt" translatable across APIs.

Browsers provide "font-size" attribute directly as "12pt|px|em|%|rem|ex" or "large", "small", etc.
This requires a more strategic approach for catching each font-size case and replacing it with a translated string.
Documentation has been added for this.
https://developer.mozilla.org/en-US/docs/Web/CSS/font-size

Testing strategy:

Unit testing has been added to test the more complicated parsing of the browser font-size attribute.

Manual testing

Enable "font size" reporting in "Document Formatting".

Added a translation to /de/nvda.po.

msgctxt "font size"
msgid "%s pt"
msgstr "%s Punkt"

Switched NVDA language to German.
Confirm that font-size is reported as "xx Punkt" when navigating text in:
- adobe reader (unable to test due to x64 bug, 774f1ea has been tested)
- word (with and without UIA)
- excel (with and without UIA)
- powerpoint
- Wordpad
- Libre Office Writer (soffice)
- Notepad++ (scintilla)
- JAB (unable to test, unknown relevant software)
- MSHTML (tested with IE mode for Edge on Win 11)
- IAccessible (Chrome, Firefox)

Known issues with pull request:

None

Change log entries:

Changes
- Font size measurements are now translatable in NVDA. (#13573)

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

lukaszgo1

This fix is incomplete as it affects only UIA controls. The same should be done for all other places where NVDA can report font size.

michaelDCurran

Okay. If I ignore the line ending changes, everything looks good to me.

AppVeyorBot · 2022-04-06T04:28:05Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/4aq86aeg451ojgoc/artifacts/output/nvda_snapshot_pr13575-25171,c6eb1739.exe

See test results for failed build of commit c6eb17399b

CyrilleB79 · 2022-04-06T06:32:36Z

Regarding tests, all the modified code paths are not tested. It would be interesting to test them (and indicate it) or at least indicate that they are not tested (and why). Exemples of untested code paths:

edit (e.g. Wordpad & Wordpad + trick)
soffice (e.g. Libre Office Writer)
JAB
Not tested, but the font size in browsers does not seem to be translated (MSHtml, IAccessible).
For a more exhaustive of all the interface that should be considered and tested, please look at Do not report "baseline" on formatting information request #11897's initial description (test paragraph).

AppVeyorBot · 2022-06-15T08:43:20Z

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/jeqnam1hnffs1lwk/artifacts/output/nvda_snapshot_pr13575-25589,930ca8d3.exe

See test results for failed build of commit 930ca8d353

seanbudd · 2022-06-16T04:06:49Z

Thanks @CyrilleB79 - I've fixed this up now. The case for browsers is quite complex, a start for covering each font-size case for browser has been added to this PR.

feerrenrut

Generally looks good (assuming actions from your self review are committed).

You might consider adding an integration test with chrome to ensure that the font-size is reported correctly end-to-end.

CyrilleB79 · 2022-06-16T16:35:43Z

@seanbudd, @lukaszgo1 I feel a bit picky with such remark but prefer to have things explicit.

Technically, do we consider that removing imports (or converting star imports in explicit imports) are an API change?
Or do only the variable / class / function directly defined in a module belong to its API (off course if they are not defined as private, i.e. starting with underscore)?

Thanks for clarification.

lukaszgo1 · 2022-06-16T17:56:35Z

I feel a bit picky with such remark but prefer to have things explicit.

Technically, do we consider that removing imports (or converting star imports in explicit imports) are an API change? Or do only the variable / class / function directly defined in a module belong to its API (off course if they are not defined as private, i.e. starting with underscore)?

Unused imports were removed in various PR's for non compatibility breaking releases, so IMO these are not a part of public API. Removing star imports is just another case of removing unused imports, and should not be treated differently. For one example where similar cleanup was performed in non compat breaking release see #12924.

AppVeyorBot · 2022-06-17T06:24:37Z

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/gt9pj8k1g3hv3n2a/artifacts/output/nvda_snapshot_pr13575-25609,49766517.exe

See test results for failed build of commit 497665179d

Make font size pt translatable

8710bac

seanbudd requested a review from a team as a code owner April 4, 2022 05:19

seanbudd requested a review from michaelDCurran April 4, 2022 05:19

This comment was marked as off-topic.

Sign in to view

michaelDCurran approved these changes Apr 5, 2022

View reviewed changes

lukaszgo1 suggested changes Apr 5, 2022

View reviewed changes

make pt translatable in all contexts

774f1ea

seanbudd requested a review from michaelDCurran April 6, 2022 03:29

michaelDCurran reviewed Apr 6, 2022

View reviewed changes

Comment thread source/appModules/powerpnt.py

michaelDCurran approved these changes Apr 6, 2022

View reviewed changes

CyrilleB79 reviewed Apr 6, 2022

View reviewed changes

Comment thread source/NVDAObjects/window/excel.py Outdated

seanbudd marked this pull request as draft April 7, 2022 23:20

seanbudd added 2 commits June 15, 2022 16:45

Merge branch 'master' into fix-13573

a730104

add missing translator comments

839df28

support handling browser font size, group font sizes

4792784

seanbudd changed the title ~~Make font size pt translatable~~ Make font size measurements translatable Jun 16, 2022

add unit tests

72cca22

seanbudd marked this pull request as ready for review June 16, 2022 04:03

seanbudd requested a review from feerrenrut June 16, 2022 04:03

seanbudd commented Jun 16, 2022

View reviewed changes

Comment thread nvdaHelper/remote/displayModel.cpp Outdated

seanbudd commented Jun 16, 2022

View reviewed changes

Comment thread source/controlTypes/formatFields.py Outdated

feerrenrut approved these changes Jun 16, 2022

View reviewed changes

lukaszgo1 mentioned this pull request Jun 16, 2022

No longer rely on star imports in NVDAObjects.IAccessible.winword #13805

Merged

5 tasks

lukaszgo1 reviewed Jun 16, 2022

View reviewed changes

Comment thread source/NVDAObjects/window/winword.py

seanbudd added 3 commits June 17, 2022 14:25

Apply suggestions from code review

b85449c

Merge remote-tracking branch 'origin/master' into fix-13573

a948cb9

update changes

6a2f753

seanbudd merged commit 6f211a1 into master Jun 17, 2022

seanbudd deleted the fix-13573 branch June 17, 2022 08:20

nvaccessAuto added this to the 2022.3 milestone Jun 17, 2022

seanbudd mentioned this pull request Jan 17, 2023

Make reporting MS Word UIA indent size consistent, use pt #14553

Closed

6 tasks

Uh oh!

Conversation

seanbudd commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

This comment was marked as off-topic.

This comment was marked as off-topic.

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Apr 6, 2022

Uh oh!

CyrilleB79 commented Apr 6, 2022

Uh oh!

Uh oh!

AppVeyorBot commented Jun 15, 2022

Uh oh!

seanbudd commented Jun 16, 2022

Uh oh!

Uh oh!

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrilleB79 commented Jun 16, 2022

Uh oh!

lukaszgo1 commented Jun 16, 2022

Uh oh!

AppVeyorBot commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

seanbudd commented Apr 4, 2022 •

edited

Loading