Display model: Try to provide the correct background color for transparent text drawing (issue #6467) by mwcampbell · Pull Request #6844 · nvaccess/nvda

mwcampbell · 2017-02-04T16:18:25Z

As described in issue #6467, the NVDA display model doesn't provide the correct background color for text that is drawn in transparent mode. A complete solution for this problem is difficult, but a good enough fix for some instances of the problem is to get the previous color at the origin (X, Y) coordinates before calling the original TextOut or ExtTextOut function. Note that this implementation only applies to the TextOut and ExtTextOut functions, since other cases such as PolyTextOut are more difficult.

THe motivating use case, and the only one I've tested, is the Dragon NaturallySpeaking vocabulary editor. In that editor's list view, without this fix, the highlighted line is reported as being white on white. With this fix, it's correctly reported as white on aqua blue.

…color at the provided X, Y coordinates before calling the original function, so we at least have a chance of knowing the correct background color if the text is being drawn in transparent mode. This is necessary to detect highlighted text in some parts of Dragon NaturallySpeaking.

michaelDCurran

A part from documenting the new argument in the docstring above the function, this looks good to me.

michaelDCurran · 2017-02-05T00:16:03Z

@@ -360,7 +360,7 @@ GlyphTranslatorCache glyphTranslatorCache;
 * @param resultTextSize an optional pointer to a SIZE structure that will contain the size of the text.
 * @param direction >0 for left to right, <0 for right to left, 0 for neutral or unknown. Text must still be passed in in visual order.
  */


Please document prevColor in the comment above the function.

derekriemer · 2017-02-05T00:19:53Z

 * @param direction >0 for left to right, <0 for right to left, 0 for neutral or unknown. Text must still be passed in in visual order.
  */
-void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT* lprc,UINT fuOptions,UINT textAlign, BOOL stripHotkeyIndicator, const wchar_t* lpString, const int codePage, const int* lpdx, int cbCount, LPSIZE resultTextSize, int direction) {
+void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT* lprc,UINT fuOptions,UINT textAlign, BOOL stripHotkeyIndicator, const wchar_t* lpString, const int codePage, const int* lpdx, int cbCount, LPSIZE resultTextSize, int direction, COLORREF prevColor) {


Mind putting documentation on the color parameter, like done above?

derekriemer

Mind updating copyright info
Copyright 2006-2010 NVDA contributers.
to
Copyright 2006-2017 NV Access Limited, (you)

derekriemer · 2017-02-05T00:22:00Z

 * @param direction >0 for left to right, <0 for right to left, 0 for neutral or unknown. Text must still be passed in in visual order.
  */
-void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT* lprc,UINT fuOptions,UINT textAlign, BOOL stripHotkeyIndicator, const wchar_t* lpString, const int codePage, const int* lpdx, int cbCount, LPSIZE resultTextSize, int direction) {
+void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT* lprc,UINT fuOptions,UINT textAlign, BOOL stripHotkeyIndicator, const wchar_t* lpString, const int codePage, const int* lpdx, int cbCount, LPSIZE resultTextSize, int direction, COLORREF prevColor) {


While you're here, if you get a moment, mind putting a space between the comma and u after lprc,UINT

derekriemer · 2017-02-05T00:23:30Z

@@ -512,7 +516,7 @@ void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT*
 * @param lpString the string of ansi text you wish to record.
 * @param codePage the code page used for the string which will be converted to unicode
  */


Please put an @param if possible.

derekriemer · 2017-02-05T00:24:28Z

 * @param codePage the code page used for the string which will be converted to unicode
  */
-void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT* lprc,UINT fuOptions,UINT textAlign, BOOL stripHotkeyIndicator, const char* lpString, const int codePage, const int* lpdx, int cbCount, LPSIZE resultTextSize, int direction) {
+void ExtTextOutHelper(displayModel_t* model, HDC hdc, int x, int y, const RECT* lprc,UINT fuOptions,UINT textAlign, BOOL stripHotkeyIndicator, const char* lpString, const int codePage, const int* lpdx, int cbCount, LPSIZE resultTextSize, int direction, COLORREF prevColor) {


Mind putting the space here again? You didn't introduce this, but might as well clean it up while here.

mwcampbell · 2017-02-05T15:53:08Z

I made the documentation change that @michaelDCurran requested. I'm not sure if the copyright notice should be updated as @derekriemer requested, since other source files don't include contributors' names in the copyright notice.

michaelDCurran · 2017-02-05T23:05:19Z

In most of the NVDA source, we've been moving to a joint copyright statement between NV Access Limited and external contributors e.g. Copyright (C) 2006-2017 NV Access Limited, Mat Campbell Granted though it seems we seem to be only doing this for our Python files and forgetting all the c++. :) So, you are welcome to change it or not as you please.

jcsteh · 2017-06-07T05:06:38Z

Unfortunately, this needs to be backed out due to serious performance regressions experienced in certain apps. See #7251.

It's possible that using BitBlt and GetDIBits might be more performant, though this is just a guess. We did see some performance problems in the past with GetPixel (see 18b3933), though that could just have been due to multiple calls.

…r transparent text drawing (#6844)" This has been confirmed by several users as causing severe performance regressions in certain apps. This reverts commit 29efeda. Fixes #7251. Reverts PR #6844. Reopens issue #6467.

…arent text drawing (#6844) * GDI hooks: In the TextOut and ExtTextOut functions, get the previous color at the provided X, Y coordinates before calling the original function, so we at least have a chance of knowing the correct background color if the text is being drawn in transparent mode. This is necessary to detect highlighted text in some parts of Dragon NaturallySpeaking. Fixes #6467

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>

michaelDCurran requested changes Feb 5, 2017

View reviewed changes

derekriemer reviewed Feb 5, 2017

View reviewed changes

derekriemer requested review from michaelDCurran and removed request for michaelDCurran February 5, 2017 00:31

derekriemer reviewed Feb 5, 2017

View reviewed changes

GDI hooks: Document the new prevColor parameter for ExtTextOutHelper

0c36b12

michaelDCurran approved these changes Feb 7, 2017

View reviewed changes

michaelDCurran added a commit that referenced this pull request Feb 7, 2017

Incubates #6844

0027a67

nvaccessAuto added the incubating label Feb 7, 2017

nvaccessAuto assigned michaelDCurran Feb 7, 2017

michaelDCurran merged commit 29efeda into nvaccess:master Mar 14, 2017

nvaccessAuto removed the incubating label Mar 14, 2017

nvaccessAuto added this to the 2017.2 milestone Mar 14, 2017

michaelDCurran mentioned this pull request Jul 25, 2017

displayModel: second try at recording the actual background color for text when a transparant background color is set. #7440

Merged

feerrenrut mentioned this pull request Jul 20, 2021

Expose transparent background colors for displayModel #12658

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Display model: Try to provide the correct background color for transparent text drawing (issue #6467)#6844

Display model: Try to provide the correct background color for transparent text drawing (issue #6467)#6844
michaelDCurran merged 2 commits into
nvaccess:masterfrom
mwcampbell:displaymodel-transparency-fix

mwcampbell commented Feb 4, 2017

Uh oh!

michaelDCurran left a comment

Uh oh!

michaelDCurran Feb 5, 2017

Uh oh!

derekriemer Feb 5, 2017

Uh oh!

derekriemer left a comment

Uh oh!

derekriemer Feb 5, 2017

Uh oh!

derekriemer Feb 5, 2017

Uh oh!

derekriemer Feb 5, 2017

Uh oh!

mwcampbell commented Feb 5, 2017

Uh oh!

michaelDCurran commented Feb 5, 2017 via email

Uh oh!

jcsteh commented Jun 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

mwcampbell commented Feb 4, 2017

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

michaelDCurran Feb 5, 2017

Choose a reason for hiding this comment

Uh oh!

derekriemer Feb 5, 2017

Choose a reason for hiding this comment

Uh oh!

derekriemer left a comment

Choose a reason for hiding this comment

Uh oh!

derekriemer Feb 5, 2017

Choose a reason for hiding this comment

Uh oh!

derekriemer Feb 5, 2017

Choose a reason for hiding this comment

Uh oh!

derekriemer Feb 5, 2017

Choose a reason for hiding this comment

Uh oh!

mwcampbell commented Feb 5, 2017

Uh oh!

michaelDCurran commented Feb 5, 2017 via email

Uh oh!

jcsteh commented Jun 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants