Added PlaceholderText functionality to TextBox control

[stefanov-stefan]

Added TextBox NullText support.

[merriemcgaw]

Please determine if the contrast ration between the NullText and the background color is a minumum of 4.5.1 as described here. This is a minimum compliance requirement for accessibility. I see that you're using SystemColors.GrayText, which should adjust properly in HighContrast modes. But it is also required that all text in the UI meet the minimum contrast ratios.

Please also ensure that the text of the property is made available to accessibility tooling so that Narrator can announce the the text when running.

[merriemcgaw]

I'd like to consider an alternative name to the property. Perhaps PlaceholderText?

[dotMorten]

PlaceHolderText and WatermarkText seem to be more common. Also I assume it'll also show when it's empty and not null.

[stefanov-stefan]

I'd like to consider an alternative name to the property. Perhaps PlaceholderText?

PlaceHolderText and WatermarkText seem to be more common. Also I assume it'll also show when it's empty and not null.

PlaceholderText sounds great and yes, it will change when null or empty.

[stefanov-stefan]

Please determine if the contrast ration between the NullText and the background color is a minumum of 4.5.1 as described here. This is a minimum compliance requirement for accessibility. I see that you're using SystemColors.GrayText, which should adjust properly in HighContrast modes. But it is also required that all text in the UI meet the minimum contrast ratios.

Please also ensure that the text of the property is made available to accessibility tooling so that Narrator can announce the the text when running.

@merriemcgaw I have checked and the contrast ration between SystemColors.GrayText (#6D6D6D) and the White (#ffff) background is 5.15:1

I will also make sure to return the PlaceholderText property value in the AccessibleObject, when the control has no text. But have in mind, that the narrator will probably not be able to read this text, as the PlaceholderText disappears when the control take focus.

[zsd4yr]

hey @dotMorten just flagging with No Merge until we go open (trying to avoid someone accidentally pushing the button). I wouldn't want anyone to pull the "pull" level on accident

[merriemcgaw]

@stefanov-stefan The PR is really adding new API. All new APIs in .NET Coreare required to go through API approval process first and should follow the Issue Guide as well. .
Can you please open an issue to track the API addition? We can have the discussion there about all the things you and I are discussing directly in the PR and other topics.
I would recommend we close this PR in the meantime. We can reopen it once the API is approved.

[stefanov-stefan]

@stefanov-stefan The PR is really adding new API. All new APIs in .NET Coreare required to go through API approval process first and should follow the Issue Guide as well. .
Can you please open an issue to track the API addition? We can have the discussion there about all the things you and I are discussing directly in the PR and other topics.
I would recommend we close this PR in the meantime. We can reopen it once the API is approved.

@merriemcgaw the issue is added: #134

Let me know if you need anything else.

[AdamYoblick]

@stefanov-stefan Please close and reopen this PR to trigger the CI build again. Sorry for the inconvenience.

[merriemcgaw]

@stefanov-stefan thank you for the feature! We need to keep this in the future milestone until we get a functional test infrastructure stood up for Core so that you can add functional tests. I'd also really like to see some unit tests given our lack of coverage in the TextBox, especially over the code paths added.

Could you also attach screenshots of it running on a Per-Monitor DPI Aware application before and after it's been dragged from 100% scaling monitor to 300% scaling.

[Tanya-Solyanik]

Had you checked ratio for all high contrast modes?

[Tanya-Solyanik]

Looks good, please add unit tests, add the new API to the ref assembly and proceed with the API review.
Thank you!

[nikeee]

I am really excited about this new feature!
One question, though: Why did you implement the actual drawing? There have been other ways of achieving this that use the native Windows placeholder functionality. E.g.: https://github.com/nikeee/HolzShots/blob/master/src/HolzShots.UI/Windows/Forms/PromptTextBox.vb#L27 (using a SendMessage call, sending the EM_SETCUEBANNER message).

[KlausLoeffelmann]

Since we are surfacing a new property publicly here (PlaceholderText), I think we need to update the reference assembly definition in this case? @AdamYoblick , @Shyam-Gupta , @zsd4yr , @Tanya-Solyanik - until know, I patched System.Windows.Forms.cs in \winforms\src\System.Windows.Forms\ref manually. Is this best practice? Is there still an automatic process where I could generate the updated RefAssembly source for example with /t:GenerateReferenceSource?

Edit:
I just saw #175, and I like that a lot! Still, we'd need the updated reference assembly as part of this PR, I suppose.

[zsd4yr]

Yeah once #175 is done this should not be an issue

[stefanov-stefan]

@Tanya-Solyanik i have addressed your reviews and pushed a new commit.

Below you can see how it looks on 100% scale, on 200% scale and then back on 100% scale - it is pixel perfect.

@KlausLoeffelmann I have added the property with its arttributes in the System.Windows.Forms.cs file in the ref project. Is there is anything else I gotta add here?

[KlausLoeffelmann]

Hey Stefan,

Thanks for the screenshots!

Can you check, though, if those are rendered DpiUnaware? (Because it seems like that on first sight).
We would need screenshots for SystemAware and PerMonitorAwareV2 with VisualStyles enabled.

Said that, it looks like the Window is SystemAware, but the content does not render correctly then with visual styles. Can you check if you got the latest bits for testing that? (We had a problem with resources not being properly included in system.windows.forms.dll in one of the previous versions which caused EnableVisualStyles to fail - but that bug is fixed. If you have EnableVisualStyles in your program.cs, we probably got another problem I don't know of yet, and would need to investigate. In that case: Thanks for pointing out a bug! 😊).

[stefanov-stefan]

I attempted to run the CI with close and reopen, however when i closed git is not playing well with be and does not allow me to reopen. Can someone reopen this PR? @merriemcgaw @Tanya-Solyanik

[stefanov-stefan]

Hey Stefan,

Thanks for the screenshots!

Can you check, though, if those are rendered DpiUnaware? (Because it seems like that on first sight).
We would need screenshots for SystemAware and PerMonitorAwareV2 with VisualStyles enabled.

Said that, it looks like the Window is SystemAware, but the content does not render correctly then with visual styles. Can you check if you got the latest bits for testing that? (We had a problem with resources not being properly included in system.windows.forms.dll in one of the previous versions which caused EnableVisualStyles to fail - but that bug is fixed. If you have EnableVisualStyles in your program.cs, we probably got another problem I don't know of yet, and would need to investigate. In that case: Thanks for pointing out a bug! 😊).

Yes, these shots are rendered with DpiUnaware. Attaching for SystemAware and PerMonitorAwareV2.

[KlausLoeffelmann]

Sorry, they are not. Also, VisualStyles are still off?
They should look something like this:

[stefanov-stefan]

VisualStyles are enabled as is by default. The app I am testing with is in the winforms sln WinformsControlsTests project. Here is the branch where I work: https://github.com/stefanov-stefan/winforms/tree/TextBox-NullText

Could you please clarify why they are not OK?

[KlausLoeffelmann]

When you create a new WinForms Core App based on the latest build, and enable SystemAware, does it render correctly? Is your demo app VB or C# (there might be a problem with VB)?

[stefanov-stefan]

I am in FX app, not core with C#. Above I indicated the app I am using - it is the one included in the System.Windows.Forms.sln

[nikeee]

I'm curious, what's the advantage over using the SendMessage described above? Wouldn't this come with High-DPI support for free?

[AdamYoblick]

@stefanov-stefan I'm trying to reopen this PR and I'm not able to, even as a repo admin. :(

[karelz]

I think it is, because it was created when the repo was private. @raffaeler ran into the same thing and had to recreate his PR.

[raffaeler]

@AdamYoblick I can confirm. It was a pain because my repo lost the status of forked and I had to start from scratch (re-fork, clone the repo, diff the files whose state was of course different, etc.)

[karelz]

I wonder if making the fork public would help restore the relationships and everything ... didn't try it myself though.

[stefanov-stefan]

I made it public, but it did not help. Will go over the process again and make new PR, however, the project from master is no longer building due to missing types from System.Security.Permissions. Once it is back up and running i will get the new PR.

Thanks

[bergmeister]

Since MSFT owns GitHub now, you should definitely feed this problem back to them and get it fixed.

[karelz]

@bergmeister I don't think that statement differs if MS owns GH or not. It is feedback which should be sent there.
How to prioritize is obviously up to the product group (GH in this case).
Once GH has bug reporting channel, we should try to report it there. Given that this is a rare case, it is not too high on my priority list (i.e. there is 20 other things I want GH to fix first as they have much higher impact on repos ;)).

[zsd4yr]

(fyi: I sent the feedback)

[stefanov-stefan]

I sent it too back when I experienced it, so I hope it will be taken into consideration.

[bergmeister]

Sorry for the late response and thank you for taking the time to send the feedback :)

[RussKie]

One question, though: Why did you implement the actual drawing? There have been other ways of achieving this that use the native Windows placeholder functionality. E.g.: ...using a SendMessage call, sending the EM_SETCUEBANNER message.

This remains a big open question. Why have we opted to a custom implementation instead of leveraging the behaviours provided to us by the underlying Win32 control?

Not only the custom implementation introduced a number of issues, it is unaware of the native implementation and visually inconsistent with it:

            textBox1.PlaceholderText = "Custom!";
            SendMessageW(textBox1.Handle, EM_SETCUEBANNER, IntPtr.Zero, "NATIVE PlaceHolder..");

Note the visual differences:

• This implementation - TextBox control with PlaceHolderText property set:
  

• Native implementation - TextBox control with the placeholder text set via the EM_SETCUEBANNER:
  

  • different locations,
  • different font colours, and
  • different shortening strategies.

What's worse though the current implementation suffers from a quite an observable flickering when mouse is moved over the control:

• The top is TextBox control with PlaceHolderText property set, and
• the bottom is TextBox control with the placeholder text set via the EM_SETCUEBANNER
  

The new implementation is also not UIA aware:

• This implementation
  
• Native implementation:
  

[KlausLoeffelmann]

Can't we change this for 6? (And make it virtual along the way?)