Skip to content

Added a margin to the browseable message.#13885

Merged
seanbudd merged 1 commit into
nvaccess:masterfrom
CyrilleB79:browseableMessage
Jul 12, 2022
Merged

Added a margin to the browseable message.#13885
seanbudd merged 1 commit into
nvaccess:masterfrom
CyrilleB79:browseableMessage

Conversation

@CyrilleB79

@CyrilleB79 CyrilleB79 commented Jul 9, 2022

Copy link
Copy Markdown
Contributor

Link to issue number:

None

Summary of the issue:

The ui.browseableMessage has no margin. For people with visual difficulties, it makes more difficult to isolate the content of the browseable message from the text present in the background around the browseable message.

Description of user facing changes

The content of browseable message such as formatting information (double press of NVDA+F) is now rendered with a margin around the text.

Description of development approach

Just added the following style property to the message's body:
style="margin:1em"
The 1em measurement should correspond to 1 character.

Note for reviewers:
I know almost nothing in CSS and styles. Thus feel free to suggest something else if the way the margin is implemented is not the best solution.

Testing strategy:

  • Checked visually the left and top margin of the text on a formatting information message.

Before (NVDA 2022.2beta3):
Before

After (with this PR):
After

Known issues with pull request:

The right margin does not seem to appear. Maybe due to the <pre> tag in the ui.browseableMessage function?
Anyway, the most important margin is the left one, at least for left-aligned LTR text; I guess that all is reverse for RTL text.

Change log entries:

Not needed.

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

@seanbudd

Copy link
Copy Markdown
Member

When this is ready, would you be able to add screenshots of the change in appearance?
If this is difficult, I am happy to add them before merging.

@CyrilleB79

Copy link
Copy Markdown
Contributor Author

When this is ready, would you be able to add screenshots of the change in appearance?

Done.
Please double-check since I may miss very obvious things with my low vision.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review July 11, 2022 21:28
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner July 11, 2022 21:28
@CyrilleB79 CyrilleB79 requested a review from seanbudd July 11, 2022 21:28
@seanbudd

Copy link
Copy Markdown
Member

Thanks @CyrilleB79 - the screenshots look good

@seanbudd

Copy link
Copy Markdown
Member

I tested this using Arabic as an RTL language, it appears that browseableMessage does not respect RTL layout.

@seanbudd seanbudd merged commit e5ec5b7 into nvaccess:master Jul 12, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jul 12, 2022
@CyrilleB79 CyrilleB79 deleted the browseableMessage branch July 12, 2022 21:52
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.

3 participants