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

Fix-up of 12814: Ensure that role label for landmark is once again abbreviated in braille #13160

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

lukaszgo1
Copy link
Contributor

@lukaszgo1 lukaszgo1 commented Dec 12, 2021

Opened against beta since this PR fixes regression introduced in 2021.3 development cycle and including in a point release seems very low risk.

Link to issue number:

Fixes #13158

Summary of the issue:

PR #12814 replaced usages of deprecated controlTypes.roleLabels with role.displayString. However braille has its own mapping of roles to labels which should be used for abbreviations of roles for braille. The role label used for speech has been mistakenly used for a role of landmark.

Description of how this pull request fixes the issue:

Once again uses braille abbreviation of landmark for landmarks.

Testing strategy:

Performed STR from #13158 - made sure that 'lmk' is shown on a braille display for landmarks.

Known issues with pull request:

None known

Change log entries:

Bug fixes
- Landmark is once again abbreviated in braille (#13158)

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 lukaszgo1 requested a review from a team as a code owner December 12, 2021 17:25
@lukaszgo1 lukaszgo1 requested review from feerrenrut and removed request for a team December 12, 2021 17:25
@lukaszgo1
Copy link
Contributor Author

cc @feerrenrut

feerrenrut
feerrenrut previously approved these changes Dec 13, 2021
@feerrenrut feerrenrut merged commit ded5ef5 into nvaccess:beta Dec 13, 2021
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Dec 13, 2021
@lukaszgo1 lukaszgo1 deleted the I13158 branch December 13, 2021 10:22
@feerrenrut feerrenrut modified the milestones: 2022.1, 2021.3.1 Dec 14, 2021
feerrenrut pushed a commit that referenced this pull request Dec 14, 2021
…lle (PR #13160)

Fixes #13158

PR #12814 replaced usages of deprecated controlTypes.roleLabels with role.displayString. However braille has its own mapping of roles to labels which should be used for abbreviations of roles for braille. The role label used for speech has been mistakenly used for a role of landmark.
@feerrenrut
Copy link
Contributor

Please test and confirm this is fixed in the RC for 2021.3.1 https://www.nvaccess.org/post/nvda-2021-3-1rc1/

@CyrilleB79
Copy link
Collaborator

RC is already available. Does it mean that the translated change log will not be included in 2021.3.1, i.e. all languages except English will not have any 2021.3.1 section in the change log?
Or 2021.3.1rc1 willl differ from 2021.3.1? (on the contrary to usual rc that are identical to final releases if no issue is found)
Thanks.

@lukaszgo1
Copy link
Contributor Author

I can confirm the issue is fixed in 2021.3.1 RC1. Are you planning to merge RC to master?

@feerrenrut
Copy link
Contributor

@CyrilleB79 Historically we haven't allowed translation updates into point releases, there is some risk that translation updates can break NVDA. Ideally we would allow updates ONLY to the changes files, we don't have that ability at this stage.

We prefer not to have a long RC period for point releases, just confirm that the addressed issues are fixed, and do the release.

We'll be discussing this internally to try to improve the process in the future, however this will likely rely on updates to our translation system.

@CyrilleB79
Copy link
Collaborator

OK @feerrenrut no problem.

It seemed to me that all the PRs for 2021.3.1 were opened and merged on top of beta branch, not rc.
And in #13154 (comment), you wrote:

A point release is required to address #13153. This will require updates from translators, and confirmation from affected individuals that this issue is resolved.

That's why I have thought that the change log content would have been translated.

Anyway, everything is clarified now. I understand why new translations will not be integrated in 2021.3.1 and agree with this process. Thanks.
We will send the translated change log for 2021.3.1 by e-mail in local communities.

@feerrenrut
Copy link
Contributor

Yes, that was the initial plan and so we merged the changes to beta. I cherry picked the changes into the RC branch to prevent any risk from updated translations and made the RC from there. Sorry for the confusion this caused.

@feerrenrut feerrenrut mentioned this pull request Dec 15, 2021
5 tasks
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