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.

Author: MitMaro

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| {

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,

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),

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]

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'))
-		]);
-	}
 }

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
+		});
+	}
+}