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

Align NamedKey and KeyCode more closely with the W3C specs #4018

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Dec 1, 2024

Remove NamedKey::Space and rename uses of "super" to "meta".

These differences from the W3C specifications were introduced in #2662, though I can't find any discussion about it other than in #2662 (comment). It seems like it was left mostly as an unresolved question?

I don't have any preference for either name, but I believe it is more confusing than helping that Winit is diverging and choosing to prefer "super" when the spec prefers "meta". Besides, it is blocking integration with keyboard-types.

Part of #2394.

Beware that this could break in confusing ways if someone was matching on NamedKey::Super, but we have ModifiersState for a reason, which I think most users will be using anyhow (and there it'll break loudly because ModifiersState::SUPER is gone).

CC @maroider @ArturKovacs @dhardy whom previously worked on this.

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality

@madsmtm madsmtm added S - api Design and usability S - docs Awareness, docs, examples, etc. I - BREAKING CHANGE labels Dec 1, 2024
@maroider
Copy link
Member

maroider commented Dec 1, 2024

These differences from the W3C specifications were introduced in #2662, though I can't find any discussion about it other than in #2662 (comment). It seems like it was left mostly as an unresolved question?

I recall there being more of a discussion about it, but I don't recall where. I'll have a look when i have time.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 1, 2024

I found #1788, searching for "super" and "meta" with all items in that thread expanded reveals #1788 (comment).

While I can see the arguments in there (the "alt" key was commonly called "meta"), I don't think they really holds for our use-case (we have a distinct "alt" keys, so there should be no confusion). Regardless of my desire to use keyboard-types, I think this concern is overshadowed by the value in having the same terminology as the web standard, and in emitting the same keyboard events as browser do.

CC @chrisduerr

@madsmtm
Copy link
Member Author

madsmtm commented Dec 1, 2024

Also, I found #1788 (comment) regarding NamedKey::Space. Responding to that:

I introduced Space as a variant to the Key enum. In the UI Events Spec this is instead indicated by the normal space character ' '. To me it seems odd to have a Tab variant but not have a Space variant; it feels out of place to use a Character for representing the space key, especially because logical_key is not intended for text input.

One argument for why these are different might be that space is a normal, printable character, while tab is a control character? Though that's mostly post-rationalization on my part.

@chrisduerr
Copy link
Contributor

While I can see the arguments in there (the "alt" key was commonly called "meta"), I don't think they really holds for our use-case (we have a distinct "alt" keys, so there should be no confusion). Regardless of my desire to use keyboard-types, I think this concern is overshadowed by the value in having the same terminology as the web standard, and in emitting the same keyboard events as browser do.

The meta key can be one of many keys. Especially for Linux users this very frequently refers to the alt key. I see no reason why there would be a requirement to align with the web standard over other platform's standards and if all are considered equal then the most logical solution should be chosen. I have little doubt that super is least likely to cause any confusion compared to alternatives like "meta" or "hyper", which are just historically highly inaccurate.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 2, 2024

The meta key can be one of many keys. Especially for Linux users this very frequently refers to the alt key. [...] I have little doubt that super is least likely to cause any confusion compared to alternatives like "meta" or "hyper", which are just historically highly inaccurate.

Hmm, I read the Wikipedia articles on these, and I can see that "super" is superior (hehe) in the sense that it's likely what you will want to show the user in your documentation - but you will want to do special handling here anyhow, e.g. neither "meta" nor "super" is a nice name to show the user on macOS.

Looking more widely:

  • The web uses "meta", e.g. metaKey
  • Qt seems to use "meta", e.g. Qt::MetaModifier, though they seem to muck around with things a lot, e.g. "ctrl" is swapped with "meta" on macOS, and they do also expose Qt::Key_Super_[LR].
  • GTK have both "super" and "meta", not sure which they prefer, but they do map macOS' Command to "meta", see Gdk.ModifierType.
  • The Linux Kernel uses "meta" in input-event-codes.h.
  • X11 uses "super", e.g. it maps the above to XK_Super_[LR].
  • Android uses "meta", e.g. AKEYCODE_META_LEFT.
  • macOS and Windows use their own naming ("command" and "win").

I guess we could choose to emit NamedKey::Meta on most platforms, but NamedKey::Super on Linux when we get keysyms::Super_L, though that would be inconsistent with what browsers do on Linux, so I'd prefer to avoid that.

I see no reason why there would be a requirement to align with the web standard over other platform's standards and if all are considered equal then the most logical solution should be chosen.

I think my core argument is that all are not considered equal, and that the web standard, however flawed, makes sense to align to, for the simple reason that it's a tried and tested way to make cross-platform applications.

This is not in the W3C spec, and doesn't make sense to special-case.
@madsmtm madsmtm force-pushed the madsmtm/keyboard-align-to-spec branch 2 times, most recently from 040e621 to 3bd40d7 Compare December 2, 2024 12:53
@ArturKovacs
Copy link
Contributor

I still don't feel strongly about renaming Super to Meta. What @chrisduerr said made sense at that time, but with that said, I don't mind if this rename happens. In fact here's an argument for renaming: I remember it being clumsy trying to avoid calling functions and variables super. Unfortunately it must be avoided as super is a keyword in Rust.

Regarding the question about Space, I feel similarly. If there's a decision to change it, that's okay with me, with the caveat that it should be clearly documented that the space key is represented as Character(" ")

Comment on lines -798 to +792
/// Used to enable "super" modifier function for interpreting concurrent or subsequent keyboard
/// Used to enable "meta" modifier function for interpreting concurrent or subsequent keyboard
/// input. This key value is used for the "Windows Logo" key and the Apple `Command` or `⌘`
/// key.
///
/// Note: In some contexts (e.g. the Web) this is referred to as the "Meta" key.
Super,
Meta,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding that this is referred to as Super in some contexts. Having this in the documentation makes it easier to search.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a doc alias of "super" so that people searching the docs for super will find it?

https://doc.rust-lang.org/rustdoc/advanced-features.html#add-aliases-for-an-item-in-documentation-search

Copy link
Member

Choose a reason for hiding this comment

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

Maybe alias it the other way (meta to super) around to not break things in the first place? Especially given that meta means so many different things on platforms and to get it wrong will be very easy.

@madsmtm madsmtm force-pushed the madsmtm/keyboard-align-to-spec branch 2 times, most recently from f61c465 to c04e6a1 Compare December 2, 2024 13:53
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Super is the label you likely see on the keyboard. Meta is often an Alt on Linux and I don't think I've seen anyone referring to meta as super, so I don't see a point in renaming at this point.

One could add an alias(somehow), but I don't like to change its name at this point, especially users will likely not have a clue what it is.

@madsmtm madsmtm mentioned this pull request Dec 2, 2024
@madsmtm
Copy link
Member Author

madsmtm commented Dec 2, 2024

Super is the label you likely see on the keyboard.

Possibly, though not on keyboards manufactured with macOS and/or Windows in mind, so I don't think we need to use the same label in source code.

Meta is often an Alt on Linux and I don't think I've seen anyone referring to meta as super, so I don't see a point in renaming at this point.

I feel like I outlined a lot of cases in #4018 (comment) where people are referring to the key as "meta"?


In any case, my issue is not really the naming (I superficially agree that "super" is the better name), but that we diverge from the web specification which we otherwise try to follow.

My main motivation here is because aligning with the web standard is needed to do #4026. If y'all have other suggestions for how to integrate keyboard-types without doing this change in Winit, then I'm all ears?

This is inconsistent with the W3C spec, and while it's arguably not the
best name, it's worse that Winit is diverging and choosing a different
name.

---

Full list of changes:

KeyCode:
- SuperLeft -> MetaLeft
- SuperRight -> MetaRight
- Meta -> Super

NamedKey::Meta is swapped with NamedKey::Super.

ModifiersState::SUPER is renamed to ModifiersState::META.

ModifiersState::super_key is renamed to ModifiersState::meta_key.

ModifiersKeys::LSUPER and RSUPER are renamed to LMETA and RMETA.
@madsmtm madsmtm force-pushed the madsmtm/keyboard-align-to-spec branch from c04e6a1 to 6d46d52 Compare December 2, 2024 16:22
@chrisduerr
Copy link
Contributor

What @chrisduerr said made sense at that time, but with that said, I don't mind if this rename happens.

I think every breaking change should be minded. There's already many people complaining that winit breaks things too much.

@madsmtm
Copy link
Member Author

madsmtm commented Dec 2, 2024

I think every breaking change should be minded. There's already many people complaining that winit breaks things too much.

Fair point! Would it help if we kept ModifiersState::SUPER as a deprecated alias of ModifiersState::META?

We can't really do something similar with NamedKey::Super though, since both that and NamedKey::Meta exist (it's the semantics that change).

@chrisduerr
Copy link
Contributor

Would it help if we kept ModifiersState::SUPER as a deprecated alias of ModifiersState::META?

Steps can be taken to make the impact of breaking changes minimal or avoid a breaking change entirely. Of course this makes sense whenever possible.

However any breaking change, no matter how small, will be a strain on library consumers. The only way to avoid that is to not break things unless absolutely necessary.

@kchibisov
Copy link
Member

I personally don't think that we should change it at this point, like we had it as LOGO now it's SUPER and changing to META makes it even more confusing.

I'm also not entirely sure if raw keyboard-types should be used transparently and not wrapped by us and e.g. provide a conversion for it under feature or something (especially given that winit-core will be very light).

@madsmtm madsmtm added the C - nominated Nominated for discussion in the next meeting label Dec 2, 2024
@madsmtm
Copy link
Member Author

madsmtm commented Dec 2, 2024

I'm also not entirely sure if raw keyboard-types should be used transparently and not wrapped by us and e.g. provide a conversion for it under feature or something

Yeah, I recognize that's an option, but it's just so... Unsatisfying! Especially given that the types (winit::keyboard::KeyCode and keyboard_types::Code) are literally just enums, and meant to be derived from the same spec...

I've nominated the issue, let's discuss it next meeting, whenever that happens.

@ArturKovacs
Copy link
Contributor

I think that it's a big issue is that keyboard_types has a Super variant, which means something different from our current Super. But I also agree that winit should support keyboard_types.

Here's a suggestion for completely avoiding breaking changes: Introduce a new key event called KeyEvent2 or KeyboardEvent. This new event would be almost entirely identical to the original KeyEvent except, it would use the keyboard_types crate. Adding a deprecated attribute to the original KeyEvent could be done, but at that point it could just be left as-is, always kept backward compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - nominated Nominated for discussion in the next meeting I - BREAKING CHANGE S - api Design and usability S - docs Awareness, docs, examples, etc.
Development

Successfully merging this pull request may close these issues.

6 participants