After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 624916 - Universal Access status indicator
Universal Access status indicator
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-07-21 12:08 UTC by Giovanni Campagna
Modified: 2010-09-16 19:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add Universal Access status indicator (12.01 KB, patch)
2010-07-21 12:09 UTC, Giovanni Campagna
reviewed Details | Review
Universal Access indicator (revised) (13.55 KB, patch)
2010-07-21 22:43 UTC, Giovanni Campagna
reviewed Details | Review
Universal Access indicator (the third!) (13.29 KB, patch)
2010-07-22 22:33 UTC, Giovanni Campagna
reviewed Details | Review
Same patch, with latest comments (14.01 KB, patch)
2010-09-14 20:13 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2010-07-21 12:08:26 UTC
Per bug 624361, the status area should contain the Universal Access indicator (http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/UniversalAccess). I implemented it and am proposing my code along with some notes:

1) This indicator, like the corresponding Gtk dialog, relies on gnome-settings-daemon to actually expose modified values to X and applications.

2) Because g-s-d has not yet been ported to GSettings, this patch requires GConf for all settings, except for the Gtk theme and icon theme, for which a schema exists in the gsettings-desktop-schemas module.
This means that the High Contrast setting is not actually recognized. I can revert to GConf if g-s-d is not ported in the GNOME 3.0 timeframe.

3) The mockup is missing the on screen keyboard. Is this deliberate?
Anyway, I added it for completeness.

4) The mockup says generically "Zoom". I splitted it into "Large text" (increase DPI) and "Zoom screen" (magnifier)

5) The font DPI setting is not picked up by many applications: no effect on Gnome Shell, no effect on KDE apps (with Gtk style), no effect on Firefox with custom theme, only menu resized (but not the text inside) in OpenOffice.org Writer.

6) The AT settings (screen keyboard, magnifier, screen reader) are hooked to /desktop/gnome/applications/at/screen_*_enabled, like the g-s-d dialog does, except that in g-s-d the relevant checkboxes are hidden, and setting those GConf keys has no effect at all.
(Actually, the checkboxes are shown and do work, but only in gdm, not in a desktop session).
Comment 1 Giovanni Campagna 2010-07-21 12:09:01 UTC
Created attachment 166271 [details] [review]
Add Universal Access status indicator

Introduce the Universal Access status indicator as designed, modeled
after the similar UI provided by g-s-d. This indicator allows the user
to change rapidly the keyboard and mouse behaviour (sticky keys, slow
keys, bounce keys, mouse keys), as well as the enabled ATs (magnifier,
screen reader, screen keyboard) and the HighContrast Gtk theme.
Comment 2 Giovanni Campagna 2010-07-21 12:10:31 UTC
I forgot to add that this depends on bug 611880 and bug 621705
Comment 3 Dan Winship 2010-07-21 16:13:46 UTC
(In reply to comment #2)
> I forgot to add that this depends on bug 611880 and bug 621705

bug 621880, not 611880
Comment 4 Dan Winship 2010-07-21 19:14:08 UTC
(In reply to comment #0)
> 3) The mockup is missing the on screen keyboard. Is this deliberate?

Probably not...

> 4) The mockup says generically "Zoom". I splitted it into "Large text"
> (increase DPI) and "Zoom screen" (magnifier)

I think "Large text" is handled by the choice of gtk theme. Tweaking the DPI seems like a bad idea. Unless you copied it from some existing a11y UI?

If you're going to tweak the DPI, you need to set it back to the correct original value if the user toggles the switch on and then back off.

> 6) The AT settings (screen keyboard, magnifier, screen reader) are hooked to
> /desktop/gnome/applications/at/screen_*_enabled, like the g-s-d dialog does,
> except that in g-s-d the relevant checkboxes are hidden, and setting those
> GConf keys has no effect at all.

In the desktop session, this is currently controlled by the Accessibility pane of the Preferred Applications capplet.

(See also the questions I added to the wiki page.)
Comment 5 Dan Winship 2010-07-21 19:57:18 UTC
Comment on attachment 166271 [details] [review]
Add Universal Access status indicator

>+++ b/js/ui/accessibility.js

needs a less-generic filename for this. Or maybe we should have a subdirectory just for the status area icon implementations.

>+const Signals = imports.signals;
>+const DBus = imports.dbus;

each group of imports should be alphabetized

>+const HIGH_CONTRAST_THEME = "HighContrast";

for "Inverted Colors" it would have to be "HighContrastInverse". We may need another menu item for "HighContrast" as well.

Of course, "HighContrastInverse" really means "White text on black", which is the default for the shell, so you'd select "Inverted Colors" to indicate that you *don't* want to invert the shell colors? Hm... We probably need to make the items say "High Contrast (White on Black)" and "High Contrast (Black on White)" or something. And once we figure out a11y theming for the shell (bug 618888) it needs to affect both shell and gtk.

(Of course, if there are three possibilities (W-on-B, B-on-W, neither) then we can't do it with switchbutton menu items.)

>+        // new in Gnome Shell

that comment will grow old fast. :) just get rid of it.

>+        client.add_dir(KEY_A11Y_DIR, GConf.ClientPreloadType.PRELOAD_ONELEVEL, null);

you should do the add_dir() calls before creating all the menu items

>+        this._a11y_dir_cnxn = client.notify_add(KEY_A11Y_DIR, 

if you're going to bother saving the IDs, I suppose you should connect to 'destroy' on the menubutton actor, and clean things up at that point.

>+        let widget = this._buildItemExtended(string, on_get,
>+                function(enabled) {

indented too far. Likewise in some other places.

>+        let highContrast = this._buildItemExtended(_("Inverted colors"),
>+                function () {
>+                    return hasHC;
>+                },

that's only true at startup... either you should rename on_get() to on_init() or something, or else make the on_get() method always look up the correct answer.

>+                function () {
>+                    return settings.is_writable(KEY_GTK_THEME);

&& settings.is_writable(KEY_ICON_THEME)

>+        settings.connect('changed::'+KEY_GTK_THEME, function() {

spaces around the "+" here and in some other places.

>+            let value = settings.get_string(KEY_GTK_THEME);
>+            if (value == HIGH_CONTRAST_THEME)
>+                highContrast.setToggleState(true);
>+            else {
>+                highContrast.setToggleState(false);
>+                gtkTheme = value;
>+            }
>+        });
>+        settings.connect('changed::'+KEY_ICON_THEME, function() {
>+            let value = settings.get_string(KEY_ICON_THEME);
>+            if (value != HIGH_CONTRAST_THEME)
>+                iconTheme = value;
>+        });

hm... maybe we do need tri-state toggles...

>@@ -45,6 +45,7 @@ const STANDARD_TRAY_ICON_IMPLEMENTATIONS = {
>  * };
>  */
> const STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION = {
>+    'a11y': imports.ui.accessibility.ATIndicator
> };

can you remove the example in the comment above this as well, since it's unnecessary now.

>+  <autotools id="gsettings-desktop-schemas">
>+    <branch repo="git.gnome.org" module="gsettings-desktop-schemas" />
>+  </autotools>

We also need to fix up the gnome-shell wrapper to set $XDG_DATA_DIRS or whatever; right now the shell can only find the schemas if you do "jhbuild run ./src/gnome-shell", which isn't supposed to be necessary.
Comment 6 Giovanni Campagna 2010-07-21 21:47:26 UTC
(In reply to comment #4)
> (In reply to comment #0)
> > 3) The mockup is missing the on screen keyboard. Is this deliberate?
> 
> Probably not...
> 
> > 4) The mockup says generically "Zoom". I splitted it into "Large text"
> > (increase DPI) and "Zoom screen" (magnifier)
> 
> I think "Large text" is handled by the choice of gtk theme. Tweaking the DPI
> seems like a bad idea. Unless you copied it from some existing a11y UI?

From gnome-settings-daemon (you get it by setting "use keybindings to enable accessibility features" or something like that in the keyboard capplet)

> If you're going to tweak the DPI, you need to set it back to the correct
> original value if the user toggles the switch on and then back off.

My code does what g-s-d does, but I can enable resetting the GConf original value (instead of the Xorg native)

> > 6) The AT settings (screen keyboard, magnifier, screen reader) are hooked to
> > /desktop/gnome/applications/at/screen_*_enabled, like the g-s-d dialog does,
> > except that in g-s-d the relevant checkboxes are hidden, and setting those
> > GConf keys has no effect at all.
> 
> In the desktop session, this is currently controlled by the Accessibility pane
> of the Preferred Applications capplet.

So what GConf (GSettings) keys do we hook up?
/desktop/gnome/applications/at/mobility/enable only enables orca, not its features (orca is braille, screen reader, screen magnifier...)

> (See also the questions I added to the wiki page.)

1) The *Keys are set on XKB at any time and take effect immediately, same for visual bell and high contrast theme. The screen reader needs the at-spi infrastructure though, thus needs a logout.

2.1) Definitely the user should not lose its theme, expecially if the account is shared.
We could go for a "secondary-gtk-theme", or instead have "gtk-theme", "gtk-highcontrast-theme" ("gtk-hc-theme"?), "gtk-highcontrast-enabled", and have g-s-d expose the right gtk theme in Xsettings

2.2) 2.3) skipped...

2.4) Visual alerts toggles the visual bell setting. It does not change the visual bell type the user set using the g-c-c panel.


(In reply to comment #5)
> (From update of attachment 166271 [details] [review])
> >+++ b/js/ui/accessibility.js
> 
> needs a less-generic filename for this. Or maybe we should have a subdirectory
> just for the status area icon implementations.

Uhm... js/ui/system-status/accessibility.js ?
> 
> >+const Signals = imports.signals;
> >+const DBus = imports.dbus;
> 
> each group of imports should be alphabetized

Ok

> >+const HIGH_CONTRAST_THEME = "HighContrast";
> 
> for "Inverted Colors" it would have to be "HighContrastInverse". We may need
> another menu item for "HighContrast" as well.
> 
> Of course, "HighContrastInverse" really means "White text on black", which is
> the default for the shell, so you'd select "Inverted Colors" to indicate that
> you *don't* want to invert the shell colors? Hm... We probably need to make the
> items say "High Contrast (White on Black)" and "High Contrast (Black on White)"
> or something. And once we figure out a11y theming for the shell (bug 618888) it
> needs to affect both shell and gtk.

You could have
1) "High contrast" boolean setting on the menu, and inside the g-c-c panel you set which one
2) "High contrast" as a menu with children "B on W", "W on B", "disable" (what about other high contrast color combinations? Windows for example has a lot of them)
3) Two switch items "High Contrast (White on Black)" and "High Contrast (Black on White)" - setting one on sets off the other, but not the other way round.

> (Of course, if there are three possibilities (W-on-B, B-on-W, neither) then we
> can't do it with switchbutton menu items.)
> 
> >+        // new in Gnome Shell
> 
> that comment will grow old fast. :) just get rid of it.

It was to distinguish what is shared functionality between g-s-d/gsd-a11y-preferences-dialog.c and gnome-shell/accessibility.js, and what needs to be backported

> >+        client.add_dir(KEY_A11Y_DIR, GConf.ClientPreloadType.PRELOAD_ONELEVEL, null);
> 
> you should do the add_dir() calls before creating all the menu items

Ok (copied from g-s-d)

> >+        this._a11y_dir_cnxn = client.notify_add(KEY_A11Y_DIR, 
> 
> if you're going to bother saving the IDs, I suppose you should connect to
> 'destroy' on the menubutton actor, and clean things up at that point.

Copied again (and I see no reason why you would ever destroy the item, rather than hide it)

> >+        let widget = this._buildItemExtended(string, on_get,
> >+                function(enabled) {
> 
> indented too far. Likewise in some other places.

Ok

> >+        let highContrast = this._buildItemExtended(_("Inverted colors"),
> >+                function () {
> >+                    return hasHC;
> >+                },
> 
> that's only true at startup... either you should rename on_get() to on_init()
> or something, or else make the on_get() method always look up the correct
> answer.

Will rename to on_init in this._buildItemExtended, but will keep on_get on the others.
 
> >+                function () {
> >+                    return settings.is_writable(KEY_GTK_THEME);
> 
> && settings.is_writable(KEY_ICON_THEME)

g-s-d did not check for this, and actually even with a mandatory icon theme it would be useful to set the high contrast Gtk theme

> >+        settings.connect('changed::'+KEY_GTK_THEME, function() {
> 
> spaces around the "+" here and in some other places.

Ok

> >+            let value = settings.get_string(KEY_GTK_THEME);
> >+            if (value == HIGH_CONTRAST_THEME)
> >+                highContrast.setToggleState(true);
> >+            else {
> >+                highContrast.setToggleState(false);
> >+                gtkTheme = value;
> >+            }
> >+        });
> >+        settings.connect('changed::'+KEY_ICON_THEME, function() {
> >+            let value = settings.get_string(KEY_ICON_THEME);
> >+            if (value != HIGH_CONTRAST_THEME)
> >+                iconTheme = value;
> >+        });
> 
> hm... maybe we do need tri-state toggles...
> 
> >@@ -45,6 +45,7 @@ const STANDARD_TRAY_ICON_IMPLEMENTATIONS = {
> >  * };
> >  */
> > const STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION = {
> >+    'a11y': imports.ui.accessibility.ATIndicator
> > };
> 
> can you remove the example in the comment above this as well, since it's
> unnecessary now.

Ok.

> >+  <autotools id="gsettings-desktop-schemas">
> >+    <branch repo="git.gnome.org" module="gsettings-desktop-schemas" />
> >+  </autotools>
> 
> We also need to fix up the gnome-shell wrapper to set $XDG_DATA_DIRS or
> whatever; right now the shell can only find the schemas if you do "jhbuild run
> ./src/gnome-shell", which isn't supposed to be necessary.

It should be enough to set XDG_DATA_DIRS=${XDG_DATA_DIRS:-"/usr/share:/usr/local/share"}
Comment 7 Giovanni Campagna 2010-07-21 22:43:55 UTC
Created attachment 166340 [details] [review]
Universal Access indicator (revised)

Fixed style suggestions and moved everything to js/ui/system_status
Comment 8 Dan Winship 2010-07-22 12:56:38 UTC
(In reply to comment #6)
> So what GConf (GSettings) keys do we hook up?
> /desktop/gnome/applications/at/mobility/enable only enables orca, not its
> features (orca is braille, screen reader, screen magnifier...)

I don't know. I guess the current design of the menu depends on the new capplet design (linked from the wiki page) being implemented as well, and the GSettings port therefore being done in a certain way to facilitate that...

I guess for anything that doesn't currently seem to be implementable, just leave the menu items disabled.

> > (See also the questions I added to the wiki page.)
> 
> 1) The *Keys are set on XKB at any time and take effect immediately...

sorry, I wasn't clear. I meant, "see the questions *that I asked the designers* on the wiki page". I wasn't expecting you to answer them, I was warning you of missing pieces in the design.

> > if you're going to bother saving the IDs, I suppose you should connect to
> > 'destroy' on the menubutton actor, and clean things up at that point.
> 
> Copied again (and I see no reason why you would ever destroy the item, rather
> than hide it)

OK, but if you're not ever going to detach the signal handlers, then don't save the IDs.
Comment 9 Dan Winship 2010-07-22 16:06:08 UTC
Comment on attachment 166340 [details] [review]
Universal Access indicator (revised)

as mentioned above, please disable any of the menu items where we aren't sure how to make them work.

>+  js/ui/system_status/Makefile

hm... How about just js/ui/status/. "System" is pretty much implied by the fact that this is the shell source tree.

>+const DBus = imports.dbus;

sorry, the "plain" (imports.foo) and GI (imports.gi.foo) imports should be sorted together as one group, followed by the "local" (imports.ui, imports.misc) imports as a second group. (http://live.gnome.org/GnomeShell/StyleGuide).

(The rule there was written before we started using gettext, so it doesn't take that into account, but the consensus seems to be that that comes last, separately, because of the weird syntax there.)

>+        let width_dpi = (screen.get_width() / (screen.get_width_mm() / 25.4));

the g-s-d code does a check for get_width_mm()/get_height_mm() returning a non-0 value. Your code would throw an exception in that case. (I think it returns 0 if the video driver doesn't know the size of the monitor.)

>+        if (width_dpi < DPI_LOW_REASONABLE_VALUE
>+                || width_dpi > DPI_HIGH_REASONABLE_VALUE

The || should be indented to the same point as the "w" in the previous line.
(Yes, this makes them indented to the same spot as the body of the if. Yes this is annoying.)

>+        PanelMenu.SystemStatusButton.prototype._init.call(this, 'preferences-desktop-accessibility', null);

SystemStatusButton._init() only takes 2 arguments

>+        let widget = this._buildItemExtended(_("Large text"), on_get,
>+                function (enabled) {
>+                    if (enabled)
>+                        client.set_float(KEY_FONT_DPI, DPI_FACTOR_LARGE * getDPIFromX());
>+                    else
>+                        client.unset(KEY_FONT_DPI);

As I said before, if the user toggles and then untoggles this (or the theme setting) within the same session, we should revert to the previously-set value, not the default. You don't need to store the previous value in gconf though; this is just an "undo" feature ("Hm... what does this setting do? [Toggle] AAAAGH! [Toggle back]"). If the user switches to Large text, and then a month later switches back, it's ok to fall back to the default value.

>+                'XDG_DATA_DIRS'       : '@datadir@:' + (os.environ.get('XDG_DATA_DIRS') or '/usr/share:/usr/local/share'),

I committed this part separately this morning (although with /usr/local/share first, because that's what the spec says).

>+  <autotools id="gsettings-desktop-schemas">
>+    <branch repo="git.gnome.org" module="gsettings-desktop-schemas" />
>+  </autotools>

You will need to add <dep package="gsettings-desktop-schemas"> to the gnome-shell module as well.
Comment 10 Giovanni Campagna 2010-07-22 16:31:28 UTC
(In reply to comment #9)
> (From update of attachment 166340 [details] [review])
> as mentioned above, please disable any of the menu items where we aren't sure
> how to make them work.

Do they include On Screen Keyboard? That does work (after logout / login)

> >+  js/ui/system_status/Makefile
> 
> hm... How about just js/ui/status/. "System" is pretty much implied by the fact
> that this is the shell source tree.

Ok

> >+const DBus = imports.dbus;
> 
> sorry, the "plain" (imports.foo) and GI (imports.gi.foo) imports should be
> sorted together as one group, followed by the "local" (imports.ui,
> imports.misc) imports as a second group.
> (http://live.gnome.org/GnomeShell/StyleGuide).
> 
> (The rule there was written before we started using gettext, so it doesn't take
> that into account, but the consensus seems to be that that comes last,
> separately, because of the weird syntax there.)

Ok

> >+        let width_dpi = (screen.get_width() / (screen.get_width_mm() / 25.4));
> 
> the g-s-d code does a check for get_width_mm()/get_height_mm() returning a
> non-0 value. Your code would throw an exception in that case. (I think it
> returns 0 if the video driver doesn't know the size of the monitor.)

Except that C code gets SIGFPE for division by 0, while JS code gets Infinity, which is obviously outside the reasonable interval.

> >+        if (width_dpi < DPI_LOW_REASONABLE_VALUE
> >+                || width_dpi > DPI_HIGH_REASONABLE_VALUE
> 
> The || should be indented to the same point as the "w" in the previous line.
> (Yes, this makes them indented to the same spot as the body of the if. Yes this
> is annoying.)

Ok

> >+        PanelMenu.SystemStatusButton.prototype._init.call(this, 'preferences-desktop-accessibility', null);
> 
> SystemStatusButton._init() only takes 2 arguments

So SystemStatusButton._init.call takes 3.
 
> >+        let widget = this._buildItemExtended(_("Large text"), on_get,
> >+                function (enabled) {
> >+                    if (enabled)
> >+                        client.set_float(KEY_FONT_DPI, DPI_FACTOR_LARGE * getDPIFromX());
> >+                    else
> >+                        client.unset(KEY_FONT_DPI);
> 
> As I said before, if the user toggles and then untoggles this (or the theme
> setting) within the same session, we should revert to the previously-set value,
> not the default. You don't need to store the previous value in gconf though;
> this is just an "undo" feature ("Hm... what does this setting do? [Toggle]
> AAAAGH! [Toggle back]"). If the user switches to Large text, and then a month
> later switches back, it's ok to fall back to the default value.

Ok

> >+                'XDG_DATA_DIRS'       : '@datadir@:' + (os.environ.get('XDG_DATA_DIRS') or '/usr/share:/usr/local/share'),
> 
> I committed this part separately this morning (although with /usr/local/share
> first, because that's what the spec says).

I saw (and rebased locally)

> >+  <autotools id="gsettings-desktop-schemas">
> >+    <branch repo="git.gnome.org" module="gsettings-desktop-schemas" />
> >+  </autotools>
> 
> You will need to add <dep package="gsettings-desktop-schemas"> to the
> gnome-shell module as well.

Yeah, I did not add it because it gets squashed with other changes I did immediately after.
Comment 11 Dan Winship 2010-07-22 16:50:55 UTC
(In reply to comment #10)
> Except that C code gets SIGFPE for division by 0, while JS code gets Infinity,
> which is obviously outside the reasonable interval.

Oh. Handy.

> > SystemStatusButton._init() only takes 2 arguments
> 
> So SystemStatusButton._init.call takes 3.

err. :-}
Comment 12 Giovanni Campagna 2010-07-22 22:33:05 UTC
Created attachment 166417 [details] [review]
Universal Access indicator (the third!)

Disabled items are manually made non reactive, but are not differently styled (except they don't get hovered). Given that we need to make them active at some point, I don't think we need additional code or CSS.
Comment 13 Giovanni Campagna 2010-09-11 16:39:42 UTC
This was last commented two months ago.
Any progress on this?
Comment 14 Dan Winship 2010-09-13 19:16:41 UTC
Comment on attachment 166417 [details] [review]
Universal Access indicator (the third!)

> Disabled items are manually made non reactive, but are not differently styled
> (except they don't get hovered).

It's very weird that they're just "dead". I think it would be better to have them be dimmed. We're going to need CSS for inactive items at some point...

The "High Contrast" item is on by default for me, and I can't turn it off.

It also thinks I have "Large text" turned on, but really I just have a very-high-resolution laptop screen (145 dpi). Which means that if I set my DPI to 96, and then toggle "Large text" on, it actually sets it to a DPI (120) which is still too small to read comfortably.

(Also, if you toggle "Large text", gnome-shell will currently crash, but that's bug 629550).

Other than that, the code is mostly fine.

>+function getDPIFromX() {
>+    let screen = Gdk.Screen.get_default();

"let screen = global.get_gdk_screen();" is preferable (and then you can remove the Gdk import as well).
Comment 15 Giovanni Campagna 2010-09-14 20:13:06 UTC
Created attachment 170286 [details] [review]
Same patch, with latest comments

Now all items are enabled.
The Zoom Screen has been connected to the shell internal magnifier (but for this reason, the magnifier is created before the panel at startup), the other AT use gsettings-desktop-schemas and are just waiting for gnome-session to be ported (like the High Contrast is waiting for gnome-settings-daemon to expose the GSettings value on XSettings/GtkSettings).
Font DPI handling should be fixed, if you still are marked with large font enabled, it may be a bug in your version of Xorg or in your screen's EDID (since those are used to compute the "real" DPI).
Comment 16 Dan Winship 2010-09-16 19:04:19 UTC
ok, let's commit this as is. we can do more work on it (possibly disabling
the non-working items, etc) later