Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose transparent background colors for displayModel #12658

Merged
merged 15 commits into from
Jul 27, 2021
Merged

Conversation

feerrenrut
Copy link
Contributor

Link to issue number:

Resolves concerns about contributed PR: #9197
Workaround for #6467

Summary of the issue:

From #6467 (comment)

When text is written with the GDI background mode set to transparent, the display model doesn't necessary report the correct background color for that text. In this case, the GDI hook should look at the actual background color before the text is drawn, rather than using the result of GetBkColor.

Description of how this pull request fixes the issue:

There have been two prior attempts to fix this, both causing severe performance regressions.

This implementation merely exposes information about the potential transparency of the text background.
Testing with various GDI dependent GUI APIs shows that the GetBkColor result can be:

  • Opaque and matches visual presentation(rawGDI)
  • Transparent and matches visual presentation: (mfcTest, GDItest)
  • Transparent and does not match visual presentation: (annecdotal)

The exposure of the transparency (via the alpha value on the colors.RGB class) will allow appModules
or add-ons to customize NVDA's presentation as necessary.

For users, there is no change in behavior by default. An option is introcuded to the advanced settings panel to allow reporting of transparency in colors. The intention here is that developers will use this to identify where
color reporting could be improved.

The prior approach used the colorref high-level byte of a colorref struct. However, MSDN requires this byte be zero.
This prior approach did not pass the colorref into any Windows API's, so it was safe. But to prevent this from being accidentally introduced in the future, a compatible alternative type is introduced and used instead.

In order to stay compatible with python code interpretting this color value, a single bit is set if the color is considered transparent.

Testing strategy:

  • Built applications from https://github.com/nvaccess/testDisplayModel
  • Tested with/without transparency reporting enabled (advanced panel)
    • Ensure color reporting is enabled in document formatting panel of NVDA settings
    • Use NVDA+Numpad 7 to switch to screen review
    • Use Numpad 7, Numpad 8, Numpad 9 to read the dialog

Known issues with pull request:

None

Change log entries:

For Developers

- Transparency of text background color, sourced from GDI applications (via the display model) are now exposed for add-ons or appModules.

Code Review Checklist:

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

@AppVeyorBot

This comment has been minimized.

Copy link
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.

These changes look good to me, this is a great change which might be able to be leveraged in #12372

Would it be too difficult to add system tests here considering the need for an external app (that was used for manual testing)?

Also can you explain why it is okay to remove all the free statements here? I'm guessing it is to do with how the construction was changed but looking to confirm.

devDocs/displayModel.md Show resolved Hide resolved
nvdaHelper/remote/gdiHooks.cpp Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Contributor Author

Also can you explain why it is okay to remove all the free statements here? I'm guessing it is to do with how the construction was changed but looking to confirm.

In C calloc allocates memory (much like new or new[] in C++), it must be matched with a call to free (in C++ delete or delete[]) to prevent a memory leak. In complicated code (and even in simple code) free / delete can be missed.

To replace these I tracked the variable being freed back to where the call to calloc was made, and checked if it was really necessary to allocate memory. In all cases I could use std::string or std::vector, these standard library containers manage their own memory. As a benefit, this change can also reduce the number of copies made, since the std::string or std::vector can be filled directly.

Since C++11 the memory of std::string has been guaranteed to be contiguous, this was already the case with std::vector. As of C++17, the 'data' method on a non-const std::string returns a non-const pointer to its internal buffer. The caveat for this is that there are a variety of ways to invalidate the pointer, anything that might cause the container to reallocate and move its contents to a new memory location. Once the data is filled however, the pointer can be discarded and member functions used again.

Would it be too difficult to add system tests here considering the need for an external app (that was used for manual testing)?

We'd have to make a build of the demo app that could be fetched by appveyor. At the moment the demo app doesn't let us test very much (only colors and text reading). So the value is limited. I think if we covered a bit more functionality this would be worth it.

There are three demo apps, which I used to test variations in how the GUI APIs use GDI. It isn't immediately obvious how we would structure the tests to interact with these variations.

Used 'git shortlog -s -n -- <filePath>' to find contributors
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

UserGuide change looks fine, good work!

source/colors.py Show resolved Hide resolved
source/colors.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants