GNOME Bugzilla – Bug 788564
[wayland]: Add keyboard a11y, support for slow-keys, sticky-keys, bounce-keys, toggle-keys
Last modified: 2018-01-11 08:18:29 UTC
Description: Wayland doesn't have keyboard accessibility support, i.e things that are typically handed in X11 via AccessX, namely: - slow-keys (only accept key presses after a delay) - sticky-keys (support one-key-at-a-time operation by latching and locking modifiers) - bounce-keys (reject quick key presses of the same key) - toggle-keys (hold Shift for 8 seconds to enable slow-keys, press sShift key 5 times in a row to enable sticky-keys) Upstream discussions [1] leaned toward an implementation in the compositor rather than client side (unlike key repeat) See also: [1] https://lists.freedesktop.org/archives/wayland-devel/2016-March/027419.html
Created attachment 360973 [details] [review] [PATCH 1/3] wayland: Disable AccessX in Xwayland Keyboard accessibility features in Wayland are handled in the compositor, we do not want AccessX in Xwayland to interfere with the compositor.
Created attachment 360974 [details] [review] [PATCH 2/3] clutter: add slow-keys support in clutter On X11, when AccessX is enabled, all X11 clients benefit from the AccessX features, including gnome-shell/mutter, meaning that special keys like the overview or other shortcuts are also affected. To achieve the same in Wayland, we need to implement the same features in clutter rather than the Wayland backend, so that all depending code within the compositor but also Wayland clients (which rely on the compositor to get keyboard events) similarly benefit from the slow-key feature.
Created attachment 360975 [details] [review] [PATCH 3/3] wayland: Add slow-keys support to MetaWaylandKeyboard This adds slow-keys support in the compositor so that all Wayland clients can have slow-keys working just like in X11 with AccessX.
For now, those patches are mostly an RFC or PoC, but it shows it works pretty well with the Compositor's own shortcuts, as well as Wayland native and Xwayland clients and with modifiers as well as regular keys.
thanks a lot for working on this, Olivier!
Review of attachment 360975 [details] [review]: Why isn't this in the clutter evdev backend?
(In reply to Olivier Fourdan from comment #4) > For now, those patches are mostly an RFC or PoC, but it shows it works > pretty well with the Compositor's own shortcuts, as well as Wayland native > and Xwayland clients and with modifiers as well as regular keys. Certainly a step in the right direction :). The rough plan I had in my head was: 1) Turning keyboard a11y features into a flags enum and a vmethod to ClutterInputDevice, implemented for keyboards. 2) Implementing a11y features through XKB for the X11 Clutter backend (i.e. taking over g-s-d a11y-keyboard module) and integrated in the ClutterDeviceManager event emission for evdev as you do in patch #2. 3) For keyboard bell, adding a ClutterInputDevice signal that we can emit on the keyboard and connect on mutter, moving XkbBell* handling into Clutter X11 so we can emit the signal, and emitting manually on the right conditions on evdev. 4) Applying GSettings to the keyboard device on MetaInputSettings, like we do for other devices. All X11 bits are entirely optional since they're implemented somewhere else :), but this way codepaths would be more similar.
(In reply to Jonas Ådahl from comment #6) > Review of attachment 360975 [details] [review] [review]: > > Why isn't this in the clutter evdev backend? Because we want to delay sending the events, which is done in clutter-main.
(In reply to Olivier Fourdan from comment #8) > (In reply to Jonas Ådahl from comment #6) > > Review of attachment 360975 [details] [review] [review] [review]: > > > > Why isn't this in the clutter evdev backend? > > Because we want to delay sending the events, which is done in clutter-main. Ah right, sorry, yeah that part could very well belong to clutter evdev backend.
Created attachment 361433 [details] [review] [PATCH 1/9] wayland: Disable AccessX in Xwayland
Created attachment 361434 [details] [review] [PATCH 2/9] clutter/backend: Add bell-notify
Created attachment 361435 [details] [review] [PATCH 3/9] backend/x11: implement bell-notify
Created attachment 361436 [details] [review] [PATCH 4/9] backend/native: implement bell-notify
Created attachment 361437 [details] [review] [PATCH 5/9] clutter-main: Add hooks to plug kbd accessibility
Created attachment 361438 [details] [review] [PATCH 6/9] clutter: add hooks for kbd a11y configuration
Created attachment 361439 [details] [review] [PATCH 7/9] clutter/evdev: implement slow keys support
Created attachment 361440 [details] [review] [PATCH 8/9] clutter/evdev: implement bounce keys
Created attachment 361441 [details] [review] [PATCH 9/9] backends: configure keyboard accessibility
Ths new series adds the required mechanism to handle keyboard accessibility and configuration in clutter backends, adds implementation for bell notification (required for keyboard accessibility from clutter) and adds slowkeys and bouncekeys support to clutter evdev. Missing, sticky keys and toggle keys (plus X11 backend implementation to configure AccessX, but this is less of a priority because, well, AccessX works with g-s-d for now, but if/when we want to handle that in mutter, we should have everything in place to do it).
Created attachment 361851 [details] [review] [PATCH 01/14] wayland: Disable AccessX in Xwayland Keyboard accessibility features in Wayland are handled in the compositor, we do not want AccessX in Xwayland to interfere with the compositor.
Created attachment 361852 [details] [review] [PATCH 02/14] clutter/backend: Add bell-notify We'll need a way to trigger a bell from within clutter for keyboard accessibility features, add the necessary hooks to be able to call a backend bell-notify method.
Created attachment 361853 [details] [review] [PATCH 03/14] backend/x11: implement bell-notify
Created attachment 361854 [details] [review] [PATCH 04/14] backend/native: implement bell-notify
Created attachment 361855 [details] [review] [PATCH 05/14] clutter-main: Add hooks to plug kbd accessibility On X11, when AccessX is enabled, all X11 clients benefit from the AccessX features, including gnome-shell/mutter, meaning that special keys like the overview or other shortcuts are also affected. To achieve the same in Wayland, we need to implement the same features in clutter main rather than the Wayland backend, so that all depending code within the compositor but also Wayland clients (which rely on the compositor to get keyboard events) similarly benefit from the accessibility features. Add hooks to the clutter main loop to be able to implement such features.
Created attachment 361856 [details] [review] [PATCH 06/14] clutter: add hooks for kbd a11y configuration Use a set of bitwise enum flags to set different keyboard accessibility features and the necessary vfunc hooks in clutter input device so that keyboard preferences can be passed to different input device implementations. The idea is to be able to configure either the clutter own implementation of keyboard accessibility for evdev, or eventually configure AccessX from the X11 clutter input device using the same mechanism.
Created attachment 361859 [details] [review] [PATCH 07/14] clutter: add keyboard accessibility signals Implementing keyboard accessibility in clutter means we need to be able to notify higher layers of possible modifier masks or setting chnages (as some keyboard accessibility can be disabled or enabled by keyboard). For this purpose, add two new signals: ClutterDeviceManager::kbd-a11y-mods-state-changed ClutterDeviceManager::kbd-a11y-flags-changed
Created attachment 361860 [details] [review] [PATCH 08/14] backends: configure keyboard accessibility Set the relevant flags and values for keyboard accessibility from gsettings.
Created attachment 361861 [details] [review] [PATCH 09/14] clutter/evdev: Implement a11y configuration hooks
Created attachment 361862 [details] [review] [PATCH 10/14] clutter/evdev: implement slow keys support Delay emitting clutter key press events when slow key is enabled.
Created attachment 361863 [details] [review] [PATCH 11/14] clutter/evdev: implement bounce keys support Ignore multiple consecutive identical key presses within a time frame.
Created attachment 361864 [details] [review] [PATCH 12/14] clutter/evdev: implement sticky keys support One key press on a modifier latches it, two consecutive presses lock it.
Created attachment 361865 [details] [review] [PATCH 13/14] wayland/keyboard: Apply sticky keys masks MetaWaylandKeyboard maintains its own xkb_state used to update Wayland clients. Add the necessary hooks to make sure the sticky keys modifier masks set in clutter-evdev are also applied in MetaWaylandKeyboard's xkb_state so that Wayland clients also benefit from sticky keys.
Created attachment 361866 [details] [review] [PATCH 14/14] clutter/evdev: implement toggle keys support Keeping Shift pressed for 8 seconds enables slow keys, pressing Shift 5 times in a row enables sticky keys.
Created attachment 362663 [details] [review] [PATCH 01/21] wayland: Disable AccessX in Xwayland Keyboard accessibility features in Wayland are handled in the compositor, we do not want AccessX in Xwayland to interfere with the compositor.
Created attachment 362664 [details] [review] [PATCH 02/21] clutter/backend: Add bell-notify We'll need a way to trigger a bell from within clutter for keyboard accessibility features, add the necessary hooks to be able to call a backend bell-notify method.
Created attachment 362665 [details] [review] [PATCH 03/21] backend/x11: implement bell-notify
Created attachment 362666 [details] [review] [PATCH 04/21] backend/native: implement bell-notify
Created attachment 362667 [details] [review] [PATCH 05/21] clutter-main: Add hooks to plug kbd accessibility On X11, when AccessX is enabled, all X11 clients benefit from the AccessX features, including gnome-shell/mutter, meaning that special keys like the overview or other shortcuts are also affected. To achieve the same in Wayland, we need to implement the same features in clutter main rather than the Wayland backend, so that all depending code within the compositor but also Wayland clients (which rely on the compositor to get keyboard events) similarly benefit from the accessibility features. Add hooks to the clutter main loop to be able to implement such features.
Created attachment 362668 [details] [review] [PATCH 06/21] clutter: add hooks for kbd a11y configuration Use a set of bitwise enum flags to set different keyboard accessibility features and the necessary vfunc hooks in clutter input device so that keyboard preferences can be passed to different input device implementations. The idea is to be able to configure either the clutter own implementation of keyboard accessibility for evdev, or eventually configure AccessX from the X11 clutter input device using the same mechanism.
Created attachment 362669 [details] [review] [PATCH 07/21] clutter: add keyboard accessibility signals Implementing keyboard accessibility in clutter means we need to be able to notify higher layers of possible modifier masks or setting changes (as some keyboard accessibility can be disabled or enabled by keyboard). For this purpose, add two new signals: ClutterDeviceManager::kbd-a11y-mods-state-changed ClutterDeviceManager::kbd-a11y-flags-changed
Created attachment 362670 [details] [review] [PATCH 08/21] backends: configure keyboard accessibility Set the relevant flags and values for keyboard accessibility from gsettings.
Created attachment 362671 [details] [review] [PATCH 09/21] clutter/evdev: Implement a11y configuration hooks
Created attachment 362675 [details] [review] [PATCH 10/21] clutter/evdev: implement slow keys support Delay emitting clutter key press events when slow key is enabled.
Created attachment 362676 [details] [review] [PATCH 11/21] clutter/evdev: implement bounce keys support Ignore multiple consecutive identical key presses within a time frame.
Created attachment 362677 [details] [review] [PATCH 12/21] clutter/evdev: implement sticky keys support One key press on a modifier latches it, two consecutive presses lock it.
Created attachment 362678 [details] [review] [PATCH 13/21] wayland/keyboard: Apply sticky keys masks MetaWaylandKeyboard maintains its own xkb_state used to update Wayland clients. Add the necessary hooks to make sure the sticky keys modifier masks set in clutter-evdev are also applied in MetaWaylandKeyboard's xkb_state so that Wayland clients also benefit from sticky keys.
Created attachment 362679 [details] [review] [PATCH 14/21] clutter/evdev: implement toggle keys support Keeping Shift pressed for 8 seconds enables slow keys, pressing Shift 5 times in a row enables sticky keys.
Created attachment 362680 [details] [review] [PATCH 15/21] clutter/evdev: implement mouse keys support Control the pointer using the numeric keypad. When enabled, creates a virtual pointer and emulate the pointer event based on keyboard events.
Created attachment 362681 [details] [review] [PATCH 16/21] clutter/x11: Add xkb accessibility helpers Adds a set of convenient functions that can be shared between x11 input device backends (namely core-x11 and xi2) to control XKB accessibility features.
Created attachment 362682 [details] [review] [PATCH 17/21] clutter/x11: Configure XKB accessibility Configure XKB accessibility features from the x11 and xi2 clutter input device managers, offloading this feature from gnome-settings-daemon.
Created attachment 362683 [details] [review] [PATCH 18/21] core: Add MetaKbdA11yDialog Add a new interface for enabling keyboard accessibility features, typically invoked from keyboard "toggle keys" feature, i.e. press the shift key 5 times in a row or keep the shift key depressed for 8 seconds to enable either sticky keys or slow keys.
Created attachment 362684 [details] [review] [PATCH 19/21] core: implement MetaXkbA11yDialogDefault Implement a default keyboard accessibility dialog so the user can tell when the keyboard accessibility settings have changed when triggered by the keyboard accessibility toggle keys, and can either leave or cancel the change. This default dialog should be replaced by a much nicer dialog in the shell.
Created attachment 362685 [details] [review] [PATCH 20/21] compositor: add vmethod to override kbd a11y dialog A MetaPlugin implementation of the MetaKbdA11yDialog which can be used in place of the default keyboard accessibility dialog.
Created attachment 362686 [details] [review] [PATCH 21/21] backends: invoke kbd accessibility dialog Invoke the keyboard accessibility dialog when the keyboard accessibility features are changed from the toggle keys.
With this we have slow keys, sticky keys, bounce keys, toggle keys and mouse keys implemented in clutter evdev backend, along with the configuration mechanisms to control x11 XKB as well so that we have all of keyboard accessibility in one place (mutter). There is also a new KbdA11yDialog interface with the necessary compositor vmethod to let the user know and possibly cancel when features are enabled/disabled from the toggle keys.
Created attachment 363150 [details] [review] [PATCH 13/21] wayland/keyboard: Apply sticky keys masks Rebase after commit b6200ac
Created attachment 363151 [details] [review] [PATCH gnome-shell] ui: Add KbdA11yDialog This is the UI (gnome-shell) implementation of the keyboard accessibility dialog.
More bug fixes coming...
Created attachment 363207 [details] [review] [PATCH 08/21] backends: configure keyboard accessibility
Created attachment 363208 [details] [review] [PATCH 09/21] clutter/evdev: Implement a11y configuration hooks
Created attachment 363209 [details] [review] [PATCH 10/21] clutter/evdev: implement slow keys support
Created attachment 363210 [details] [review] [PATCH 11/21] clutter/evdev: implement bounce keys support
Created attachment 363211 [details] [review] [PATCH 12/21] clutter/evdev: implement sticky keys support Large rework of the sticky keys logic to keep the internal xkb_state consistent.
Created attachment 363212 [details] [review] [PATCH 13/21] wayland/keyboard: Apply sticky keys masks Fix a crash when switching VT if the wayland keyboard xkb_state is NULL
Created attachment 363213 [details] [review] [PATCH 14/21] clutter/evdev: implement toggle keys support toggle keys enables and disables now, as it should...
Created attachment 363214 [details] [review] [PATCH 15/21] clutter/evdev: implement mouse keys support
Created attachment 363215 [details] [review] [PATCH 16/21] clutter/x11: Add xkb accessibility helpers
Created attachment 363216 [details] [review] [PATCH 17/21] clutter/x11: Configure XKB accessibility
Created attachment 363217 [details] [review] [PATCH 18/21] core: Add MetaKbdA11yDialog
Created attachment 363218 [details] [review] [PATCH 19/21] core: implement MetaXkbA11yDialogDefault
Created attachment 363219 [details] [review] [PATCH 20/21] compositor: add vmethod to override kbd a11y dialog
Created attachment 363220 [details] [review] [PATCH 21/21] backends: invoke kbd accessibility dialog
Created attachment 363221 [details] [review] [PATCH gnome-shell] ui: Add KbdA11yDialog Change: Set the default button to what actually changed (either enable or disable)
Created attachment 363230 [details] [review] [PATCH gnome-shell] ui: Add KbdA11yDialog Change: reuse the "access-dialog" style instead of addign a new one, update POTFILES.in
Created attachment 363288 [details] [review] [PATCH 15/21] clutter/evdev: implement mouse keys support Change: rework the pointer motion, do not depend on key repeat but use the appropriate “mousekeys-init-delay“ and “mousekeys-accel-time“ to get a nice and smooth pointer acceleration using mouse keys.
Changes available in branch "keyboard-a11y" in github: https://github.com/ofourdan/mutter/tree/keyboard-a11y https://github.com/ofourdan/gnome-shell/tree/keyboard-a11y
Review of attachment 362663 [details] [review]: Makes sense to do that.
Review of attachment 362664 [details] [review]: This approach makes sense :).
Review of attachment 362665 [details] [review]: LGTM
Review of attachment 362666 [details] [review]: LGTM. too bad we can't share this code, but 2 lines are not worth a common parent ClutterBackend instance in src/backends/.
Review of attachment 362667 [details] [review]: Yep, we need something like this.
Review of attachment 362668 [details] [review]: Looks good! just one nit. ::: clutter/clutter/clutter-device-manager.c @@ +500,3 @@ + g_return_if_fail (CLUTTER_IS_DEVICE_MANAGER (device_manager)); + + device_manager->priv->kbd_a11y_settings = *settings; Should we maybe compare and bail out if settings are identical? I know you do some flag checks in later patches, but IMHO the earlier we cut pointless grinding the better.
Review of attachment 362669 [details] [review]: ::: clutter/clutter/clutter-device-manager.c @@ +213,3 @@ + * result of keyboard accessibilty's sticky keys operations. + */ + manager_signals[KBD_A11Y_MASK_CHANGED] = This signal makes sense to have. @@ +233,3 @@ + * keyboard accessibilty operations. + */ + manager_signals[KBD_A11Y_FLAGS_CHANGED] = but this other maybe not as much... I'll try to address my opinion on how should a11y dialogs work in the later patches.
Review of attachment 363207 [details] [review]: Looks mostly good, just the continued nit around a11y dialogs. ::: src/backends/meta-input-settings.c @@ +1213,3 @@ + { + if (settings_flags_pair[i].flag & what_changed) + g_settings_set_boolean (priv->a11y_settings, This is more nominally what I think shouldn't happen here. IMHO clutter should not be a source of settings changes. ClutterInputDevice configuration should reflect whatever settings affect the device at runtime, MutterInputSettings can influence ClutterInputDevices, but the opposite shouldn't happen.
(In reply to Carlos Garnacho from comment #83) > Review of attachment 362669 [details] [review] [review]: > [...] > but this other maybe not as much... I'll try to address my opinion on how > should a11y dialogs work in the later patches. It's not just about the dialog actually (although it's used for this as well), but the main purpose is to let higher layers know that the settings has changed internally because of "toggle keys", so the settings need to be saved in gsettings (I wouldn't want to add gsettings in clutter directly).
Review of attachment 363208 [details] [review]: LGTM
Review of attachment 363209 [details] [review]: ::: clutter/clutter/evdev/clutter-input-device-evdev.c @@ +280,3 @@ + clutter_input_device_evdev_bell_notify (); + + return FALSE; Nit: G_SOURCE_REMOVE is clearer. @@ +291,3 @@ + const ClutterKeyEvent *kb = b; + + return ka->hardware_keycode == kb->hardware_keycode ? 0 : 1; I know g_list_find_custom just cares of 0 vs !0, but doing: return kb->hardware_keycode - ka->hardware_keycode; is still just as correct and less prone to "wait, what?" moments :). @@ +375,3 @@ ClutterKbdA11ySettings *settings) { + if ((device->a11y_flags ^ settings->controls) & Would be nice here and for later patches that you have an intermediate changed_flags var that makes these checks clearer.
Review of attachment 363210 [details] [review]: ::: clutter/clutter/evdev/clutter-input-device-evdev.c @@ +375,3 @@ + device->debounce_key = ((ClutterKeyEvent *) event)->hardware_keycode; + device->debounce_timer = + clutter_threads_add_timeout (get_debounce_delay (CLUTTER_INPUT_DEVICE (device)), I think we can get multiple simultaneous timers running here. On key release start_bounce_keys() will be called, that adds another timeout without removing the first one. AFAICT this would result on the debounce_key being cleared sooner than expected.
Review of attachment 363211 [details] [review]: Looks good ::: clutter/clutter/evdev/clutter-input-device-evdev.c @@ +537,3 @@ + + if (device->stickykeys_depressed_mask && + device->a11y_flags & CLUTTER_A11Y_STICKY_KEYS_TWO_KEY_OFF) Extra () around the & condition please :). @@ +539,3 @@ + device->a11y_flags & CLUTTER_A11Y_STICKY_KEYS_TWO_KEY_OFF) + { + clear_stickykeys_event (event, device); Hmm, this is maybe the one place where it could make sense to alter configuration from the lower layers :(. I'll come up later with suggestions...
Review of attachment 363212 [details] [review]: Makes sense
Review of attachment 363213 [details] [review]: Suggestion: How about doing this handling (and the 2 simultaneous modifiers one to disable sticky keys) in gnome-shell? I think would be neat to handle this stuff so: - gnome-shell peeks on stage key events, triggers the dialogs and changes GSettings. - mutter reacts to GSettings changes and applies them to ClutterInputDevice - Clutter modifies the runtime keyboard behavior to reflect the settings. I think gnome-shell has all the means (besides maybe missing passive key grabs for x11, which should be set up by mutter) to track this a11y-dialog-triggering input, and also has the means to create dialogs without further Mutter plumbing :).
Review of attachment 363288 [details] [review]: Nice :). ::: clutter/clutter/evdev/clutter-input-device-evdev.c @@ +967,3 @@ + + /* We reschedule each time */ + return FALSE; Nit: G_SOURCE_REMOVE
Review of attachment 363215 [details] [review]: Yay! The only thing I see missing is moving the handling of XkbBellNotify events from src/x11/events.c to this helper. Can be adressed in a separate patch though.
Review of attachment 363216 [details] [review]: \o/
... And the remaining UI side patches are left ATM, awaiting for plumbing discussion.
(In reply to Carlos Garnacho from comment #91) > Review of attachment 363213 [details] [review] [review]: > > Suggestion: How about doing this handling (and the 2 simultaneous modifiers > one to disable sticky keys) in gnome-shell? > > I think would be neat to handle this stuff so: > - gnome-shell peeks on stage key events, triggers the dialogs and changes > GSettings. > - mutter reacts to GSettings changes and applies them to ClutterInputDevice > - Clutter modifies the runtime keyboard behavior to reflect the settings. > > I think gnome-shell has all the means (besides maybe missing passive key > grabs for x11, which should be set up by mutter) to track this > a11y-dialog-triggering input, and also has the means to create dialogs > without further Mutter plumbing :). But we'd need that plumbing for X11 anyway, because AccessX changes the controls itself. Without this plumbing, it means we need to add x11 code specifically in gnome-shell, which kinda defeats the idea of the clutter backends, no?
(In reply to Olivier Fourdan from comment #96) > > I think gnome-shell has all the means (besides maybe missing passive key > > grabs for x11, which should be set up by mutter) to track this > > a11y-dialog-triggering input, and also has the means to create dialogs > > without further Mutter plumbing :). > > But we'd need that plumbing for X11 anyway, because AccessX changes the > controls itself. > > Without this plumbing, it means we need to add x11 code specifically in > gnome-shell, which kinda defeats the idea of the clutter backends, no? Hmm, not sure why but I thought the shift key handling was done by g-s-d... In my head we could have a common implementation for both backends. If we can't opt out that behavior from AccessX in order to implement our own, then I'll resume review of the remaining patches tomorrow :). But still, Meta*Dialog infrastructure sounds a bit overkill to me :(. Maybe having gnome-shell listen to ClutterInputDevice::kbd-a11y-settings-changed, as obscure as that sounds?
(In reply to Carlos Garnacho from comment #97) > Hmm, not sure why but I thought the shift key handling was done by g-s-d... Err, nope. toggle keys is really an AccessX feature. > In my head we could have a common implementation for both backends. If we > can't opt out that behavior from AccessX in order to implement our own, then > I'll resume review of the remaining patches tomorrow :). We /could/ opt out by disabling toggle keys in AccessX configuration, but for X11, it's always a bit tricky because of grabs, if an X11 client has an active grab on the keyboard, mutter/gnome-shell won't get the events and won't be able to behave like the "real" toggle keys implementation in the Xserver does.
(In reply to Carlos Garnacho from comment #97) > [...] > But still, Meta*Dialog infrastructure sounds a bit overkill to me :(. Maybe Probably, one (small) benefit though is the basic, default dialog that shows up when the shell doesn;t implement the dialog (thinking of other shells that use libmutter) > having gnome-shell listen to ClutterInputDevice::kbd-a11y-settings-changed, > as obscure as that sounds? That means gnome-shell needs to know about the ClutterInputDevice, i.e. retrieve it from the default clutter device manager and listen to the signal. We still need the signal and the gsettings handler in mutter, as the dialog acts like "this was changed, keep it or revert it?" (i.e. it occurs after the change was triggered by the toggle keys internally). Main concern for me is I am not familiar with gnome-shell and even less with Javascript...
Hey Carlos, following out discussions on irc in #gnome-shell about moving toggle-keys support to the UI instead of handling it directly in clutter, I have rebased my patches, removing toggle-keys support and amending the remaining ones based on your reviews. That removes quite a few patches and also simplify the x11 support as we can get rid of the event filter as we don't need to monitor control chnages (no toggle keys, no two-keys off) For testing purpose, I created a new branch "keyboard-a11y-no-togglekeys" here: https://github.com/ofourdan/mutter/tree/keyboard-a11y-no-togglekeys
Created attachment 363713 [details] [review] [PATCH] ui: Add keyboard accessibility dialog Show a dialog informing the user each time the keyboard accessibility flags are changed by one of the clutter backends (either from toggle keys or two-keys-off modifiers). Changes: We don't use a compositor plugin anymore (thus getting rid of 4 patches), g-s connects to the ClutterDevineMAnager signal directly instead.
Also amened the patches following the reviews, uploaded to the following branches on github: mutter: https://github.com/ofourdan/mutter/tree/keyboard-a11y-no-dialog gnome-shell: https://github.com/ofourdan/gnome-shell/tree/keyboard-a11y
Created attachment 363715 [details] [review] [PATCH 06/17] clutter: add hooks for kbd a11y configuration
Created attachment 363716 [details] [review] [PATCH 07/17] clutter: add keyboard accessibility signals
Created attachment 363717 [details] [review] [PATCH 08/17] backends: configure keyboard accessibility
Created attachment 363719 [details] [review] [PATCH 09/17] clutter/evdev: Implement a11y configuration hooks
Created attachment 363722 [details] [review] [PATCH 10/17] clutter/evdev: implement slow keys support
Created attachment 363724 [details] [review] [PATCH 11/17] clutter/evdev: implement bounce keys support
Created attachment 363725 [details] [review] [PATCH 12/17] clutter/evdev: implement sticky keys support
Created attachment 363726 [details] [review] [PATCH 13/17] wayland/keyboard: Apply sticky keys masks
Created attachment 363727 [details] [review] [PATCH 14/17] clutter/evdev: implement toggle keys support
Created attachment 363728 [details] [review] [PATCH 15/17] clutter/evdev: implement mouse keys support
Created attachment 363730 [details] [review] [PATCH 16/17] clutter/x11: Add xkb accessibility helpers
Created attachment 363731 [details] [review] [PATCH 17/17] clutter/x11: Configure XKB accessibility
Created attachment 363732 [details] [review] [PATCH gnome-shell] ui: Add keyboard accessibility dialog
Created attachment 363792 [details] [review] [PATCH 11/17] clutter/evdev: implement bounce keys support Change: Oops, forgot to set the timer value back to 0 in the trigger, so we could possibly try to remove a timeout that was already fired.
Review of attachment 363715 [details] [review]: LGTM ::: clutter/clutter/clutter-device-manager.c @@ +502,3 @@ + a->mousekeys_init_delay == b->mousekeys_init_delay && + a->mousekeys_max_speed == b->mousekeys_max_speed && + a->mousekeys_accel_time == b->mousekeys_accel_time); FWIW, I was kind of expecting memcmp, but this floats my boat :).
Review of attachment 363716 [details] [review]: Yup. Agreed we need the 2 signals.
Review of attachment 363717 [details] [review]: Agreed we must store settings on exceptional situations. The rest looks good.
Review of attachment 363719 [details] [review]: LGTM
Review of attachment 363722 [details] [review]: LGTM
Review of attachment 363792 [details] [review]: Looks good :), just a minor nit. ::: clutter/clutter/evdev/clutter-input-device-evdev.c @@ +455,3 @@ + if (changed_flags & (CLUTTER_A11Y_KEYBOARD_ENABLED | CLUTTER_A11Y_BOUNCE_KEYS_ENABLED)) + device->debounce_key = 0; Should this also clear the timeout?
Review of attachment 363725 [details] [review]: Looks good!
Review of attachment 363726 [details] [review]: *thumbs up*
Review of attachment 363727 [details] [review]: Yep. Agreed we can't do this in stead of AccessX on X11, so similar handling in evdev is due.
Review of attachment 363728 [details] [review]: All looks correct :).
Review of attachment 363730 [details] [review]: Looks good :). ::: clutter/clutter/x11/clutter-xkb-a11y-x11.c @@ +123,3 @@ + +static ClutterX11FilterReturn +xkb_a11y_event_filter (XEvent *xevent, It seems the suggestion to move XkbBellNotify handling here from src/core/events.c went forgotten with all other noise... It's fine to do that in later commits.
Review of attachment 363731 [details] [review]: *nod*
Review of attachment 363732 [details] [review]: You're updating the gnome-shell-sass submodule in this commit :). Probably unintended. Other than that, I'd say it looks good. The transition from notification to dialog makes some sense to me given this becomes shell UI. ::: js/ui/kbdA11yDialog.js @@ +24,3 @@ + let deviceManager = Clutter.DeviceManager.get_default(); + deviceManager.connect('kbd-a11y-flags-changed', + Lang.bind(this, this._ShowKbdA11yDialog)) Missing semicolon here. JS is amazing :p
Comment on attachment 363732 [details] [review] [PATCH gnome-shell] ui: Add keyboard accessibility dialog Err, was meaning to set this one to "reviewed", probably better to let Florian have a look.
(In reply to Carlos Garnacho from comment #129) > Review of attachment 363732 [details] [review] [review]: > > You're updating the gnome-shell-sass submodule in this commit :). Probably > unintended. Completely unintended, yet I dunno how to avoid it (I tried)... I reckon Florian had the same issue in commit dff3e4e0 > Other than that, I'd say it looks good. The transition from notification to > dialog makes some sense to me given this becomes shell UI. > > ::: js/ui/kbdA11yDialog.js > @@ +24,3 @@ > + let deviceManager = Clutter.DeviceManager.get_default(); > + deviceManager.connect('kbd-a11y-flags-changed', > + Lang.bind(this, this._ShowKbdA11yDialog)) > > Missing semicolon here. JS is amazing :p Wow, and it didn't even complain at runtime, amazing it is! :)
(In reply to Carlos Garnacho from comment #122) > Review of attachment 363792 [details] [review] [review]: > > Looks good :), just a minor nit. > > ::: clutter/clutter/evdev/clutter-input-device-evdev.c > @@ +455,3 @@ > > + if (changed_flags & (CLUTTER_A11Y_KEYBOARD_ENABLED | > CLUTTER_A11Y_BOUNCE_KEYS_ENABLED)) > + device->debounce_key = 0; > > Should this also clear the timeout? Not needed actually, as we set the debounce key to 0, we can let the timeout trigger, it won't be much of a problem (the timeout will simply redo the debounce_key = 0)
Created attachment 363829 [details] [review] [PATCH gnome-shell] ui: Add keyboard accessibility dialog Change: Add the missing semicolon and ignored the subproject gnome-shell.css
Comment on attachment 362663 [details] [review] [PATCH 01/21] wayland: Disable AccessX in Xwayland attachment 362663 [details] [review] pushed to git master as commit 0461eed - wayland: Disable AccessX in Xwayland
Comment on attachment 362664 [details] [review] [PATCH 02/21] clutter/backend: Add bell-notify attachment 362664 [details] [review] pushed to git master as commit 2ffe597 - clutter/backend: Add bell-notify
Comment on attachment 362665 [details] [review] [PATCH 03/21] backend/x11: implement bell-notify attachment 362665 [details] [review] pushed to git master as commit 428af6d - backend/x11: implement bell-notify
Comment on attachment 362666 [details] [review] [PATCH 04/21] backend/native: implement bell-notify attachment 362666 [details] [review] pushed to git master as commit dc0fc65 - backend/native: implement bell-notify
Comment on attachment 362667 [details] [review] [PATCH 05/21] clutter-main: Add hooks to plug kbd accessibility attachment 362667 [details] [review] pushed to git master as commit 7d5e08c - clutter-main: Add hooks to plug kbd accessibility
Comment on attachment 363715 [details] [review] [PATCH 06/17] clutter: add hooks for kbd a11y configuration attachment 363715 [details] [review] pushed to git master as commit 32305b4 - clutter: add hooks for kbd a11y configuration
Comment on attachment 363716 [details] [review] [PATCH 07/17] clutter: add keyboard accessibility signals attachment 363716 [details] [review] pushed to git master as commit aa73504 - clutter: add keyboard accessibility signals
Comment on attachment 363717 [details] [review] [PATCH 08/17] backends: configure keyboard accessibility attachment 363717 [details] [review] pushed to git master as commit 333b5d1 - backends: configure keyboard accessibility
Comment on attachment 363719 [details] [review] [PATCH 09/17] clutter/evdev: Implement a11y configuration hooks attachment 363719 [details] [review] pushed to git master as commit 76b064c - clutter/evdev: Implement a11y configuration hooks
Comment on attachment 363722 [details] [review] [PATCH 10/17] clutter/evdev: implement slow keys support attachment 363722 [details] [review] pushed to git master as commit fa28481 - clutter/evdev: implement slow keys support
Comment on attachment 363725 [details] [review] [PATCH 12/17] clutter/evdev: implement sticky keys support attachment 363725 [details] [review] pushed to git master as commit 06d976e - clutter/evdev: implement sticky keys support
Comment on attachment 363726 [details] [review] [PATCH 13/17] wayland/keyboard: Apply sticky keys masks attachment 363726 [details] [review] pushed to git master as commit 3fc2ea8 - wayland/keyboard: Apply sticky keys masks
Comment on attachment 363727 [details] [review] [PATCH 14/17] clutter/evdev: implement toggle keys support attachment 363727 [details] [review] pushed to git master as commit 356b4b0 - clutter/evdev: implement toggle keys support
Comment on attachment 363728 [details] [review] [PATCH 15/17] clutter/evdev: implement mouse keys support attachment 363728 [details] [review] pushed to git master as commit bdf3f49 - clutter/evdev: implement mouse keys support
Comment on attachment 363730 [details] [review] [PATCH 16/17] clutter/x11: Add xkb accessibility helpers attachment 363730 [details] [review] pushed to git master as commit 32c22d3 - clutter/x11: Add xkb accessibility helpers
Comment on attachment 363731 [details] [review] [PATCH 17/17] clutter/x11: Configure XKB accessibility attachment 363731 [details] [review] pushed to git master as commit 03be6b6 - clutter/x11: Configure XKB accessibility
Comment on attachment 363792 [details] [review] [PATCH 11/17] clutter/evdev: implement bounce keys support attachment 363792 [details] [review] pushed to git master as commit 96ebd1c - clutter/evdev: implement bounce keys support
(In reply to Olivier Fourdan from comment #131) > (In reply to Carlos Garnacho from comment #129) > > You're updating the gnome-shell-sass submodule in this commit :). Probably > > unintended. > > I reckon Florian had the same issue in commit dff3e4e0 I'm sure it has happened to me in the past, but in this particular case the submodule update was intended :-)
(In reply to Florian Müllner from comment #151) > I'm sure it has happened to me in the past, but in this particular case the > submodule update was intended :-) Hehe, well, I removed the gnome-shell-sass submodule from the latest iteration of the patch (attachment 363829 [details] [review]).
Review of attachment 363829 [details] [review]: I haven't tested this (yet), but the code looks good to me except for some style nits: ::: js/ui/kbdA11yDialog.js @@ +2,3 @@ +const Gio = imports.gi.Gio; +const GObject = imports.gi.GObject; +const Gtk = imports.gi.Gtk; Unused import @@ +4,3 @@ +const Gtk = imports.gi.Gtk; +const Lang = imports.lang; +const Meta = imports.gi.Meta; Dto @@ +5,3 @@ +const Lang = imports.lang; +const Meta = imports.gi.Meta; +const Shell = imports.gi.Shell; Dto @@ +6,3 @@ +const Meta = imports.gi.Meta; +const Shell = imports.gi.Shell; +const St = imports.gi.St; Dto @@ +24,3 @@ + let deviceManager = Clutter.DeviceManager.get_default(); + deviceManager.connect('kbd-a11y-flags-changed', + Lang.bind(this, this._ShowKbdA11yDialog)); Style nit: camelCase, not CamelCase @@ +27,3 @@ + }, + + _ShowKbdA11yDialog: function(deviceManager, new_flags, what_changed) { Style nit: Use camel case for variable names instead of underscores (Exception: Newer JS has a shortcut for setting an object property from a variable with the same name ({ foo } instead of { foo: foo }); if a variable needs to use an underscore to make that work, that's fine: let w = new Gtk.Window({ default_width, default_height }); ) @@ +63,3 @@ + + dialog.addButton({ label: enabled ? _("Leave On") : _("Turn On"), + action: () => { Indentation @@ +70,3 @@ + + dialog.addButton({ label: enabled ? _("Turn Off") : _("Leave Off"), + action: () => { Dto.
Created attachment 364893 [details] [review] PATCH gnome-shell] ui: Add keyboard accessibility dialog Updated patch ater review.
Humble ping, could you do a review of my updated patch (attachment 364893 [details] [review]) please, so we can close this bug and proceed with dependent bug 790121?
Any chance to have this review? Note: this cannot be tested in “nested” as keyboard a11y is not triggered in the client but the compositor (or the Xserver).
(In reply to Olivier Fourdan from comment #156) > Note: this cannot be tested in “nested” Sorry for the silly question, but: What exactly are the actions that should trigger the dialog? I've tried toggling the various a11y actions, but apparently that's not it (on either X11 or wayland) ...
(In reply to Florian Müllner from comment #157) > (In reply to Olivier Fourdan from comment #156) > > Note: this cannot be tested in “nested” > > Sorry for the silly question, but: What exactly are the actions that should > trigger the dialog? I've tried toggling the various a11y actions, but > apparently that's not it (on either X11 or wayland) ... One simple test is to run is to trigger sticky keys: 1. Run that against a current mutter from master as a display server (not nested) 2. Press the shift key 5 times in a row Note 1: if you don't trigger that in Xorg right now, it means that you have the keyboard a11y features turned off (in which case this would not work even in Wayland or Xorg with these patches either). Note 2: this dialog in gnome-shell is a duplication of the one found in gnome-settings-daemon, meaning that you may actually trigger *two* dialogs unless you also apply the patch to remove the feature from g-s-d (bug 790121) Thanks!
You can c(In reply to Olivier Fourdan from comment #158) > Note 1: if you don't trigger that in Xorg right now, it means that you have > the keyboard a11y features turned off (in which case this would not work > even in Wayland or Xorg with these patches either). > Just a quick addition, you can quickly check that with dconf: $ dconf read /org/gnome/desktop/a11y/keyboard/enable true $ dconf read /org/gnome/desktop/a11y/keyboard/togglekeys-enable true
Meh, so I'm to blame twice - the gsettings key was off, and I somehow remembered pressing shift for 5 seconds, not 5 times in a row ...
(In reply to Florian Müllner from comment #160) > I somehow remembered pressing shift for 5 seconds, not 5 times in a row ... Double-meh, the patch even spells it out :-(
(In reply to Florian Müllner from comment #160) > Meh, so I'm to blame twice - the gsettings key was off, and I somehow > remembered pressing shift for 5 seconds, not 5 times in a row ... You can also keep the shift key pressed for 8 seconds, that triggers slowkeys.
Review of attachment 364893 [details] [review]: Two nits: - unintentional submodule update - a quick grep suggests this is the only modal dialog (apart from the restart message) that doesn't support canceling with Escape (but I'm not exactly sure what the right action for that would be) ::: js/ui/kbdA11yDialog.js @@ +52,3 @@ + + let icon = new Gio.ThemedIcon({ name: 'preferences-desktop-accessibility-symbolic' }); + let contentParams = { icon, title, body, styleClass: 'access-dialog' }; There's now quite a number of dialogs that use the same spacing, so maybe it's time to replace the access-dialog, audio-device-selection-dialog, geolocation-dialog and inhibit-shortcuts-dialog classes with a common simple-message-dialog or something.
Created attachment 366611 [details] [review] [PATCH gnome-shell] ui: Add keyboard accessibility dialog (In reply to Florian Müllner from comment #163) > Review of attachment 364893 [details] [review] [review]: Thanks for the review! > Two nits: > - unintentional submodule update > - a quick grep suggests this is the only modal dialog > (apart from the restart message) that doesn't support > canceling with Escape (but I'm not exactly sure what > the right action for that would be) Sure. Updated patch attached, Escape should now revert to the old setting value (i.e. if the toggle key enabled the feature, Escape disables it, and vice-versa) > ::: js/ui/kbdA11yDialog.js > @@ +52,3 @@ > + > + let icon = new Gio.ThemedIcon({ name: > 'preferences-desktop-accessibility-symbolic' }); > + let contentParams = { icon, title, body, styleClass: > 'access-dialog' }; > > There's now quite a number of dialogs that use the same spacing, so maybe > it's time to replace the access-dialog, audio-device-selection-dialog, > geolocation-dialog and inhibit-shortcuts-dialog classes with a common > simple-message-dialog or something. Well, I'd rather keep that as a separate patch from this one (preferably done by some else, considering that JS is not exactly my field of expertise ^_~).
Review of attachment 366611 [details] [review]: (In reply to Olivier Fourdan from comment #164) > Well, I'd rather keep that as a separate patch from this one (preferably > done by some else) Fair enough :-)
Comment on attachment 366611 [details] [review] [PATCH gnome-shell] ui: Add keyboard accessibility dialog attachment 366611 [details] [review] pushed to git master as commit bc5be10 - ui: Add keyboard accessibility dialog
(In reply to Florian Müllner from comment #165) > Fair enough :-) Thanks!