Normalize Shift modifier for character keycodes

Depending on the platform, reporting of the SHIFT modifier is not always
supported. This results in cases where keybindings that usually use the
SHIFT modifier (such as the ? character) not working as expected.

This change updates the keybindings, to remove and ignore any SHIFT
modifiers for any character keybinding, as well as convert the character
to the uppercase variant. To reflect this change, the event handler has
been changed to remove the SHIFT modifier for all characters, instead of
the old behavior of removing the SHIFT modifier only for uppercase ASCII
characters.

6 files changed, 131 insertions, 73 deletions

diff --git a/src/config/src/utils/get_input.rs b/src/config/src/utils/get_input.rs
index 1c77765..1205b26 100644
--- a/src/config/src/utils/get_input.rs
+++ b/src/config/src/utils/get_input.rs
@@ -9,8 +9,8 @@ pub(crate) fn get_input(config: Option<&Config>, name: &str, default: &str) -> R
 	for mut value in input.split_whitespace().map(String::from) {
 		let mut modifiers = vec![];
 
-		if let Some(index) = value.to_lowercase().find("shift+") {
-			modifiers.push("Shift");
+		let shift_index = value.to_lowercase().find("shift+");
+		if let Some(index) = shift_index {
 			value.replace_range(index..index + 6, "");
 		}
 		if let Some(index) = value.to_lowercase().find("control+") {
@@ -22,45 +22,53 @@ pub(crate) fn get_input(config: Option<&Config>, name: &str, default: &str) -> R
 			value.replace_range(index..index + 4, "");
 		}
 
-		values.push(format!(
-			"{}{}",
-			modifiers.join(""),
-			match value.to_lowercase().as_ref() {
-				"backspace" => String::from("Backspace"),
-				"backtab" => String::from("BackTab"),
-				"delete" => String::from("Delete"),
-				"down" => String::from("Down"),
-				"end" => String::from("End"),
-				"enter" => String::from("Enter"),
-				"esc" => String::from("Esc"),
-				"home" => String::from("Home"),
-				"insert" => String::from("Insert"),
-				"left" => String::from("Left"),
-				"pagedown" => String::from("PageDown"),
-				"pageup" => String::from("PageUp"),
-				"right" => String::from("Right"),
-				"tab" => String::from("Tab"),
-				"up" => String::from("Up"),
-				v => {
-					if v.len() > 1 {
-						// allow F{number} values
-						if v.starts_with('f') && v[1..].parse::<u8>().is_ok() {
-							v.to_uppercase()
-						}
-						else {
-							return Err(ConfigError::new(
-								name,
-								input.as_str(),
-								ConfigErrorCause::InvalidKeyBinding,
-							));
-						}
-					}
-					else {
-						value
-					}
-				},
+		let mut key = match value.to_lowercase().as_ref() {
+			"backspace" => String::from("Backspace"),
+			"backtab" => String::from("BackTab"),
+			"delete" => String::from("Delete"),
+			"down" => String::from("Down"),
+			"end" => String::from("End"),
+			"enter" => String::from("Enter"),
+			"esc" => String::from("Esc"),
+			"home" => String::from("Home"),
+			"insert" => String::from("Insert"),
+			"left" => String::from("Left"),
+			"pagedown" => String::from("PageDown"),
+			"pageup" => String::from("PageUp"),
+			"right" => String::from("Right"),
+			"tab" => String::from("Tab"),
+			"up" => String::from("Up"),
+			v => {
+				let v_len = v.chars().count();
+				// allow F{number} values
+				if v_len > 1 && v.starts_with('f') && v[1..].parse::<u8>().is_ok() {
+					v.to_uppercase()
+				}
+				else if v_len == 1 {
+					value
+				}
+				else {
+					return Err(ConfigError::new(
+						name,
+						input.as_str(),
+						ConfigErrorCause::InvalidKeyBinding,
+					));
+				}
+			},
+		};
+
+		// Shift support was partially removed, due to Shift not being universally reported, but still maintain
+		// some backwards compatibility with printable characters
+		if shift_index.is_some() {
+			if key.len() == 1 {
+				key = key.to_uppercase();
+			}
+			else {
+				modifiers.push("Shift");
 			}
-		));
+		}
+
+		values.push(format!("{}{}", modifiers.join(""), key));
 	}
 	Ok(values)
 }
@@ -75,7 +83,9 @@ mod tests {
 	use crate::testutils::{invalid_utf, with_git_config};
 
 	#[rstest]
-	#[case::single("a", "a")]
+	#[case::single_lower("a", "a")]
+	#[case::single_upper("A", "A")]
+	#[case::single_non_ascii("ẞ", "ẞ")]
 	#[case::backspace("backspace", "Backspace")]
 	#[case::backtab("backtab", "BackTab")]
 	#[case::delete("delete", "Delete")]
@@ -97,16 +107,19 @@ mod tests {
 	#[case::modifier_character_uppercase("Control+A", "ControlA")]
 	#[case::modifier_character_number("Control+1", "Control1")]
 	#[case::modifier_character_special("Control++", "Control+")]
+	#[case::modifier_character_non_ascii("Control+ẞ", "Controlẞ")]
 	#[case::modifier_character("Control+a", "Controla")]
 	#[case::modifier_special("Control+End", "ControlEnd")]
 	#[case::modifier_function("Control+F32", "ControlF32")]
-	#[case::modifier_control_alt_shift_lowercase("alt+shift+control+end", "ShiftControlAltEnd")]
-	#[case::modifier_control_alt_shift_mixedcase("aLt+shIft+conTrol+eNd", "ShiftControlAltEnd")]
-	#[case::modifier_control_alt_shift_out_of_order_1("Alt+Shift+Control+End", "ShiftControlAltEnd")]
-	#[case::modifier_control_alt_shift_out_of_order_2("Shift+Control+Alt+End", "ShiftControlAltEnd")]
+	#[case::modifier_control_alt_shift_lowercase("alt+shift+control+end", "ControlAltShiftEnd")]
+	#[case::modifier_control_alt_shift_mixedcase("aLt+shIft+conTrol+eNd", "ControlAltShiftEnd")]
+	#[case::modifier_control_alt_shift_out_of_order_1("Alt+Shift+Control+End", "ControlAltShiftEnd")]
+	#[case::modifier_control_alt_shift_out_of_order_2("Shift+Control+Alt+End", "ControlAltShiftEnd")]
 	#[case::modifier_only_shift("Shift+End", "ShiftEnd")]
 	#[case::modifier_only_control("Control+End", "ControlEnd")]
-	#[case::multiple("a b c d", "a,b,c,d")]
+	#[case::shift_with_printable_lower("Shift+a", "A")]
+	#[case::shift_with_printable_upper("Shift+A", "A")]
+	#[case::multiple("a b ẞ c d", "a,b,ẞ,c,d")]
 	#[case::multiple_with_modifiers("Control+End Control+A", "ControlEnd,ControlA")]
 	fn read_value(#[case] binding: &str, #[case] expected: &str) {
 		with_git_config(&["[test]", format!("value = {binding}").as_str()], |git_config| {
diff --git a/src/core/src/components/confirm/mod.rs b/src/core/src/components/confirm/mod.rs
index dd76c0a..6c0d009 100644
--- a/src/core/src/components/confirm/mod.rs
+++ b/src/core/src/components/confirm/mod.rs
@@ -41,7 +41,6 @@ impl Confirm {
 		if let Event::Key(key) = event {
 			if let KeyCode::Char(c) = key.code {
 				let mut event_lower_modifiers = key.modifiers;
-				event_lower_modifiers.remove(KeyModifiers::SHIFT);
 				let event_lower = Event::Key(KeyEvent::new(
 					KeyCode::Char(c.to_ascii_lowercase()),
 					event_lower_modifiers,
diff --git a/src/core/src/components/shared/editable_line.rs b/src/core/src/components/shared/editable_line.rs
index 42e46e3..cb3167d 100644
--- a/src/core/src/components/shared/editable_line.rs
+++ b/src/core/src/components/shared/editable_line.rs
@@ -186,7 +186,7 @@ impl EditableLine {
 			},
 			Event::Key(KeyEvent {
 				code: KeyCode::Char(c),
-				modifiers: KeyModifiers::NONE | KeyModifiers::SHIFT,
+				modifiers: KeyModifiers::NONE,
 			}) => {
 				let start = UnicodeSegmentation::graphemes(self.content.as_str(), true)
 					.take(self.cursor_position)
@@ -485,10 +485,7 @@ mod tests {
 	fn add_character_uppercase() {
 		let mut editable_line = EditableLine::new();
 		editable_line.set_content("abcd");
-		_ = editable_line.handle_event(Event::Key(KeyEvent {
-			code: KeyCode::Char('X'),
-			modifiers: KeyModifiers::SHIFT,
-		}));
+		_ = editable_line.handle_event(Event::from(KeyCode::Char('X')));
 		assert_rendered_output!(
 			Options AssertRenderOptions::INCLUDE_TRAILING_WHITESPACE,
 			view_data_from_editable_line!(&editable_line),
diff --git a/src/input/src/event.rs b/src/input/src/event.rs
index 9b57e10..7ad9d3d 100644
--- a/src/input/src/event.rs
+++ b/src/input/src/event.rs
@@ -93,13 +93,10 @@ mod tests {
 	fn from_crossterm_key_event_char_with_modifier() {
 		let event = Event::from(ct_event::Event::Key(ct_event::KeyEvent::new(
 			KeyCode::Char('?'),
-			KeyModifiers::SHIFT,
+			KeyModifiers::ALT,
 		)));
 
-		assert_eq!(
-			event,
-			Event::Key(KeyEvent::new(KeyCode::Char('?'), KeyModifiers::SHIFT))
-		);
+		assert_eq!(event, Event::Key(KeyEvent::new(KeyCode::Char('?'), KeyModifiers::ALT)));
 	}
 
 	#[test]
diff --git a/src/input/src/key_bindings.rs b/src/input/src/key_bindings.rs
index ab22276..0c5184a 100644
--- a/src/input/src/key_bindings.rs
+++ b/src/input/src/key_bindings.rs
@@ -84,10 +84,7 @@ pub fn map_keybindings<CustomEvent: crate::CustomEvent>(bindings: &[String]) ->
 					let key_number = k[1..].parse::<u8>().unwrap_or(1);
 					KeyCode::F(key_number)
 				},
-				k => {
-					// printable characters cannot use shift
-					KeyCode::Char(k.chars().next().expect("Expected only one character from Char KeyCode"))
-				},
+				k => KeyCode::Char(k.chars().next().expect("Expected only one character from Char KeyCode")),
 			};
 			Event::Key(KeyEvent::new(code, modifiers))
 		})
@@ -168,11 +165,4 @@ mod tests {
 			Event::from(key_code)
 		]);
 	}
-
-	#[test]
-	fn map_keybindings_key_code_char_remove_shift() {
-		assert_eq!(map_keybindings::<TestEvent>(&[String::from("ShiftA")]), vec![
-			Event::from(KeyCode::Char('A'))
-		]);
-	}
 }
diff --git a/src/input/src/key_event.rs b/src/input/src/key_event.rs
index 40d9a2a..2eaf13e 100644
--- a/src/input/src/key_event.rs
+++ b/src/input/src/key_event.rs
@@ -15,13 +15,11 @@ impl KeyEvent {
 	#[must_use]
 	#[inline]
 	pub fn new(mut code: KeyCode, mut modifiers: KeyModifiers) -> Self {
-		// ensure that uppercase characters always have SHIFT
+		// normalize keys with the SHIFT modifier
 		if let KeyCode::Char(c) = code {
-			if c.is_ascii_uppercase() {
-				modifiers.insert(KeyModifiers::SHIFT);
-			}
-			else if modifiers.contains(KeyModifiers::SHIFT) {
+			if modifiers.contains(KeyModifiers::SHIFT) {
 				code = KeyCode::Char(c.to_ascii_uppercase());
+				modifiers.remove(KeyModifiers::SHIFT);
 			}
 		}
 		Self { code, modifiers }
@@ -41,3 +39,67 @@ impl From<KeyCode> for KeyEvent {
 		Self::new(code, KeyModifiers::empty())
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	#[test]
+	fn new_non_character() {
+		assert_eq!(KeyEvent::new(KeyCode::Backspace, KeyModifiers::ALT), KeyEvent {
+			code: KeyCode::Backspace,
+			modifiers: KeyModifiers::ALT
+		});
+	}
+
+	#[test]
+	fn new_lowercase_character_without_shift() {
+		assert_eq!(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE), KeyEvent {
+			code: KeyCode::Char('a'),
+			modifiers: KeyModifiers::NONE
+		});
+	}
+
+	#[test]
+	fn new_uppercase_character_without_shift() {
+		assert_eq!(KeyEvent::new(KeyCode::Char('A'), KeyModifiers::NONE), KeyEvent {
+			code: KeyCode::Char('A'),
+			modifiers: KeyModifiers::NONE
+		});
+	}
+
+	#[test]
+	fn new_lowercase_character_with_shift() {
+		assert_eq!(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::SHIFT), KeyEvent {
+			code: KeyCode::Char('A'),
+			modifiers: KeyModifiers::NONE
+		});
+	}
+
+	#[test]
+	fn new_uppercase_character_with_shift() {
+		assert_eq!(KeyEvent::new(KeyCode::Char('A'), KeyModifiers::SHIFT), KeyEvent {
+			code: KeyCode::Char('A'),
+			modifiers: KeyModifiers::NONE
+		});
+	}
+
+	#[test]
+	fn from_crossterm_key_event() {
+		assert_eq!(
+			KeyEvent::from(crossterm::event::KeyEvent::new(KeyCode::Char('a'), KeyModifiers::ALT)),
+			KeyEvent {
+				code: KeyCode::Char('a'),
+				modifiers: KeyModifiers::ALT
+			}
+		);
+	}
+
+	#[test]
+	fn from_keycode() {
+		assert_eq!(KeyEvent::from(KeyCode::Char('a')), KeyEvent {
+			code: KeyCode::Char('a'),
+			modifiers: KeyModifiers::NONE
+		});
+	}
+}