[WinRT/UWP] Enable selection in password entry #677

Merged
merged 2 commits into from Mar 27, 2017

Conversation

Projects
None yet
5 participants
@pauldipietro
Member

pauldipietro commented Jan 11, 2017

Description of Change

There is presently an issue in WinRT/UWP where entries with Password = true do not allow selection of text. This is a larger issue on the desktop than mobile, but is an issue for both. What this PR changes in particular is that selection is allowed, but also handles a variety of other inputs. The code is appropriately commented due to being a process that involves

  • Caching the size of the selection of text for deletion purposes
  • Caching whether the Ctrl key is in a pressed state and handling the key routing
  • Handling any use of the Shift key, Tab key, arrow keys, or Home/End/PgUp/PgDown keys (which can be used to select text or leave the entry)
  • Handle any use of the Copy (C), Cut (X), Undo (Z), or Redo (Y) keys in the instance that the Ctrl key is in being actively pressed to prevent those shortcuts from working (which emulates the behavior of a PasswordBox)
  • Remove any text if the aforementioned cached selection size is greater than 0 (in other instances a backspace will simply remove the single character)

Note that this should fix the core problem with being unable to select the text, but does not fix three issues:

  • If the back button is pressed with the cursor mid-password, the trailing character gets removed.
  • Inserting a character/pasting into in the middle of the password causes unintended results.
  • The context menu still permits use those keyboard shortcuts which are otherwise disabled. An issue is that WinRT does not support the use of Clipboard so this makes things more difficult. If the text is copied, however, it is only copied as obfuscation characters. This can possibly be revisited in the future.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=45277

API Changes

None

Behavioral Changes

The only one that was very clearly visible was the fact that the password characters can be copied, but again, those are only the obfuscation characters (●).

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
- SelectionStart = Text.Length;
+ // The _ctrlKeyActive flag is used to track if that key is pressed; we want to pretend Ctrl is not actively being
+ // used by treating the key routing as handled.
+ if (e.Key == VirtualKey.Control)

This comment has been minimized.

@hartez

hartez Feb 7, 2017

Member

Is this double check intentional?

@hartez

hartez Feb 7, 2017

Member

Is this double check intentional?

This comment has been minimized.

@pauldipietro

pauldipietro Feb 8, 2017

Member

Nope 😃

@pauldipietro

pauldipietro Feb 8, 2017

Member

Nope 😃

+ {
+ if (e.Key == VirtualKey.Control)
+ {
+ _ctrlKeyActive = true;

This comment has been minimized.

@hartez

hartez Feb 7, 2017

Member

Rather than tracking this with a member variable, you could probably just do something like
var ctrl = Window.Current.CoreWindow.GetKeyState(VirtualKey.Control).HasFlag(CoreVirtualKeyStates.Down); here; then you don't have to worry about OnKeyUp.

@hartez

hartez Feb 7, 2017

Member

Rather than tracking this with a member variable, you could probably just do something like
var ctrl = Window.Current.CoreWindow.GetKeyState(VirtualKey.Control).HasFlag(CoreVirtualKeyStates.Down); here; then you don't have to worry about OnKeyUp.

This comment has been minimized.

@pauldipietro

pauldipietro Feb 8, 2017

Member

Good idea, made that change and it looks to behave the same.

@pauldipietro

pauldipietro Feb 8, 2017

Member

Good idea, made that change and it looks to behave the same.

+ // key to trigger OnKeyDown, then treat it as handled.
+ var ctrlDown = Window.Current.CoreWindow.GetKeyState(VirtualKey.Control).HasFlag(CoreVirtualKeyStates.Down);
+
+ if (ctrlDown && e.Key == VirtualKey.Control)

This comment has been minimized.

@hartez

hartez Feb 8, 2017

Member

This if block seems unnecessary. You're checking whether the Ctrl key is down and the Ctrl key is the one being pressed.

@hartez

hartez Feb 8, 2017

Member

This if block seems unnecessary. You're checking whether the Ctrl key is down and the Ctrl key is the one being pressed.

This comment has been minimized.

@pauldipietro

pauldipietro Feb 8, 2017

Member

Yeah, you're right; I was concerned about the handling but it should be taken care of later via base.OnKeyDown(e) so I'll remove it.

@pauldipietro

pauldipietro Feb 8, 2017

Member

Yeah, you're right; I was concerned about the handling but it should be taken care of later via base.OnKeyDown(e) so I'll remove it.

@rmarinho rmarinho merged commit 90582e9 into master Mar 27, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3745, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

@rmarinho rmarinho deleted the fix-bugzilla45277 branch Jun 22, 2017

@samhouts samhouts added D-15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment