-
Notifications
You must be signed in to change notification settings - Fork 41
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
Spec Review: Input Validation #26
base: master
Are you sure you want to change the base?
Conversation
Adding the current API for InputValidation
} | ||
} | ||
``` | ||
#### Code – ValidationWrapper (no new APIs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, is this what will be provided? or is it an example of something a developer would write themselves? It seems generic enough that it could be used by anyone and not need recreating by each developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Yes this is boilerplate code that the developer would have to add. What if we added this as part of the community toolkit to make it easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toolkit seems like a good place for this
#endregion | ||
|
||
#region Populate errors (C# - DataAnnotations) | ||
private Task Validate(string propertyName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of this method looks to have errors.
It clears all errors--which may come from multiple properties.
It then only adds errors for the current property.
If multiple properties had errors before validating a property, you'd end up with an error collection that doesn't contain all the errors.
#endregion | ||
|
||
#region privates | ||
private object GetPropertyValue([CallerMemberName] string propertyName = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using [CallerMemberName]
here is unnecessary as this is only called in one place and you'd never want the name of the caller passed.
</ItemsControl> | ||
``` | ||
|
||
## Customizing the error template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work with different InputValidationKind
options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input validation kind determine if the template is displayed inline or in tooltip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm clear that I understand correctly, you're saying that if this is specified it becomes the template that's used when in the error state, regardless of the InputValidationKind
?
What then about needing to allow for coping with variations in the UI for different focus states when also in the error state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for a control primitive which can be styled for this Validation message? It could also be used on other controls that are not TextBox?
|
||
Description text provides input to the end user on what format or content is expected from the control when it is not clear from the header and placeholder text. | ||
|
||
Developers can choose to either show the description text for a control or not. Default is that description text is off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this controlled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the controls have a Description
property:
https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.textbox.description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarification needed: Is this adding a new property/ sub-element called Description
everywhere or is this related to the property that already exists on some controls.
- Disappearing placeholder text strains users’ short-term memory. | ||
- Users may mistake a placeholder for data that was automatically filled in. | ||
- The default light-grey color of placeholder text has poor color contrast against most backgrounds. | ||
- Not all screen readers read placeholder text aloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will the DescriptionText work with screen readers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going to update this section, to speak about how description text works with Input Validation, and remove the general Description text guidance. Taking a look at our documentation, we don't have this called out any where. I will follow up, description was added in 1809, I need to make sure we have this scenario covered.
```csharp | ||
public class Person : ValidationWrapper | ||
{ | ||
[Required] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be any UI affordance or indicator that works with required properties?
If not, why not?
If so, how can this be controlled beyond using the attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the controls were intended to know they are required and put a '*' on the header.
It can be set using the ValidationContext.IsInputRequired
field. This get's auto-generated and set by the markup compiler when using System.ComponentModel.DataAnnotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call this out in the samples.
```xaml | ||
<TextBox x:Name="Email" | ||
Header="Email" | ||
Validation.IsRequired="True" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this indicate that the field is a required field? or that validation should be performed?
If this means that entry in this textbox is required
- How is that displayed?
- Can it be re-templated?
- How should a developer support this in custom controls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is out-of-date. This should say ValidationContext.IsInputRequired
, which hopefully clears any confusion :)
How is that displayed?
By an '*' next to the header
Can it be re-templated?
Yes, this will be part of the ControlTemplate
How should a developer support this in custom controls?
By using the built-in controls as an example and following what they do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that saying developers wishing to build their own custom controls should copy the built-in ones is adequate.
Lots of developers will want to use C#/XAML, rather than the C++ the built-in ones are created with.
Hopefully toolkit controls will be updated accordingly and so they can be used as a reference by 3rd parties but may this also serve as a reminder that samples/examples often need to be in more than one language and copying C++ to create C# equivalents can be daunting for many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully toolkit controls will be updated accordingly and so they can be used as a reference by 3rd parties but may this also serve as a reminder that samples/examples often need to be in more than one language and copying C++ to create C# equivalents can be daunting for many.
Fair point, thank you for bringing that up. I am working on a full end-to-end example for C# and will make sure it's made available
else | ||
// Do something when an error was removed. | ||
|
||
SaveButton.IsEnabled = Validation.GetErrors(e.Source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation on this line risk confusing it as being part of the else
clause.
# API Details | ||
<!-- The exact API, in MIDL3 format (https://docs.microsoft.com/en-us/uwp/midl-3/) --> | ||
|
||
## InputPropertyAttribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This title is very vague and non-descriptive of what it does.
There are lots of properties that capture input but don't generate any validation.
Why not use a name that communicates the intent better, such as GeneratesValidationAttribute
or InputValidationAttribute
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point, thank you! The vagueness was a bit intentional, as I know we've discussed other uses for having this attribute in the past (I don't recall off the top of my head what those were). This attribute was not intended for validation only scenarios, but if we'd prefer it to be more scoped, that is a reasonable thing to do.
<!-- The exact API, in MIDL3 format (https://docs.microsoft.com/en-us/uwp/midl-3/) --> | ||
|
||
## InputPropertyAttribute | ||
The `InputPropertyAttribute` helps us ensure that we only generate validation code for the property specified by this attribute. This will help reduce code bloat by not generating code for properties that will never perform validation (like width). Controls can specify multiple properties as their input property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not apply this to the properties that generate validation, rather than at the class level?
This would remove the need to specify the name. It would also link the property to the fact that is has validation, by putting the attribute next to the property, as this can be easy to miss with large code files or controls that have code over multiple partial files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was meant to follow the same pattern that we did for ContentPropertyAttribute
. I personally don't have much of a preference, and think you make some good points :)
} | ||
|
||
## InputValidationKind | ||
Determines how the control displays validation visuals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a custom control implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will add example that illustrates how a custom control can implement this.
/// <summary> | ||
/// Validation visuals are not displayed at all. | ||
/// </summary> | ||
Hidden, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include an example of why this is relevant/useful?
I assume that it's for scenarios where the app will show errors for the view in a list together, rather than inline, but it would be good to include an explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great questions! So originally, we had to make Compact
the default, since having them set to Inline
would have been a breaking change. Specifying Inline
means that an extra padding is added below the control so that when the errors are shown, there is no affect to layout (assuming only a single line of text).
As for the scenario you are describing about showing errors in a list, that is a great point! That is what Hidden
was for, and it just means that the control won't display the visuals, but some other element can still gather all the errors and display them. Although I do call out that controls shouldn't invoke validation when set to Hidden
, and i think that needs to be re-adjusted.
}; | ||
|
||
## IInputValidationControl | ||
Any control that wants to participate in input validation should derive from this interface. This, among other things, is what allows the markup compiler to know when to generate the code necessary for listening to the `INotifyDataErrorInfo.ErrorsChanged` event and propagate the results to the `IInputValidationControl.Errors` property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should controls "derive" from this interface or implement it?
}; | ||
|
||
## InputValidationCommand | ||
The command allows for easy use of input validation without the need for a tight dependency on x:Bind. This enables developers who aren't striclty using MVVM to use input validation (i.e ReactiveUI). The InputValidationCommand is what allows for the correct-by-design approach where controls initially validate on focus lost, but then will query for validation on property changed once in an invalid state. This wouldn't be for free, but the amount of code here is pretty trivial. We could provide some attribute or pattern that tells the markup compiler how to generate that code automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"striclty" - typo?
## InputValidationCommand | ||
The command allows for easy use of input validation without the need for a tight dependency on x:Bind. This enables developers who aren't striclty using MVVM to use input validation (i.e ReactiveUI). The InputValidationCommand is what allows for the correct-by-design approach where controls initially validate on focus lost, but then will query for validation on property changed once in an invalid state. This wouldn't be for free, but the amount of code here is pretty trivial. We could provide some attribute or pattern that tells the markup compiler how to generate that code automatically. | ||
|
||
The command can shared between different IInputValidationControls, as would most likely be the case in the scenario of a forms control. A custom command can be made that listens for changes to all errors and knows when a form has been completed and ready for submition. The control will query the command as to whether they should perform validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command can be shared between ...
[webhosthidden] | ||
unsealed runtimeclass InputValidationCommand | ||
{ | ||
[method_name("CreateInstance")] InputValidationCommand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting looks weird. Should [method_name("CreateInstance")]
be on its own line, as an attribute on the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this attribute should be needed.
/// <summary> | ||
/// Called by a control to determine if it should call the Validate method. | ||
/// </summary> | ||
Boolean CanValidate(Windows.UI.Xaml.Controls.IInputValidationControl validationControl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include an explanation of why this is needed?
I can see lots of implementations that involve some of the logic necessary for validation. If that's not desirable or the intent this needs to be made clear.
Also, are there considerations that need to be made for the implementation here? such as, this needs to be really performant and so must return quickly as it may get called a lot.
}; | ||
|
||
## ValidationContext | ||
The `ValidationContext` class provides context as to what is being required by validation. This class is auto-generated by the XAML markup compiler in certain scenarios (see [IInputValidationControl]()). Controls can use the provided properties on the `ValidationContext` to customize their appearance. For example, the IsInputRequired field is can allow controls to put the little * next to a header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, the IsInputRequired field is can allow controls to put the little * next to a header.
[webhosthidden] | ||
unsealed runtimeclass InputValidationContext | ||
{ | ||
[method_name("CreateInstance")] InputValidationContext(String memberName, Boolean isRequired); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting looks weird. Should [method_name("CreateInstance")]
be on its own line, as an attribute on the method?
String MemberName{ get; }; | ||
}; | ||
|
||
## InputValidationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this is needed?
Why not just have IInputValidationControl .ValidationErrors
return an Windows.Foundation.Collections.IObservableVector<string>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great question, and i was wondering that myself a lot. A few reasons i kept it around:
- Closer to what WPF has (maybe not a great reason?)
- We might want to change the concept of an "error" to a "result" that can be either positive or negative, allowing controls to give visual feedback when a field is correctly filled out. This becomes a bit more complicated if we are relying on
INotifyDataErrorInfo
since it well, only notifies about errors :) - Easier to extend for the future if it turns out we want to add more functionality. This was a bigger deal when we were designing this feature as part of Windows, since we can't really change APIs once they ship. We probably can have more flexibility here and could just make this a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should probably be system brush resources added to the ThemeResource dictionaries for positive validation, as well as the negative error red and yellow, currently afforded
and why to use this API. --> | ||
|
||
# Background | ||
In WPF Developers had access to Input Validation via [INotifyDataErrorInfo](https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.inotifydataerrorinfo?view=netframework-4.7.2) We are adding the same capability to UWP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not IDataErrorInfo, it's part of .net standard 2.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INotifyDataErrorInfo is a super set. It's a v2 of IDataErrorInfo, we fell that this covers all of the same scenarios and is async by nature. Are there any scenarios we are not thinking of that are supported in IDataErrorInfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but if I understand correctly, IDataErrorInfo is part of .NET Standard 2.0, and it would be extremely confusing not to support it.
I think you can easily do something like this so it will never cost additional performance:
// Async first
var notifyDataErrorInfo = obj as INotifyDataErrorInfo;
if (!(notifyDataErrorInfo is null))
{
// async validation here
return;
}
var dataErrorInfo = obj as IDataErrorInfo;
if (!(dataErrorInfo is null))
{
// sync validation here
return;
}
// Validation not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having worked on several complex WPF and Silverlight forms applications, INotifyDataErrorInfo was sufficient for validation purposes. There was no need for IDataErrorInfo given it was a subset. There a lot of discussion here around this without making reference to Silverlight. Silverlight had/has the most evolved validation support given how it nicely tied into UI controls. Just replicating Silverlight's validation features would be a big step forward for UWP/WPF/Xamarin.
|
||
public ObservableCollection<ValidationError> GetErrors(string propertyName) | ||
{ | ||
return propertyName != null && Errors.ContainsKey(propertyName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this serves as an example which many might just copy/paste, please consider using TryGetValue and a cached empty collection in case of no validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going to update the examples to clearly call out the work required from a developer. We will add in standard conventions to replace my bad PM code ;)
|
||
# Examples | ||
|
||
## End to end input validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example shows both data annotations and INDEI, which looks confusing. Separating out the INDEI into a second example would be easier to read.
|
||
public class Person : ValidationWrapper | ||
{ | ||
[MinLength(4, ErrorMessage = "Name must be at least 4 characters")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message for [MinLength] isn't automatically generated?
#### Code – INotifyDataErrorInfo base class | ||
The NotifyDataErrorInfoBase class (app code) provides a reusable implementation of INotifyDataErrorInfo (new API). It’s the base of ValidationWrapper, which is the base class for Person. | ||
``` csharp | ||
public class NotifyDataErrorInfoBase : INotifyDataErrorInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big example that's covering a lot of ground, too much IMO. On top of learning INDEI I'm learning NotifyDataErrorInfoBase and ValidationWrapper. Better would be to show a trivial INDEI example to explain it, then suggest the wrappers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for feedback. @stevenbrix Is going to write some more coherent examples for this.
``` | ||
|
||
#### C# only - Person (Model) | ||
Person.Name is the source of the bind (this.ViewModel.Person.Name). Person derives from ValidationWrapper (app code) which will provide the data validation for Name. It’s marked with [MinLength] and [MaxLength] data annotations. The property accessors use GetValue/SetValue in the base ValidationWrapper class for storage and to check the validity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ValidationWrapper not part of the API?
``` | ||
|
||
## Validation for view-centric developers | ||
Code a view-centric (Winforms-like) developer would write to enable Validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "WinForms" section is kind of out of the blue. So really there's three approaches: DataAnnotations, INDEI, and programmatic. Explaining this context would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback. Will expand.
<!-- The exact API, in MIDL3 format (https://docs.microsoft.com/en-us/uwp/midl-3/) --> | ||
|
||
## InputPropertyAttribute | ||
The `InputPropertyAttribute` helps us ensure that we only generate validation code for the property specified by this attribute. This will help reduce code bloat by not generating code for properties that will never perform validation (like width). Controls can specify multiple properties as their input property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring an InputPropertyAttribute sounds difficult. Is there a way an x:Bind can specify it if the control author didn't realize it was required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One potential way we could do this is add a Validation
value to the BindingMode
enum:
Text="{x:Bind Person.Name, Mode=Validation}"
Or, we loosen the restriction. It might be unlikely that we'll be generating dead code, but it's hard to say with any certainty that we won't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I associate the mode only with the direction of the binding, so a mode Validation
looks a bit strange to me. Can't we have a ValidatesOnNotifyDataErrors
property like on Binding in WPF:
{x:Bind Person.Name, Mode=TwoWay,ValidatesOnNotifyDataErrors=True}
Unlike in WPF, it could have the default value false. But: We could have the InputPropertyAttribute in addition. If you create a binding on such a property that has the InputPropertyAttribute, you have to explicitly set ValidatesOnNotifyDataErrors to false if you don't want to generate the binding code. By default, ValidatesOnNotifyDataErrors is true on an InputProperty binding
On the other side, if you have a property without the InputPropertyAttribute - the case that @MikeHillberg mentioned - you could set ValidatesOnNotifyDataErrors to true to generate the validation code for that binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prospect of adding Validation
as an option for binding mode concerns me as it potentially causes the mode to have two meanings and would require changes in (especially 3rd party) tooling to handle the change in meaning.
<!-- Option 2: Put these descriptions in the below API Details section, | ||
with a "///" comment above the member or type. --> | ||
|
||
# API Details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost none of this API is demonstrated or explained in the Examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are updating to match the latest IDL provided and clean up the samples.
3. When focus has been lost, validation should be attempted by calling the `ValidationCommand.CanValidate` method. | ||
4. If the `IInputValidationControl.ValidationErrors` collection is non-empty, validation should be attempted by calling the `ValidationCommand.CanValidate` method. | ||
|
||
The markup compiler uses this interface to properly generate code when using x:Bind iff: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if means "if and only if", please write out the whole thing
Co-Authored-By: MikeHillberg <[email protected]>
Text="{x:Bind ViewModel.Person.Name, | ||
UpdateSourceTrigger=PropertyChanged, | ||
Mode=TwoWay}" | ||
InputValidationKind="Hidden" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will all controls that support input validation need this property added? If so it should be in the API section.
Windows.UI.Xaml.Controls.InputValidationCommand ValidationCommand; | ||
}; | ||
|
||
## InputValidationCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did WPF have an equivalent of this? I know we got some pushback on having the framework define Commanding types in the framework layer -- I think folks were expecting these types to come out of the .NET layer. Now maybe .NET doesn't have any equivalent types that can stand in here, but I'd like to understand the need that this is satisfying. Or maybe just the "Command" part of the type name is throwing me off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no they don't, we're going to remove this type and come up with a different, and simpler model for achieving what it's purpose was
/// <summary> | ||
/// DataTemplate that expresses how the errors are displayed. | ||
/// </summary> | ||
Windows.UI.Xaml.DataTemplate ErrorTemplate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does ErrorTemplate need to be on this interface?
/// <summary> | ||
/// Collection of errors to display based on the InputValidationKind property. | ||
/// </summary> | ||
Windows.Foundation.Collections.IObservableVector<Windows.UI.Xaml.Controls.InputValidationError> ValidationErrors{ get; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update this comment to describe where the errors come from? Does the control itself populate them? Does the XAML compiler generated code set them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are populated by the app - either via the compiler if using x:Bind, or the app author
[webhosthidden] | ||
unsealed runtimeclass InputValidationCommand | ||
{ | ||
[method_name("CreateInstance")] InputValidationCommand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this attribute should be needed.
I apologise, but I don't know how to reference the markdown file directly as others have done. With respect to the Custom Validation UI, I'd love to see an attached property for UIElement controls added. I feel this would be less verbose than the sample linked above and also clearer to grok while reading the XAML. I do see value in supporting both methodologies. What would be nice to see is something like
Being able to build a validation message as I've proposed above would allow the validation UI for each form element to be defined in the XAML tree for the relevant control. I don't know whether attached properties allow attributes, but I feel that if possible those two attributes would be quite useful. |
I think there needs to be some thought about the ErrorTextColor in both light and dark themes. Looking at the iconography used by Windows defender, and across the OS - as well as the colours for the proposed Status Banner #913... The Dark Theme error colour is a yellow colour, and light theme uses red. Should the red, green, and yellow colours, used in these icons, not be added as theme resources, and used for error and confirmation validation? They would then be consistent with Light and Dark themes? |
Why only Mode=TwoWay. I would like to see errors for all modes. |
Hmmm, I'm not sure if I understand the reason why you want it for all modes. Normally you show errors to tell the user what they have to enter and in which form they have to enter the data. To allow a user to enter data, you have to use a TwoWay Data Binding -> assuming you're using Data Bindings to connect the UI to your Data/ViewModel. What is the scenario to show errors in a User Interface where a user can't change anything, because there are for example only OneWay Data Bindings? This would be a readonly User Interface. |
In the current project, I have errors in the datagrid in oneway mode |
I'm with @thomasclaudiushuber on this, but would like to understand your use cases more. How are you propagating data back to your model and performing validation? |
@mdtauk if it's not in the spec then that was unintentionally left out, there will be theme resources made available for the error color (not sure we have a confirmation color?) |
@stevenbrix |
|
||
### [App Code] Proper Xaml usage | ||
|
||
Lets first start with some simple XAML that illustrates how this is done from markup. This example satisfies requirements 1-3 described above, since the `TextBox` implements the proper interfaces and specifies that the `Text` property is the input property, and we are using the proper `BindingMode`. It's important to note that the default `UpdateSourceTrigger` for TextBox is `FocusLost`, which is desirable. This ensures that the initial validation occurs after the user has finished entering input before trying to validate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What determines that the UpdateSourceTrigger for TextBox is FocusLost?
Before committing to a final solution, please review the validation features that were present in Silverlight. These worked very well in complex forms applications. Also, I always wondered why Adorners were not added to Silverlight and now UWP. An Adorner allows you to add a layer of functionality over an existing control, so could also be used for validation. |
adding the first draft of Input Validation for review