(In reply to comment #0)
> The object one uses to change the volume in gnome-shell:

After a quick skim on volume.js and popupMenu.js:

> 1. Claims to be of ROLE_UNKNOWN (ROLE_SLIDER is suggested)

This should be solved by bug 648598

> 3. Does not implement the value-changed signal

popupMenu itself emits value-changed signals, so it would be about a re-emission via ATK.

> 2. Does not implement AtkValue

Well this is the problematic part. As we know, we can't subclass an gobject on the java script code. Current slider component is a javascript object that manages the clutter object, and it is a generic-container. From generic-container documentation:

 * #ShellGenericContainer is mainly a workaround for the current
 * lack of GObject subclassing + vfunc overrides in gjs.  We
 * implement the container interface, but proxy the virtual functions
 * into signals, which gjs can catch.

So, as a first thought, one solution would use a similar "have a global workaround" approach here, and define a specific ShellGenericContainerAccessible implementing all the ATK interfaces that we would require eventually, with a proxified implementation.

This is not ideal, as AT tools usually do heuristics based on the interfaces implemented by an accessible, but workaround are by definition not ideal solution. ATs in this case should require to rely more on the ROLE information.

In the same way, that means that part of this AtkValue implementation should be required to be implemented on the js code (signal callbacks).

But as I said, this was just a first thought.

Doubts, opinions, any other ideas?

(In reply to comment #1)
anged signals, so it would be about a
> re-emission via ATK.
> 
> > 2. Does not implement AtkValue
> 
> Well this is the problematic part.

[snip]

> This is not ideal, as AT tools usually do heuristics based on the interfaces
> implemented by an accessible, but workaround are by definition not ideal
> solution. ATs in this case should require to rely more on the ROLE information.

I'll leave it to y'all to sort out how to deal with this, but from the AT side of things, I don't think it's unreasonable to expect ATs to take into account the ROLE. Orca already needs to distinguish progress bars from other value-changing objects.

If we got the value-changed event for an object claiming to be of ROLE_SLIDER, we could then query AtkValue for the information we desire. Similarly, if an object claiming to be of ROLE_SLIDER claimed to be showing, we'd query AtkValue.

So, from my perspective, I'm failing to see any real problems as long as the three requested things all get implemented. (Unless I'm missing something.)

*** Bug 675108 has been marked as a duplicate of this bug. ***

(In reply to comment #1)

> 
> > 2. Does not implement AtkValue
> 
> Well this is the problematic part. As we know, we can't subclass an gobject on
> the java script code.

This is not true anymore, as the javascript gobject support on gjs has improved recently, ie:

http://blog.mecheye.net/2012/02/gjs-improvements/

> Current slider component is a javascript object that
> manages the clutter object, and it is a generic-container. From
> generic-container documentation:
> 
>  * #ShellGenericContainer is mainly a workaround for the current
>  * lack of GObject subclassing + vfunc overrides in gjs.  We
>  * implement the container interface, but proxy the virtual functions
>  * into signals, which gjs can catch.

In fact, after a brief talk with Florian during the UX Hackfest, it seems that the ShellGenericContainer is planned to be replaced.

So, in theory we could create an ATK object subclass on one of our StXXXAccessible object, implementing AtkValue (exposing PopupSliderMenuItem._value and notifying any change on that value).

*But*, the problem now is that this slider is still using as actor a GenericContainer, and we set which accessibility object return by class not by instance. So with the current accessibility support, it would be required to subclass GenericContainer (to something like GenericContainerSlider) in order to set the class attribute get_accessible_type. Something that a) seems an overkill, b) as I said there are plans to replace GenericContainer so not sure if it is worth to change that now.

Other option is just add a extra way to specify the accessible object. This time based on the instance additional to the current default way to specify the accessible object based on the class. After all, we are already doing something similar with the role (each relevant StXXX sets a default role, but StWidget provides a .role property in case a specific instance have a different role).

CCing Jasper and Florian just in case they want to add to this like-brainstorming commet.

*** Bug 684461 has been marked as a duplicate of this bug. ***

Updating the bug summary. This bug now focused on the lack of AtkValue implementation. Name and role is now managed by bug 706391. Key navigation is now managed by bug 706386.

(In reply to comment #4)

> In fact, after a brief talk with Florian during the UX Hackfest, it seems that
> the ShellGenericContainer is planned to be replaced.

But almost one year after, is still there.

> So, in theory we could create an ATK object subclass on one of our
> StXXXAccessible object, implementing AtkValue (exposing
> PopupSliderMenuItem._value and notifying any change on that value).

But this is also not done at all at the current gnome-shell javascript code.

> CCing Jasper and Florian just in case they want to add to this
> like-brainstorming commet.

And I didn't get any reply, probably due the lack of patches. As I'm right now fixing some slider related bugs, I used the intertia to work on this. In the end I created a StGenericAccessible. The idea of this widget is provide an accessible with signal-based implementation of Atk interfaces needed at that moment. For that same reason, putting this as part of St makes more sense (imho).

Right now, only AtkValue, as is the one we need. I have some WIP patches, with a basic functionality of AtkValue implemented for the slider (specifically get_current_value and notify a value change). Will upload now just to have a tentative yes/no as quick as possible.

Created attachment 252887 [details] [review]
Add generic accessible

Right now only adds the possibility of implementing AtkValue.

WIP because not all the AtkValue methods are included yet.

Created attachment 252888 [details] [review]
Add the possibility to set the accessible object

So you could instantiante the generic accessible at the javascript code, and use it, instead of the usual one created by the StWidget itself.

Created attachment 252889 [details] [review]
Using the generic accessible at the slider

Example of usage of the previous two patches. Slider creates a generic accessible and use the signals to fill the data needed by AtkValue methods.

WIP because right  now only implement two functionalities of AtkValue: get current value, notify a value change.

Created attachment 252893 [details]
at-spi2 listener

Just in case someone is interested: this is the listener app I used to test this. Is based on the one Joanmarie uploaded at bug 706391, but as I'm having some weird python-problems on my development environment, it is written in C.

Just compile it, and run it. It will print gnome-shell events as you move around. If you go to the slider and start to change the value should start to print the new values.

Review of attachment 252888 [details] [review]:

::: src/st/st-widget.c
@@ +2592,3 @@
+    g_object_unref (widget->priv->accessible);
+
+  widget->priv->accessible = accessible;

Shouldn't you take a ref here, since you unref above?

On IRC I have been talking with Jasper, and he prefers to define the accessibility object for the slider at the javascript code.

The beginning was promising, as getting the accessible defined, and extending the interface was easy. And the method defined (I started with atk_value_get_current_value) is being called.

Anyway, I got several problems getting the method working, specifically with the support of GValue. All the methods of AtkValue expects the value to be returned on a GValue.

1) First, it seems that in order to call the new virtual method, and do the transformation, the GValue needs to be initialized to a specific type. If not it fails:

(gnome-shell:23834): Gjs-WARNING **: JS ERROR: Error: Don't know how to convert GType (null) to JavaScript object

Something that although understandable, removes the use of GValue, as the idea of using a GValue was putting whatever type is needed. Anyway, this was not a big issue, as we just care about doubles here (anyway, needs a change on atk).

2.) Fixing 1) doesn't solves the problem. If I do a myValue.set_double, I get the following error:

(gnome-shell:23834): Gjs-WARNING **: JS ERROR: TypeError: myValue.set_double is not a function

Although taking into account this example:
https://bug703412.bugzilla-attachments.gnome.org/attachment.cgi?id=248181

It should be possible to do that.

3) In any case, to workaround this, I tried to create a new method on shell-util as also suggested by Jasper. A simple method that just sets the value of the value (will add a patch with the code so you could understand). I get this error:

Window manager warning: Log level 8: g_value_set_double: assertion 'G_VALUE_HOLDS_DOUBLE (value)' failed

Something weird, as it was already initialized like that at the caller. But at the utility method, the type is INT.

4) You could workaround that on the utility method, but at this point it is clear that the problem is that at the javascript code you have a copy of the GValue. And in fact a bad copy, as doesn't use the same type.

This is already reported as a bug (or at least I think that is the same bug) here: 703412

In summary, it seems that although extending a class on javascript, implementing interfaces on top, was not as problematic as I thought, there are problems with the GValue support.

Created attachment 252938 [details] [review]
WIP:Defines the accessibility object at the javascript code

FYI, just in case you don't understand what I said on previous comment and want some code.

WIP: defines the accessibility object at slider.js. Also adds a method on shell_util in order to workaround the GValue problem, something that I failed. Ideas are welcome.

I'll let y'all sort out the best way to do this, but for what it's worth, the patches in comment 8, comment 9, and comment 10 combined with two changes I made to Orca cause Orca to present the slider name and value for gnome-shell sliders.

* https://git.gnome.org/browse/orca/commit/?id=b46671149f1cb1ad70a86ce183ef159ca7964667
* https://git.gnome.org/browse/orca/commit/?id=9eb35c9e7e68c2ef9149fe2246c8653426fe489e

Would be great to have this solved for 3.10.

Thanks!!

Created attachment 253163 [details] [review]
Add the possibility to set the accessible object

(In reply to comment #12)
> Review of attachment 252888 [details] [review]:
> 
> ::: src/st/st-widget.c
> @@ +2592,3 @@
> +    g_object_unref (widget->priv->accessible);
> +
> +  widget->priv->accessible = accessible;
> 
> Shouldn't you take a ref here, since you unref above?

I don't think so. The usual wisdom for a base-object/accessible-object relation is that the first ref of the accessible belongs to the base-object. This is what is done with the object that is created at st_widget_get_accessible if the widget doesn't have still an accessible. The object is created there, but g_object_ref is not called there. For coherence I prefer to treat both accessibles in the same way, as belonging to the widget. In any case I updated the documentation in order to make that clearer.

Additionally, I also moved the call to atk_object_initialize to this method. For any reason it was failing if called at the javascript code, and after all, it is also called at st_widget_get_accessible. Updated documentation to make it clear.

Created attachment 253164 [details] [review]
WIP:Defines the accessibility object at the javascript code

A more complete version of the previous AtkValue implementation at the javascript code. It implements all AtkValue method, and this time with real data. But as I said on previous messages, not working due the problems with GValue, that I don't have any idea of how to workaround. Also removed the previous not working workaround.

Additionally I also added the notification when the value changes. That is the only part that is working right now.

(In reply to comment #16)
> I don't think so. The usual wisdom for a base-object/accessible-object relation
> is that the first ref of the accessible belongs to the base-object. This is
> what is done with the object that is created at st_widget_get_accessible if the
> widget doesn't have still an accessible. The object is created there, but
> g_object_ref is not called there. For coherence I prefer to treat both
> accessibles in the same way, as belonging to the widget. In any case I updated
> the documentation in order to make that clearer.

Let me rephrase: you don't get to choose how your refcount logic works when you deal with bindings. All bindings assume that anything that wants to hold onto a GObject after you call a function with it will take its own ref. When the GC runs, it will unref it, and the program will crash.

If atk_object_initialize takes its a ref, then OK, but I'd reorder the calls and add a comment.

(In reply to comment #18)
> (In reply to comment #16)
> > I don't think so. The usual wisdom for a base-object/accessible-object relation
> > is that the first ref of the accessible belongs to the base-object. This is
> > what is done with the object that is created at st_widget_get_accessible if the
> > widget doesn't have still an accessible. The object is created there, but
> > g_object_ref is not called there. For coherence I prefer to treat both
> > accessibles in the same way, as belonging to the widget. In any case I updated
> > the documentation in order to make that clearer.
> 
> Let me rephrase: you don't get to choose how your refcount logic works when you
> deal with bindings. All bindings assume that anything that wants to hold onto a
> GObject after you call a function with it will take its own ref. When the GC
> runs, it will unref it, and the program will crash.

Ok. Thanks for the explanation.

> If atk_object_initialize takes its a ref, then OK, but I'd reorder the calls
> and add a comment.

No, atk_object_initialize implementation at AtkGObjectAccessible adds a weak_ref. So in the end, as you said before, a g_object_ref is needed.

I will update the patch as soon as we conclude something about the GValue problem.

Created attachment 253266 [details] [review]
Add the possibility to set the accessible object

Patched updated following what was discussed on comment 18 and comment 19.

Created attachment 253267 [details] [review]
Add generic accessible

I got boring of guessing what would be the error on the support for GValue (FWIW, I don't have too much experience with gjs internals), so I complete the patch. All AtkValue is supported now. I kwnow that in theory this is not the way to go, but at least now we have a complete working patch.

Created attachment 253268 [details] [review]
Using the generic accessible at the slider

Ditto.

Review of attachment 253267 [details] [review]:

Yeah, this is probably the way to go for now. GValue is sort of a mess.

Review of attachment 253266 [details] [review]:

::: src/st/st-widget.c
@@ +2608,3 @@
+  g_return_if_fail (ST_IS_WIDGET (widget));
+
+  if (widget->priv->accessible != accessible) {

We use the GNU style in our C code, so the braces should be on the next line.

Review of attachment 253268 [details] [review]:

::: js/ui/slider.js
@@ +31,3 @@
         this._dragging = false;
+
+        this.customAccessible = St.GenericAccessible.new_for_actor(this.actor);

This should be private; this._customAccessible = ...;

Created attachment 253275 [details] [review]
Add the possibility to set the accessible object

(In reply to comment #24)
> Review of attachment 253266 [details] [review]:
> 
> ::: src/st/st-widget.c
> @@ +2608,3 @@
> +  g_return_if_fail (ST_IS_WIDGET (widget));
> +
> +  if (widget->priv->accessible != accessible) {
> 
> We use the GNU style in our C code, so the braces should be on the next line.

Yeah sorry. Too much switching between projects with different code guidelines.

Created attachment 253276 [details] [review]
Using the generic accessible at the slider

Making customAccessible private (so _customAccessible)

Review of attachment 253276 [details] [review]:

OK.

Review of attachment 253275 [details] [review]:

style nit, otherwise OK

::: src/st/st-widget.c
@@ +2614,3 @@
+
+      if (accessible)
+        widget->priv->accessible =  g_object_ref (accessible);

extra space here

Patches pushed. Thanks for the review. Closing bug