Skip to content

Only remove XML tags if JAB widget's text starts with <html> tag#13531

Merged
michaelDCurran merged 7 commits into
nvaccess:masterfrom
mwhapples:jabHTML
Mar 27, 2022
Merged

Only remove XML tags if JAB widget's text starts with <html> tag#13531
michaelDCurran merged 7 commits into
nvaccess:masterfrom
mwhapples:jabHTML

Conversation

@mwhapples

@mwhapples mwhapples commented Mar 24, 2022

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #13102

Summary of the issue:

NVDA was incorrectly stripping text from Java widgets if the text looked like an XML tag. According to the Oracle Java docs, when using HTML in a Java widget you should start the text with the <html> tag.

Description of how this pull request fixes the issue:

This change gets NVDA to check if the text starts with <html> and only then will remove any text which appears to be XML tags.

Testing strategy:

Manually tested, some unit tests.

Known issues with pull request:

I haven't checked exactly what the regular expression does, but I suspect it does not handle entity substitution (eg. &lt; with < and such like). Ideally this should also be fixed, but we may want to track that as a separate issue as its something slightly different to the original issue this is designed to fix.

Change log entries:

New features
Changes
Bug fixes
Fix issue where NVDA may incorrectly remove text from Java widgets when presenting to the user.
For Developers

Code Review Checklist:

  • Pull Request description:
    • description is up to date Yes
    • change log entries Done
  • Testing:
    • Unit tests Unit tests for the function which detects and removes HTML.
    • System (end to end) tests Not done, unsure how to do for this issue.
    • Manual testing Yes
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation N/A
    • Developer / Technical Documentation N/A
    • Context sensitive help for GUI changes N/A
  • UX of all users considered:
    • Speech Yes
    • Braille Yes
    • Low Vision Yes
    • Different web browsers N/A
    • Localization in other languages / culture than English N/A

@mwhapples mwhapples requested a review from a team as a code owner March 24, 2022 17:03
@mwhapples mwhapples requested a review from seanbudd March 24, 2022 17:03
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 568b2858c3

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The actual change looks fine to me.
But, Some unit tests would be great for testing _processHTML. Especially a unit test at very least for the label content in the issue. And perhaps a plain text label, and of course a label with valid html.
Add a new file to tests/unit
Name it something like test_javaAccessBridge.py
You could use a file like test_winVersion.py as an example, so you can see how to write the test class and methods.
Then you can run rununittests and ensure all of them pass.

@mwhapples

Copy link
Copy Markdown
Contributor Author

I am on creating a test for this now. In fact I realised that by refactoring out to _processHTML would help me test it as it separates the logic for detecting and removing HTML tags from the accessibility API stuff. Also testing this in the future would be more important should we do things like replacing character entities and such like as the code could become more involved.

@mwhapples

Copy link
Copy Markdown
Contributor Author

Tests now written.

Comment thread tests/unit/test_javaAccessBridge.py Outdated

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work, thanks :)

@michaelDCurran michaelDCurran merged commit 153f500 into nvaccess:master Mar 27, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 27, 2022
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.

NVDA seems to apply some kind of extra processing on label text before reading it

4 participants