Skip to content

Expose transparent background colors for displayModel#12658

Merged
feerrenrut merged 15 commits into
masterfrom
colorref
Jul 27, 2021
Merged

Expose transparent background colors for displayModel#12658
feerrenrut merged 15 commits into
masterfrom
colorref

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Jul 20, 2021

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

@feerrenrut feerrenrut requested review from a team as code owners July 20, 2021 11:16
@AppVeyorBot

This comment has been minimized.

Sign in to view
seanbudd
seanbudd reviewed Jul 21, 2021
View reviewed changes

@seanbudd seanbudd 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.

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.

Comment thread devDocs/displayModel.md
Comment thread nvdaHelper/remote/gdiHooks.cpp Outdated
@feerrenrut

feerrenrut commented Jul 21, 2021

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

@feerrenrut
update copyright year and contributors
5bd1cfc 
Used 'git shortlog -s -n -- <filePath>' to find contributors
@feerrenrut
correct display model testing steps
76bbae0
lukaszgo1
lukaszgo1 reviewed Jul 21, 2021
View reviewed changes
Comment thread devDocs/displayModel.md
Qchristensen
Qchristensen approved these changes Jul 22, 2021
View reviewed changes

@Qchristensen Qchristensen 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.

UserGuide change looks fine, good work!

LeonarddeR
LeonarddeR approved these changes Jul 22, 2021
View reviewed changes
Comment thread source/colors.py
Comment thread source/colors.py Outdated
@LeonarddeR LeonarddeR mentioned this pull request Jul 24, 2021
Closed
8 tasks
@feerrenrut feerrenrut merged commit ca871a4 into master Jul 27, 2021
@feerrenrut feerrenrut deleted the colorref branch July 27, 2021 06:56
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Jul 27, 2021
@Adriani90 Adriani90 mentioned this pull request Apr 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@michaelDCurran michaelDCurran michaelDCurran approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

+2 more reviewers

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@LeonarddeR LeonarddeR LeonarddeR approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/display-model

Projects

None yet

Milestone

2021.3

Development

Successfully merging this pull request may close these issues.

8 participants

@feerrenrut @AppVeyorBot @LeonarddeR @seanbudd @lukaszgo1 @michaelDCurran @Qchristensen @nvaccessAuto