When the background of a display model chunk is transparent, report that the color is unknown when fetching formatting info#9197
When the background of a display model chunk is transparent, report that the color is unknown when fetching formatting info#9197LeonarddeR wants to merge 4 commits into
Conversation
|
I wrote this pull request on behalf of @BabbageCom. As I'm leaving @BabbageCom after the 29th of November, I can no longer afford maintaining this pr other than applying very basic review actions. If this pull request requires major changes, they will have to be applied by someone else. |
|
I was quite hopeful when I saw this PR since in some old apps I use NVDA currently fails to detect highlight color because background is transparent. It turns out however that with this PR in many cases in which current master can successfully detect the background color it is reported as unknown. I understand that full solution for this is difficult, but in my opinion this is not a correct one. Also contrary to @LeonarddeR's commend from #6467 JAWS with the new accessibility driver introduced in 2018 which relies on hooking GDI functions similar to our displayModel code can detect real background color in these cases so hopefully we should be able to do the same. Since earlier commend by @LeonarddeR indicated that he would not work on this further and the solutions has drawbacks I vote to close this PR. |
|
@lukaszgo1 It looks like this PR reports unknown for any color that has one or more negative channels. One thing I have run into with ctypes previously is when a dll returns an unsigned int, and we try to access it as an int from python. The result of this is a negative value. You can use a ctypes cast to convert a ctypes UINT to a python int. It might be worth investigating that. |
| if (c >> 24) & 0xff: | ||
| return cls(-1, -1, -1) |
There was a problem hiding this comment.
This looks a little suspicious too. It would be good if there was more documentation to explain this. Unit tests would also be nice.
|
The intention of this pr was to report unknown in the case the background is transparent, in which case our display model is unable to assess the right colour. Therefore, it made sense to me to report that the actual color is not known. I'm happy with having this closed if we'd rather have a possibly incorrect color rather than an unknown color. |
|
I'm going to replace this PR with a variation on the approach. Being unable to test the impact of this change meant we were vary wary to introduce it. After doing some testing with GUI API's that use GDI, we found that regularly the background color is reported as "unknown" when visually it matched the background color. We believe in these cases the API on top of GDI is drawing a colored rectangle then layering the text (with transparent) on top, but also setting the background color. I'll expose the transparency via the color class, so it can be used by appmodules. This will allow a targeted approach for situations that really need this. Otherwise there will be no change in behavior. |
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 fix: There have been two prior attempts to fix this, both causing severe performance regressions. - #6844: "Display model: Try to provide the correct background color for transparent text drawing (issue #6467)" - #7440: "displayModel: second try at recording the actual background color for text when a transparant background color is set." This implementation merely exposes information about the potential transparency of the text background. Using the samples in https://github.com/nvaccess/testDisplayModel to test 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: (anecdotal) - 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 introduced 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 interpreting this color value, a single bit is set if the color is considered transparent. # Testing: - 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 Co-authored-by: Leonard de Ruijter <leonard@babbage.com>
Link to issue number:
Workaround for #6467
Summary of the issue:
From #6467 (comment)
Description of how this pull request fixes the issue:
There have been two attempts to fix this, both causing severe performance regressions.
This implementation implements a better safe than sorry approach. When the background of a device context is in transparent mode, the background color of the device context does most likely not apply. In this case, it is better to say that the background color is unknown when someone wants it reported, rather than reporting the wrong color.
The implementation works as follows:
Testing performed:
Tested with a corporate application which has a transparent background as part of a grid.
Known issues with pull request:
We might throw away cases where the dc's background color is equal to the real background color. Still, I prefer not to report wrong information whenever possible.
Change log entry:
Bugfixes, yet to be written