No longer use a single semicolon as separator between title and message with ui.browseableMessage by XLTechie · Pull Request #14668 · nvaccess/nvda

XLTechie · 2023-02-23T04:51:02Z

Link to issue number:

Summary of the issue:

If ui.browseableMessage was given a title containing a semicolon (;), the title would be broken at the semicolon, and the rest of the title would be rendered before (as part of) the message in the HTML portion of the window.
This had the potential to result in unexpected outcomes, if the message content was HTML; not to mention being confusing for the user.

While this was noticed in relation to the Report link destination feature introduced by #14583, it did not result from that feature, and could have occurred prior to its introduction. It does however effect it significantly, as the title of the link the destination of which is being identified, is presented in the title of a ui.browseableMessage, if the command is called twice, or the alternate version of the command is mapped to a key.

Description of user facing changes

If a link name contains a semicolon, portions of that name will no longer appear along with the link address, when reporting link destination in its window version.

Description of development approach

Changed the regex separator string in ui.browseableMessage, and in the message.html file, to one which should hopefully be much less likely to appear in the wild. Instead of a single semicolon (";"), the string is now "__NVDA:split-here__".

Also removed the blank line from the top of message.html. I know of no purpose for having a blank line there.

Testing strategy:

Performed the STR in Report Link Destination partially broken for link titles with semicolons #14667, and confirmed that the title no longer breaks.
Tested some other cases of ui.browseableMessage, such as NVDA+f, to make sure they act as expected.

Known issues with pull request:

Change log entries:

This was introduced in beta. If fixed in beta, no user level change should need to be listed.

For Developers
Titles in ui.browseableMessage calls, can now include semicolons without side effects.

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
Security precautions taken.

…rowseableMessage, for less ambiguity when separating title from message. Used same regex split string in message.html, as now used in ui.browseableMessage Removed an apparently unnecessary blank line from the top of message.html

XLTechie · 2023-02-23T04:59:36Z

@seanbudd Two things:

I have targeted beta with this, because the feature which highlighted this problem was introduced there, and because:
Technically, I think this is API breaking. While highly unlikely, it is remotely possible that somebody somewhere was relying on a semicolon in the title of a ui.browseableMessage, to break the title and put part of it in the message itself. While that usage seems absurd and unintended, it's possible. Therefore, if NV Access considers this API breaking, we can only get it in beta, or wait until 2024.1 to fix this.

AppVeyorBot · 2023-02-23T05:38:53Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/04y2bwkcugos4s5v/artifacts/output/nvda_snapshot_pr14668-27744,6e7a3d18.exe
FAIL: System tests (tags: installer NVDA). See test results for more information.
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 20.6,
FINISH_END 0.2
PASS: Unit tests.
PASS: Translation comments check.
PASS: Lint check.

See test results for failed build of commit 6e7a3d185a

XLTechie · 2023-02-23T06:35:14Z

I'm reasonably sure none of these failed system tests result from this PR's changes.

CyrilleB79 · 2023-02-23T11:05:36Z

@XLTechie two points:

Using a very unlikely string as a separator, the code would not be waterproof to an hypothetical situation with a link containing the same string, e.g. reading NVDA's code itself where each line or variable/token would be a link. Have you investigated an alternative solution with encapsulation of each argument, e.g. with double quotes, and escaping or doubling the encapsulation character in the encapsulated content?
I do not think that this PR is considered API breaking change. Fixing obviously bugged functions (thus changing their result) has never been considered API-breaking and it does not make sense to me.

XLTechie · 2023-02-23T12:39:27Z

@CyrilleB79

1. Using a very unlikely string as a separator, the code would not be waterproof to an hypothetical situation with a link containing the same string, e.g.

It was far less waterproof with a simple semicolon.

reading NVDA's code itself where each line or variable/token would be a link.

Do you know any real world example of that that would apply here? It is a string used inside a variable, which is then passed through the mshtml library to a javascript function. Does anything really do HTML annotations of the contents of strings?

Have you investigated an alternative solution with encapsulation of each argument, e.g. with double quotes, and escaping or doubling the encapsulation character in the encapsulated content?

We would need a Python function which encodes (escapes) HTML within double quotes, and a javascript function which agrees with it on the other side to decode it back into a string which can be split unambiguously. I am attempting now to do it with JSON, as I would like something that allows individual properties without splitting. A possible alternate solution I have just thought of, is to prepend the title string with the length of the title string, then add the message. Split it with substring() on the javascript side. If I can't get it to accept JSON, I may try this.

2. I do not think that this PR is considered API braking change. Fixing obviously bugged functions (thus changing their result) has never been considered API-breaking and it does not make sense to me.

I tend to agree with you. However, I'm not NV Access. It is their consideration to make. It was not strictly a bug, just a bad implementation.

CyrilleB79 · 2023-02-23T20:21:36Z

@CyrilleB79

Using a very unlikely string as a separator, the code would not be waterproof to an hypothetical situation with a link containing the same string, e.g.
It was far less waterproof with a simple semicolon.

I fully agree and this PR as is is already a great improvement that should handle almost (if not all) the issues.

reading NVDA's code itself where each line or variable/token would be a link.
Do you know any real world example of that that would apply here? It is a string used inside a variable, which is then passed through the mshtml library to a javascript function.

Not at all. As written before, it was just an hypothetical example but not a know real-life case. I was imagining an online code browser where each line of code would be a link. The issue would then not be solved in case you browse the code of this very PR.

XLTechie · 2023-02-26T01:25:44Z

@CyrilleB79, in response to your comments, I wrote:

I am attempting now to do it with JSON, as I would like something that allows individual properties without splitting.

And the result, after much banging head against wall, is to learn that mshtml is running in compatibility mode, and therefore does not support the JSON object.
Thus, while it is very possible to pass a JSON string into message.html, JSON.parse does not exist to do anything with it.

Unless Microsoft is going to change mshtml as they remove IE from people's machines, we are left with few options but to either include a JSON library in message.html, or use another approach. I suspect NV Access would not want a third party JSON library included.

I have looked at passing other types than string through dialogArguments. While this seems possible (some of the automation languages do it), either the monkeypatching we do to the types we pass to COM interfaces doesn't allow this, or those languages are performing some magic that we don't, because I haven't found a way to make it happen.
I even tried passing a null separated set of strings (\0), but COM Interfaces doesn't like that at all.

The last two ideas I've had, are base64 encoding some kind of object and passing that; or the numeric offset method I described earlier. I would like to discover a robust way to pass an arbitrary number of isolated strings to the javascript, so that we have options for implementing features like those under discussion in #14641.

I will continue to play with this as I have time. Suggestions appreciated.

seanbudd added this to the 2023.1 milestone Feb 23, 2023

XLTechie marked this pull request as ready for review February 23, 2023 06:29

XLTechie requested a review from a team as a code owner February 23, 2023 06:29

XLTechie requested review from seanbudd and removed request for a team February 23, 2023 06:29

seanbudd approved these changes Feb 23, 2023

View reviewed changes

seanbudd merged commit 7f9aa01 into nvaccess:beta Feb 23, 2023

XLTechie deleted the fix14667 branch February 26, 2023 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

No longer use a single semicolon as separator between title and message with ui.browseableMessage#14668

No longer use a single semicolon as separator between title and message with ui.browseableMessage#14668
seanbudd merged 1 commit into
nvaccess:betafrom
XLTechie:fix14667

XLTechie commented Feb 23, 2023 •

edited

Loading

Uh oh!

XLTechie commented Feb 23, 2023

Uh oh!

AppVeyorBot commented Feb 23, 2023

Uh oh!

XLTechie commented Feb 23, 2023

Uh oh!

CyrilleB79 commented Feb 23, 2023

Uh oh!

XLTechie commented Feb 23, 2023 via email •

edited

Loading

Uh oh!

CyrilleB79 commented Feb 23, 2023

Uh oh!

XLTechie commented Feb 26, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

XLTechie commented Feb 23, 2023 • 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 user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

XLTechie commented Feb 23, 2023

Uh oh!

AppVeyorBot commented Feb 23, 2023

Uh oh!

XLTechie commented Feb 23, 2023

Uh oh!

CyrilleB79 commented Feb 23, 2023

Uh oh!

XLTechie commented Feb 23, 2023 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrilleB79 commented Feb 23, 2023

Uh oh!

XLTechie commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

XLTechie commented Feb 23, 2023 •

edited

Loading

XLTechie commented Feb 23, 2023 via email •

edited

Loading

XLTechie commented Feb 26, 2023 •

edited

Loading