[UWP] AccessKey support on VisualElement and TabbedPage #2061

henricm · 2018-03-09T10:11:34Z

Description of Change

Fixes #1672 by adding AccessKey, AccessKeyPlacement and horizontal
and vertical offset properties to Windows specific VisualElement class.
Also handles AccessKeys for tabbed pages (ContentPage).

API Changes

Added:

New enum AccessKeyPlacement for specifying placement of the access key tooltip.
string WindowsSpecific.VisualElement.AccessKey { get; set; } //Bindable Property
AccessKeyPlacement WindowsSpecific.VisualElement.AccessKeyPlacement { get; set; } //Bindable Property
double WindowsSpecific.VisualElement.AccessKeyHorizontalOffset{ get; set; } //Bindable Property
double WindowsSpecific.VisualElement.AccessKeyVerticalOffset{ get; set; } //Bindable Property

Behavioral Changes

When you set AccessKey property button, tab etc will be able to execute its action when user presses the Alt-button. When the Alt-button is pushed once, access key tooltips will be displayed.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Consolidate commits as makes sense

jassmith · 2018-03-09T18:40:40Z

Xamarin.Forms.Platform.UAP/TabbedPageRenderer.cs

+				case NotifyCollectionChangedAction.Replace:
+					if (e.NewItems != null)
+						foreach (var c in e.NewItems)
+							(c as Page).PropertyChanged += OnChildPagePropertyChanged;


I wonder if there is a way to not sub/unsub this event and still achieve this effect. Its expensive to sub PropertyChanged as it triggers a fair bit...

Good point. However - is it that big difference compared to other property change handling.
One thought could be to try to handle this in xaml with converters or something? Haven't looked alot into that yet though.

PureWeen

Overall looks great. This will be super useful.

Did you look into implementing IsAccessKeyScope at the layout level at all?

Not sure if that will be translatable from a forms layout scope to UWP so was curious if you had looked into it much with this change?

PureWeen · 2018-03-12T16:44:10Z

Xamarin.Forms.Platform.UAP/TabbedPageRenderer.cs

+				Element.CurrentPage = page;
+		}
+
+		protected void UpdateAccessKey(TextBlock control) {


I realize there are subtle differences but can the UpdateAccessKey that's in VisualElementRenderer and TabbedPageRenderer be generalized into a reusable internal static method?

Created AccessKeyHelper which should take care of this now.

PureWeen · 2018-03-12T16:47:06Z

Xamarin.Forms.Platform.UAP/TabbedPageRenderer.cs

+				case NotifyCollectionChangedAction.Remove:
+				case NotifyCollectionChangedAction.Replace:
+					if (e.NewItems != null)
+						foreach (var c in e.NewItems)


These might go away because of Jason's comments but if not can you change to for (int i

Sure. Curious though - why is that preferred? :-)

There are cases where foreach vs for doesn't matter and there are cases where it does matter as far as Memory and performance (especially on the Mono side). Generally the for will perform better or the same as foreach

In a case like this with so few items, where it happens very infrequently and it's UWP it's most likely irrelevant but it's more about a consistent pattern we use throughout the code base. So unless there's a compelling reason (other than just being easier) to use foreach we just use for (int i

henricm · 2018-03-13T08:54:41Z

@PureWeen thanks for the feedback!

I haven't looked into implementing access key scope in this PR. I read about how it works in UWP but I reasoned that as a first step we only wanted basic support for access keys.
However, I could dive more into that if desired? In that case we probably want to add support for AccessKeyInvoked event too.
There is also localization of access keys which natively is handled in XAML and I'm not sure how that would translate in the Xamarin.Forms world.

PureWeen · 2018-03-13T18:44:35Z

@henricm Yea I looked at the Localization as well seeing what made sense. My thought here is that since this is all settable from forms as a string you can just use the Localization APIs through forms to set things to a Localized value.. If you use the bindings to set these properties then localization can just be achieved that way

As far as the native support for localization that feels like it would just be a different issue as that's just about setting the Uid of the elements right? Opening up a platform API that lets you set the Uid on the native control would just be generally useful it seems but outside the scope of this issue.

As far as IsAccessKeyScope. My thoughts would be that if it's easy enough to apply the property at the layout level and then translate this down into UWP go for it but if the devil in the details makes this too tricky then a separate issue would suffice.. Is it as easy as adding a property to Xamarin.Forms.Layout then having LayoutRenderer on UWP set the property? Or am I being short sighted?

henricm · 2018-03-14T13:11:50Z

@PureWeen sounds good regarding localization and I will definitely have a look at access key scope. Hopefully later this week.

hartez

Looks great, just need to reduce the duplicate code and I think it's ready.

hartez · 2018-03-23T17:09:25Z

Xamarin.Forms.Platform.UAP/TabbedPageRenderer.cs

@@ -429,5 +460,65 @@ void UpdateBarIcons()
 					Control.InvalidateMeasure();
 			}
 		}
+        void UpdateToolbarPlacement()


Spaces -> Tabs

henricm · 2018-03-27T14:15:47Z

@PureWeen I wrestled a bit with adding IsAccessKeyScope on Layout and then setting the property on the UWP control. However I couldn't get it to work I'm afraid. I also tried enumerate the UWP specific controls (Panels etc) and try to figure out if the property could be set there instead. But I never managed to find a way to get it to work. I'm sure that someone with more UWP and/or Xamarin.Forms knowledge will be able to solve this.

PureWeen · 2018-03-27T16:57:40Z

@henricm yea :-/ I hoped it'd be just be an easy win but I'm not surprised it wasn't. Changes look good

Fixes xamarin#1672 by adding AccessKey, AccessKeyPlacement and horizontal and vertical offset properties to Windows specific VisualElement class. Also handles AccessKeys for tabbed pages (ContentPage).

Refs xamarin#1672 and fixes issue with AccessKey not being updated after tabbed page and its children had been created. The problem was that the Reset event in OnPagesChange wasn't handled correctly. We now add listeners to the child pages of the tabbed page which makes sure AccessKey gets updated if user makes changes to tab object.

Updated foreach-loop and replaced spaces with tabs.

henricm · 2018-04-09T11:28:57Z

@hartez the duplicate has been removed and everything is rebased as of today so please take another look when you have time. Thanks!

henricm requested review from hartez and PureWeen March 9, 2018 10:11

jassmith reviewed Mar 9, 2018

View reviewed changes

samhouts added this to Ready in v3.1.0 via automation Mar 9, 2018

samhouts added t/enhancement ➕ F100 p/UWP labels Mar 9, 2018

samhouts moved this from Ready to In Review in v3.1.0 Mar 9, 2018

PureWeen suggested changes Mar 12, 2018

View reviewed changes

samhouts added this to In Progress in Enhancements Mar 22, 2018

hartez suggested changes Mar 23, 2018

View reviewed changes

henricm force-pushed the AccessKeyWindows branch 2 times, most recently from 85a05b0 to 909f070 Compare March 27, 2018 14:09

xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label Mar 27, 2018

PureWeen approved these changes Mar 27, 2018

View reviewed changes

henricm added 3 commits April 9, 2018 11:38

[UWP] Initial support for AccessKey

9008bac

Fixes xamarin#1672 by adding AccessKey, AccessKeyPlacement and horizontal and vertical offset properties to Windows specific VisualElement class. Also handles AccessKeys for tabbed pages (ContentPage).

Moved shared code to internal class AccessKeyHelper.

bdaa38a

Updated foreach-loop and replaced spaces with tabs.

henricm force-pushed the AccessKeyWindows branch from 909f070 to bdaa38a Compare April 9, 2018 09:39

xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label Apr 9, 2018

hartez approved these changes Apr 23, 2018

View reviewed changes

Merge branch 'master' into AccessKeyWindows

dcb1bc8

xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label Apr 23, 2018

rmarinho merged commit efbadcf into xamarin:master Apr 24, 2018

v3.1.0 automation moved this from In Review to Done Apr 24, 2018

Enhancements automation moved this from In Progress to Done Apr 24, 2018

henricm deleted the AccessKeyWindows branch April 24, 2018 12:34

samhouts added this to the 3.1.0 milestone May 5, 2018

samhouts modified the milestone: 3.1.0 Jun 1, 2018

samhouts removed this from Done in Enhancements Jun 12, 2018

samhouts added the community-sprint label Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UWP] AccessKey support on VisualElement and TabbedPage #2061

[UWP] AccessKey support on VisualElement and TabbedPage #2061

henricm commented Mar 9, 2018 •

edited by samhouts

jassmith Mar 9, 2018

henricm Mar 13, 2018

PureWeen left a comment

PureWeen Mar 12, 2018

henricm Mar 27, 2018

PureWeen Mar 12, 2018

henricm Mar 13, 2018

PureWeen Mar 13, 2018 •

edited

henricm commented Mar 13, 2018

PureWeen commented Mar 13, 2018

henricm commented Mar 14, 2018

hartez left a comment

hartez Mar 23, 2018

henricm commented Mar 27, 2018

PureWeen commented Mar 27, 2018

henricm commented Apr 9, 2018

[UWP] AccessKey support on VisualElement and TabbedPage #2061

[UWP] AccessKey support on VisualElement and TabbedPage #2061

Conversation

henricm commented Mar 9, 2018 • edited by samhouts

Description of Change

API Changes

Behavioral Changes

PR Checklist

jassmith Mar 9, 2018

Choose a reason for hiding this comment

henricm Mar 13, 2018

Choose a reason for hiding this comment

PureWeen left a comment

Choose a reason for hiding this comment

PureWeen Mar 12, 2018

Choose a reason for hiding this comment

henricm Mar 27, 2018

Choose a reason for hiding this comment

PureWeen Mar 12, 2018

Choose a reason for hiding this comment

henricm Mar 13, 2018

Choose a reason for hiding this comment

PureWeen Mar 13, 2018 • edited

Choose a reason for hiding this comment

henricm commented Mar 13, 2018

PureWeen commented Mar 13, 2018

henricm commented Mar 14, 2018

hartez left a comment

Choose a reason for hiding this comment

hartez Mar 23, 2018

Choose a reason for hiding this comment

henricm commented Mar 27, 2018

PureWeen commented Mar 27, 2018

henricm commented Apr 9, 2018

henricm commented Mar 9, 2018 •

edited by samhouts

PureWeen Mar 13, 2018 •

edited