Skip to content

Fix braille word wrap#18219

Merged
seanbudd merged 12 commits intonvaccess:masterfrom
nvdaes:brailleWordWrap
Jul 1, 2025
Merged

Fix braille word wrap#18219
seanbudd merged 12 commits intonvaccess:masterfrom
nvdaes:brailleWordWrap

Conversation

@nvdaes
Copy link
Collaborator

@nvdaes nvdaes commented Jun 5, 2025

  • If braille word wrap is enabled, check if the first character of the next braille window is a space
  • Update changelog

Link to issue number:

Fixes #18016

Summary of the issue:

When braille Word wrap is enabled, NVDA doesn't check if the first character of the next braille window is a space, causing that in some cases NVDA Splits text between braille Windows instead of using all braille cells.

Description of user facing changes:

When possible, all braille cells Will be used even if word wrap option is enabled.

Description of developer facing changes:

None.

Description of development approach:

In source/braille.py, the _calculateWindowRowBufferOffsets() function has been modified to check if the next character is a space when Word wrap is True, so that all cells can be used in such cases.

Testing strategy:

In Notepad, type:

end end end end end end end end end send

Use a Focus 40 braille display to check that all cells are used, when Word wrap is enabled and disabled.

Known issues with pull request:

None.

Code Review Checklist:

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

@coderabbitai summary

@nvdaes nvdaes marked this pull request as ready for review June 5, 2025 20:19
@nvdaes nvdaes requested a review from a team as a code owner June 5, 2025 20:19
@nvdaes nvdaes requested a review from seanbudd June 5, 2025 20:19
@seanbudd
Copy link
Member

seanbudd commented Jun 5, 2025

Thanks @nvdaes - do you think you could add some unit tests for _calculateWindowRowBufferOffsets?

@seanbudd
Copy link
Member

We need to manually test this with multi-line displays before merging

@seanbudd seanbudd requested a review from SaschaCowley June 10, 2025 00:13
@nvdaes
Copy link
Collaborator Author

nvdaes commented Jun 11, 2025

Sean wrote:

We need to manually test this with multi-line displays before merging

Thanks. I don't have a multiline display.

@seanbudd
Copy link
Member

That's okay - I will test. Would you consider updating braille viewer to support adjusting the cell row/col count to create a multiline display? Might be good as a separate PR.

@nvdaes
Copy link
Collaborator Author

nvdaes commented Jun 11, 2025

Would you consider updating braille viewer to support adjusting the cell row/col count to create a multiline display?

Yes, thought I may need some visual feedback in the PR review.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jun 16, 2025
@michaelDCurran
Copy link
Member

Agreed that this change now means that all cells will now be used. But in reality, this just pushes the space on to the beginning of the next braille window. Thus wasting a space at the beginning instead.
For reading efficiency, I personally feel it is better to have extra space at the end of the display rather than at the beginning, so as to help the left hand always start at the beginning. However, it could also be argued that a space at the beginning is less ambiguous. Though I will admit I don't rely on braille anywhere as much as others here.
My understanding of visual word wrap (and perhaps a sighted person can confirm this) is that editors generally just remove the space at the end, and always avoid starting a new wrapped line with space.
However, they also place a hyphen at the end if forced to break a word, so as to differentiate between wrapping between two words (where space may be removed) verses breaking a word in the middle. But, in NVDA's case, we do not yet implement / have the ability to add a braille word continuation cell at the end of the line. This would be dependent on the braille table, and we would have to be sure not to place it in the middle of a contraction unit E.g. between the dot 5 and the s in "some".
So, to me this pr looks like it just provides an alternative, rather than a clear advantage. If the space at the beginning is more standard for Braille readers, then I'm good with that. What are your thoughts @SaschaCowley?

@nvdaes
Copy link
Collaborator Author

nvdaes commented Jun 19, 2025

Mick wrote:

But in reality, this just pushes the space on to the beginning of the next braille window.

I think that the approach introduced with this PR is less ambiguous than the current implementation, since if several cells are empty at the end of the display, we don't know if they correspond to spaces or are due to the wrap implementation.
For people proofreading texts and using routing keys, I think that it's better that an empty cell means a space when possible.
About reading efficiency, sometimes several spaces may be used wrongly, and it's better to read until the end of the line, at least when proofreading a text.

@RoryMichie
Copy link

I think the best solution is to remove the spaces entirely (that is, spaces should be neither at the beginning nor the end of the line). While this might not be in line with print standards, I think it's worth inventing a new one. My argument is that some people use very small Braille displays (sometimes with as few as fourteen cells); for this group especially, any wasted space could potentially both (a) slow reading down and (b) put unnecessary strain on the more frequently used cells.
Spaces are already removed thus within the onboard software suites of most Braile notetakers and displays I've used.

@RoryMichie
Copy link

I've had another idea, though. Perhaps a setting might be helpful. For people who are doing proofreading, they could see spaces as in the original print. For reading books or on smaller displays, one might prefer for them to be removed entirely.

@SaschaCowley
Copy link
Member

Hmm, a space at the start of a line feels wrong to me, and I think I'd be surprise to find one in physical braille. That being said, I understand the arguments regarding efficiency being made here, especially for very small displays.

I don't buy the argument about this being clearer for proofreading - how do you know that there aren't multiple spaces forcing the new wrapping behaviour? Why can't you use the routing keys/arrow keys to move the cursor and see if there are multiple spaces at the end of a line on the braille display?

That being said, I don't think this change makes things less clear, so I would be fairly comfortable merging it.

@nvdaes
Copy link
Collaborator Author

nvdaes commented Jun 20, 2025

Sascha wrote:

Why can't you use the routing keys/arrow keys to move the cursor and see if there are multiple spaces at the end of a line on the braille display?

You can do it, but this is not very efficient: if all cells aren't used, you have to press a routing key, at least, in the second empty cell, to see the cursor in the last empty cell and then press the scroll down button to go to the next row.
Then the cursor, at least in Notepad, is not shown and again a routing key has to be pressed.
Of course this new behavior should be documented.
Personally I prefer to use word wrap since hyphens aren't available, and really I think that it's a good idea to use all available cells.
A space is lkie other characters, so I don't see why would be more efficient to skip them at the start of a line. I think that this may be related to personal preferences, and to the way that we are used to see things, but imo, for example the feature to add two spaces as a paragraph start marker is nice and I use it.

Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Thanks, @nvdaes

@SaschaCowley
Copy link
Member

@nvdaes

I think that this may be related to personal preferences, and to the way that we are used to see things,

Yes, I agree. I guess I'm slightly concerned that this might be mistaken for a newline with an indent, but I think that is unlikely.

@nvdaes
Copy link
Collaborator Author

nvdaes commented Jun 20, 2025

Thank you @SaschaCowley . I didn't expect this 😀

@SaschaCowley
Copy link
Member

We're aiming to merge this early in the development cycle for 2025.3, so that if people seem to strongly dislike it, we can think about next steps before it hits a stable release.

@SaschaCowley SaschaCowley added the merge-early Merge Early in a developer cycle label Jun 20, 2025
@SaschaCowley SaschaCowley added this to the 2025.3 milestone Jun 20, 2025
@SaschaCowley SaschaCowley added the blocked/merge-conflicts Merge conflicts exist on this PR label Jun 20, 2025
@nvdaes
Copy link
Collaborator Author

nvdaes commented Jun 20, 2025

Sascha wrote:

if people seem to strongly dislike it, we can think about next steps

Perfect!

# Conflicts:
#	user_docs/en/changes.md
@seanbudd seanbudd merged commit 57c469b into nvaccess:master Jul 1, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked/merge-conflicts Merge conflicts exist on this PR blocked/needs-testing conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle

Projects

None yet

Development

Successfully merging this pull request may close these issues.

braille display line wrap algorithm assumes one cell less than is present on display

5 participants