Skip to content
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

Add InputScope Property To NumberBox #3186

Merged
merged 9 commits into from
Sep 24, 2020
Merged

Add InputScope Property To NumberBox #3186

merged 9 commits into from
Sep 24, 2020

Conversation

robloo
Copy link
Contributor

@robloo robloo commented Aug 22, 2020

Add InputScope Property To NumberBox

Adds the following API to NumberBox

DependencyProperty InputScopeProperty { get; }
InputScope InputScope { get; set; }

Motivation and Context

Closes #2710 , relates to #3158

Also relates to the previous discussions in #1912 and PR #2605 where the InputScope was fixed to Number.

How Has This Been Tested?

  1. The property has been added to the MUXControlsTestApp in the NumberBoxPage and manually tested
  2. A VerifyInputScopePropogates() API test was added and passes

Screenshots (if appropriate):

example1

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 22, 2020
@ghost
Copy link

ghost commented Aug 22, 2020

CLA assistant check
All CLA requirements met.

@robloo robloo changed the title Robloo/numberbox input scope prop Adds InputScope Property To NumberBox Aug 22, 2020
@robloo robloo changed the title Adds InputScope Property To NumberBox Add InputScope Property To NumberBox Aug 22, 2020
@robloo
Copy link
Contributor Author

robloo commented Aug 22, 2020

I have no idea how to set a default value for an InputScope type in the IDL to 'Number' since InputScope is a class that only has a parameterless constructor and requires the Names property to be set see how 'To change the input scope in code'. This may not be supported? It looks like TextBox itself defaults to null.

If someone confirms this isn't possible I guess the work-around is to set the value in OnApplyTemplate().

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Aug 22, 2020

@robloo You could set the default value in the NumberBox constructor. If you set it in OnApplyTemplate() you could end up overwriting the custom value supplied by a developer (without further checks, for example). Furthermore, if you set in in the NumberBox constructor, it acts as the de-facto default value as NumberBox.InputScope will be set even if the NumberBox has only be constructed as a standalone object and not yet added to the visual tree (so no Loaded event raised or OnApplyTemplate() called). It will also make sure custom values set by developers will take precedence over the default value which is what we want.

(NumberBox, for example, also sets its NumberFormatter in the constructor. We would do the following for the InputScope API.)

@robloo
Copy link
Contributor Author

robloo commented Aug 22, 2020

@Felix-Dev Yes, not sure why I was thinking to put it in the OnApplyTemplate() now -- if I would have thought it through it will be fine in the constructor even before the TextBox is materialized.

I'll go ahead and set the default in the constructor if you also can't think of how to do this in the IDL.

@Felix-Dev
Copy link
Contributor

I'll go ahead and set the default in the constructor if you also can't think of how to do this in the IDL.

@robloo Yeah, right now I don't know how to do it it purely in the IDL either so I would just go with the constructor approach here.

@YuliKl YuliKl added area-NumberBox NumberBox Control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Aug 24, 2020
@StephenLPeters
Copy link
Contributor

I'll go ahead and set the default in the constructor if you also can't think of how to do this in the IDL.

@robloo Yeah, right now I don't know how to do it it purely in the IDL either so I would just go with the constructor approach here.

The other option, which I think I like better, would be to leave the property null and put a setter in the default style which sets it to number. That way a controls library could change the default for their customers with an updated style.

// Note that InputScope is a class that cannot be set to a default value within the IDL.
winrt::InputScopeName inputScopeName = winrt::InputScopeName(winrt::InputScopeNameValue::Number);
winrt::InputScope inputScope = winrt::InputScope();
inputScope.Names().Append(inputScopeName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names [](start = 15, length = 5)

I'm not that familiar with inputScope, does this mean that you can apply multiple input scopes? like you can have number and currency at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that you can apply multiple input scopes?
Yes, you can apply multiple name values to the same input scope.

It's certainly an interesting design decision, one which I've never understood myself. However, my assumption has always been it's for the cases where the user can switch the keyboard. It simply limits what keyboards you are allowed to switch between. You might have to reach out to the Windows team to understand the thinking here though. I don't recall ever seeing documented reasons why this is a collection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeHillberg do you happen to know?

@robloo
Copy link
Contributor Author

robloo commented Aug 31, 2020

I'll go ahead and set the default in the constructor if you also can't think of how to do this in the IDL.

@robloo Yeah, right now I don't know how to do it it purely in the IDL either so I would just go with the constructor approach here.

The other option, which I think I like better, would be to leave the property null and put a setter in the default style which sets it to number. That way a controls library could change the default for their customers with an updated style.

Good idea, that is a cleaner approach.

@robloo
Copy link
Contributor Author

robloo commented Sep 1, 2020

@StephenLPeters

    <Style TargetType="local:NumberBox">
        <Setter Property="InputScope" Value="Number" />

I'm getting a XAML compile error "The TypeConverter for "InputScope" does not support converting from a string". This works fine in C# but looks like another one of those random missing features for C++ developers.

Edit: Actually I can't explain why this doesn't work considering setting the InputBox TextBox's InputScope to Number works just fine.

@StephenLPeters
Copy link
Contributor

Edit: Actually I can't explain why this doesn't work considering setting the InputBox TextBox's InputScope to Number works just fine.

hmm and the property types are the same right? @fabiant3 is there something we need to to in the IDL to get the type conversion to work?

{
// Sets the default value of the InputScope property.
// Note that InputScope is a class that cannot be set to a default value within the IDL.
winrt::InputScopeName inputScopeName = winrt::InputScopeName(winrt::InputScopeNameValue::Number);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we prefer using auto and const for the local variable types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem, should I pursue this now though or wait for potential work-arounds setting in the default style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did it now so hopefully we can close this. There is no response on work-arounds for the XAML bugs.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fabiant3
Copy link
Member

fabiant3 commented Sep 9, 2020

@StephenLPeters

    <Style TargetType="local:NumberBox">
        <Setter Property="InputScope" Value="Number" />

I'm getting a XAML compile error "The TypeConverter for "InputScope" does not support converting from a string". This works fine in C# but looks like another one of those random missing features for C++ developers.

Edit: Actually I can't explain why this doesn't work considering setting the InputBox TextBox's InputScope to Number works just fine.

If direct conversion does not work, you can provide a custom conversion function essentially using x:Bind syntax. @RealTommyKlein

@StephenLPeters
Copy link
Contributor

@fabiant3 This is a change to a templated control, x:Bind isn't supported in templates. I think we probably could using binding with a converter, however I'm not sure we should publish a new converter to work around this, given that setting the default from the constructor seems to work alright?

@Felix-Dev
Copy link
Contributor

Felix-Dev commented Sep 10, 2020

@StephenLPeters Can you check and see what the TextBox implementation is doing as you can set TextBox.InputScope in XAML for it just fine?

@RealTommyKlein
Copy link
Contributor

I wasn't able to repro the error you're seeing with an InputScope property/type in a Setter, even for a custom class with its own custom InputScope DP to mimic this NumberBox setup. Can you share the exact error message/code you're getting?

@StephenLPeters
Copy link
Contributor

Thanks for looking into this tommy, I confirmed that in my test app, if I add a second copy of my templated control to my page I get the same error the MUX controls test app is getting. @MikeHillberg or @Austin-Lamb do you know why input scope is behaving weirdly here? @robloo I think we should go back to setting the default value in the constructor.

@robloo
Copy link
Contributor Author

robloo commented Sep 22, 2020

Should this be considered a bug to be closed with WinUI 3? I can't imagine this behavior is an optimization.

@StephenLPeters
Copy link
Contributor

I think so, but I'm hoping other team members might have a better idea what is going on.

@robloo
Copy link
Contributor Author

robloo commented Sep 22, 2020

@StephenLPeters I reverted the last commit, should be ready

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@StephenLPeters StephenLPeters merged commit 7c3ba16 into microsoft:master Sep 24, 2020
@robloo robloo deleted the robloo/numberbox-input-scope-prop branch September 25, 2020 17:16
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NumberBox NumberBox Control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Expose InputScope property on NumberBox
7 participants