Skip to content

Ensure that NVDA no longer checks validity of input for non contracted braille tables when in text controls#12850

Merged
seanbudd merged 4 commits into
nvaccess:masterfrom
lukaszgo1:I12667
Sep 23, 2021
Merged

Ensure that NVDA no longer checks validity of input for non contracted braille tables when in text controls#12850
seanbudd merged 4 commits into
nvaccess:masterfrom
lukaszgo1:I12667

Conversation

@lukaszgo1
Copy link
Copy Markdown
Contributor

@lukaszgo1 lukaszgo1 commented Sep 15, 2021

Link to issue number:

Fixes #12667

Summary of the issue:

When entering text from a braille display NVDA reports that it is unsupported even though entered combination is valid. This check has been introduced in #7843 to prevent a situation in which someone uses contracted table outside text content and enters a combination which cannot be converted to a single character (a good example is entering dots 356 using English UK Grade 2 which is a contraction for "was"). The check however was too broad and didn't take into account that for literary / computer braille tables single combination of dots can either always be converted to a single character, or if not it does not result in multiple characters i.e. number sign.

Description of how this pull request fixes the issue:

Now the check applies only when the table in use is contracted.

Testing strategy:

  • With English UK Grade 2 entered dots 356 in the text editor - ensured that after pressing space dots are translated to was.
  • Repeated this test in the web browser - made sure that NVDA reports unsupported input.
  • With the same table as above entered dots 125 - ensured that in the text editor the combination was back-translated to 'have' whereas in the web browser NVDA navigated to the next heading.
  • Repeated tests from Braille input with NVDA isn't always correct #12667 - made sure that no 'unsupported input' was reported
  • With Polish Grade 1 table entered dots 3456-12-46-1 - ensured that this combination was properly back-translated to 2A
  • Got confirmation on Braille input with NVDA isn't always correct #12667 from @Futyn-Maker that the issue is fixed for them.

Known issues with pull request:

  • brailleInput is in dire need of unit tests so that future work in this area is safer - I've created braileInput module needs a unit test coverage. #12869 to track this
  • According to @AAClause there are some problems with input for literary tables - they haven't provided more details as of yet other than the fact that problem has not been introduced by this pr.

Change log entries:

Bug fixes

  • Input with literary braille tables should behave more reliably when in edit fields.

Code Review Checklist:

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

@lukaszgo1
Copy link
Copy Markdown
Contributor Author

@LeonarddeR Even though this is still a draft I'll appreciate a look from you if time permits since you've introduced this code in the first place and you're much more familiar with the brailleInput code.

@LeonarddeR
Copy link
Copy Markdown
Collaborator

I think we really need to unit test this somehow. There are simply too much edge cases.

@AAClause
Copy link
Copy Markdown
Contributor

AAClause commented Sep 15, 2021

@lukaszgo1 I had made the same patch a short time ago for testing. It seems to me that it broke the good behavior of grade 1 tables.
Also, some inputs with these tables don't work. For example, with "en-us-g1.ctb" (English (U.S.) grade 1) with the input ⠼⠉⠠⠙.
Sorry, i only read the diff, not tested the build for now.

@lukaszgo1
Copy link
Copy Markdown
Contributor Author

@lukaszgo1 I had made the same patch a short time ago for testing. It seems to me that it broke the good behavior of grade 1 tables.

Would you be able to provide an example of input which worked before this patch and now is broken?

Also, some inputs with these tables don't work. For example, with "en-us-g1.ctb" (English (U.S.) grade 1) with the input ⠼⠉⠠⠙.

With the code from this PR entering the above combination results in 3D which is I believe an expected input.

@AAClause
Copy link
Copy Markdown
Contributor

@lukaszgo1 Please forget my previous comment. Indeed, your PR fixes the issue for contacted tables. Just that the issue still remains with literary tables, but no regression introduced. Sorry for the noise.

@lukaszgo1
Copy link
Copy Markdown
Contributor Author

@lukaszgo1 Please forget my previous comment. Indeed, your PR fixes the issue for contacted tables. Just that the issue still remains with literary tables, but no regression introduced. Sorry for the noise.

Would you be able to give an STR for an issue with literary tables so that it can be fixed and / or unit test created for this?

@lukaszgo1
Copy link
Copy Markdown
Contributor Author

@AAClause Have you seen my latest comment? It would be nice to have a STR reproducing your issue as that is the only thing getting this blocked from being converted from a draft.

@lukaszgo1 lukaszgo1 marked this pull request as ready for review September 22, 2021 11:03
@lukaszgo1 lukaszgo1 requested a review from a team as a code owner September 22, 2021 11:03
@lukaszgo1 lukaszgo1 requested a review from seanbudd September 22, 2021 11:03
@lukaszgo1
Copy link
Copy Markdown
Contributor Author

@LeonarddeR While I agree with your opinion that unit tests for brailleInput are very important to have I'm afraid I would not have time to work on them before the release and this PR improves the known bad cases,. I've created #12869 and would be happy to work on this subsequently. Would you be okay with this being reviewed as is and getting unit test at a later point

@LeonarddeR
Copy link
Copy Markdown
Collaborator

I guess NV Access will have to decide about that.

Copy link
Copy Markdown
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @lukaszgo1

@seanbudd seanbudd merged commit a4c9eee into nvaccess:master Sep 23, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Sep 23, 2021
@lukaszgo1 lukaszgo1 deleted the I12667 branch September 23, 2021 09:45
AAClause added a commit to AAClause/BrailleExtender that referenced this pull request Sep 23, 2021
…onger checks validity of input for non contracted braille tables when in text controls —

See <nvaccess/nvda#12850>
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.

Braille input with NVDA isn't always correct

5 participants