It's actually against GtkHTML, in a11y/object.c is used "gail-focus-object". The change there steals old object to gail an it then claims in gtk/modules/other/gail/gail.c when calling g_object_weak_unref on the changed object data, not on the right one. I would like to suggest to not change that object data in GtkHTML at all, but I do not know if it's the proper fix, and for what it is there.

Created attachment 105347 [details] [review]
proposed gtkhtml patch

for gtkhtml;

I'm not sure if this is the right fix, but I believe it is. The call to
g_object_set_data (G_OBJECT(obj), "gail-focus-object", focus_object);
just steals object to gail, which expects to have there something with weak ref callback set. Because of later call to atk_focus_tracker_notify it just claims on missing thing. Also this call makes same thing as the g_object_set_data, but adds there a weak ref as a bonus. With this patch it seems like working, even I didn't have much idea how to test it properly.

Please notice also other changes I did there, I think they are right too.

Milan, I absolutely have no idea on this code. CC'ing Li here.

Li can you help us review this Accesibility patch?

Sure, I will review the patch later.

Thx Li. Try for 2.21.92 (Monday)

"gail-focus-object" is used by gail in gailwidget.c:gail_widget_ref_state_set, which determines whether add focus state to the object. AT can not find the focused object after applying the patch.

Created attachment 105762 [details] [review]
proposed gtkhtml patch ][

for gtkhtml;

Thanks for the review. I found other possibility how to solve this (I hope the 'solve' word is appropriate), it seems that it depends on the order of function calls, so I changed order to one which seems to be fine for both things.

Can you check/review this patch, please? Thanks in advance.

When calls atk_focus_tracker_notify, application will send out a "focus" event to accessibility infrastructure, and some AT (like Orca) will call back to ref_state_set. This will happen before 
g_object_set_data (G_OBJECT(obj), "gail-focus-object", a11y) after applying the patch. And AT will not get the focused state. So we need to call g_object_set_data before atk_focus_tracker_notify. Can we weak_ref the object before calling g_object_set_data?

We cannot, as far as I know. If we change the order, then we will be back where we were. The weak_ref is done in gtk/modules/other/gail/gail.c, in function gail_set_focus_object and is unreffed in this function too, and in gail_focus_tracker, which is called within the call of atk_focus_tracker_notify. It was the reason why I thought the remove of g_object_set_data from gtkhtml is correct, because it is playing with same thing twice.

Are you sure that the initial patch is not working properly with gail? Because when I was testing, then it was playing with proper (same) pointers, thus it should work.

(In reply to comment #9)
> Are you sure that the initial patch is not working properly with gail? Because
> when I was testing, then it was playing with proper (same) pointers, thus it
> should work.
> 

Do you test the with Orca running? The "focus" event I mentioned is used in accessibility infrastructure, not in GUI. So Orca may not find the focused object after applying the patch, depending on the logic in Orca.

Yeah, I tested with Orca running, I turned on the screen reader (I do not know the exact name, it shows text around the cursor), because I thought it's one of the usages of this, and I didn't notice any difference between before and after patch applied, the proper text around cursor has been shown up there.
Was it bad place to look at, or how can I test this properly?

Maybe Orca didn't call back synchronously. It still could be a problem in the future. We should set the focus object before we tell them the focus is changed, I think.

(In reply to comment #12)
> Maybe Orca didn't call back synchronously. It still could be a problem in the
> future. We should set the focus object before we tell them the focus is
> changed, I think.
> 

For performance reasons, Orca handles almost all events asynchronously.  As such, it might be likely that the objects reached the desired state by the time Orca actually processed the events.   You can force Orca to process events synchronously by adding the following line to the end of your ~/.orca/user-settings.py file:

orca.settings.asyncMode = False

However, in looking at this bug, my understanding is that a focus event is being generated for an object, but that object's FOCUSED state is not sent when the event is generated.  I believe the implementation should really strive to make sure the object state matches the event -- so, if the event is telling us the object now has focus, then the FOCUSED state should be set.

Can you point me how can I ensure it is working, say with Orca (which part of Orca?). If you can point me where to look, what exactly doesn't work with the first patch, then I can look at it and try to solve it properly, but without any idea where to look, it's more than hard (where I did look it seemed fine, so asking). Thanks.

From the code, we need to call
g_object_set_data (G_OBJECT(obj), "gail-focus-object", focus_object); before
atk_focus_tracker_notify (focus_object);
That means we need to tell gail the "obj" is the focused object first and then notify the focus change. Otherwise gail cannot find the focused object.

As Will said, you can set orca.settings.asyncMode = False in ~/.orca/user-settings.py, to see if Orca can still read the focused object.

Created attachment 106606 [details]
test patch

for gtkhtml (intentionally marked as plain text)

I did this test patch to see what it is doing based on object pointers, and it showed that the atk_focus_tracker_notify *do* set the "gail-focus-object" too, and the widget *has* set FOCUSED flag before calling atk_..._notify function. Based on this it seems that the first patch is correct. Or I obviously miss here something...

Here is my output on console after applying this test patch, with caret mode on, clicking with mouse to one paragraph in the preview panel, and then into  other paragraph:
gtk_html_a11y_grab_focus_cb:new_fo:0xcff6d0, my_fo:(nil), obj:0xcff720,
   gail_fo_before_notify:(nil): focus_set:1
        gail after atk_..._notify:0xcff6d0
        gail after set_data:0xcff6d0
gtk_html_a11y_grab_focus_cb:new_fo:0xf0fde0, my_fo:0xcff6d0, obj:0xcff720,
   gail_fo_before_notify:0xcff6d0: focus_set:1
        gail after atk_..._notify:0xf0fde0
        gail after set_data:0xf0fde0

OK, with asyncMode set to False it doesn't work. Is somewhere a UI option for this? I didn't find it in my user-settings, somehow, so Iwrote it there by hand.

(In reply to comment #17)
> OK, with asyncMode set to False it doesn't work. Is somewhere a UI option for
> this? I didn't find it in my user-settings, somehow, so Iwrote it there by
> hand.

This is a developer-only option, so Orca doesn't provide a UI option for it.

Thanks, good to know, so should I bother with sync mode? You know it definitely acts differently and I'm not sure if it does work "fine" in async for me only because of "quick" machine/good timing, or there can be any hidden issue which you see and I don't?

(In reply to comment #19)
> Thanks, good to know, so should I bother with sync mode? You know it definitely
> acts differently and I'm not sure if it does work "fine" in async for me only
> because of "quick" machine/good timing, or there can be any hidden issue which
> you see and I don't?
> 

The timing issue is one concern.  The other concern, however, is that we cannot expect all assistive technologies to behave like Orca.  True and proper behavior should exist when events are processed synchronously.  Thanks!

OK, I tried as you said, and surprisingly for me, it doesn't claim the unref warning in both cases. Anyway, it doesn't wand to read the text around cursor in both modes (sync versus async) for me,  I've probably something broken on my machine, I guess.

The problem is, when I disable accessibility feature in personal preferences and do not restart my machine, then I see the unref warning on the console. I know, it can be because I didn't logout, but should not this work properly anyway?

(In reply to comment #21)
> The problem is, when I disable accessibility feature in personal preferences
> and do not restart my machine, then I see the unref warning on the console. I
> know, it can be because I didn't logout, but should not this work properly
> anyway?
> 
If you start gnome-session with accessibility support enabled, no matter if you unset the accessibility gconf key ( System->Preferences->Universal access->Assistive Technology-->Enable assistive technologies) during runtime, atk-bridge and gail are loaded anyway (loaded by gtk_module_init, because gnome-session set GTK_MODULES to gail:atk-bridge). So I don't see any difference here...

I'm trying this on my updated Fedora 8 box. Do you think it does matter?
I see the difference between enabled/disabled from preferences without restart, when I've it enabled, then I see "tons" of other warnings about invalid type casts and similar things, but with disabled it disappears, except of unref warning.
Smells like NOTGNOME or I've really something broken here.

What is the version of GNOME on your system? Seems on your system gail and atk-bridge are not loaded if you unset the flag. I realize that Evolution calls gnome_program_init instead of gtk_init during startup, but on my system, gtk_module_init is called anyway (by gnome_program_init), so gail and atk-bridge is loaded. Maybe on your system gtk_module_init is not called. Anyway, I think you should set the flag and test the patch.

My Gnome is 2.20.3, but because I use Evolution 2.21.92 (the svn checkout) and it requires some newer packages, then I also compile my own glib/gtk/... and also atk, at-spi.

I will try on F9 and either close this or start to investigate more on fresh system. Thanks for your help.

Bumping version to a stable release.

Milan, Any updates?

Nope, I'm sorry, still waiting for F9 to test it and see whether this was a problem on my local build only or is a generic problem. I will update/close this bug as soon as I will know more info.

Mcrha: Any updates?

I can see the same warnings on my test machine with fresh new F9 with evo 2.22.1, thus we should (try to) fix it. The problem is I'm not able to produce any better patch than one of those I put here and I believe one of them is correct. Or if someone is thinking that's not truth, then please provide me steps how to reproduce it, like what to have turned on in a system, maybe installed too, and then how to see in evo what is wrong in described situation, because I do not see any difference with and without the patch (probably because I look into wrong place). Thanks in advance.

(In reply to comment #13)
> However, in looking at this bug, my understanding is that a focus event is
> being generated for an object, but that object's FOCUSED state is not sent when
> the event is generated.  I believe the implementation should really strive to
> make sure the object state matches the event -- so, if the event is telling us
> the object now has focus, then the FOCUSED state should be set.

I'll attach a test app to check for this case above.

Created attachment 112623 [details]
Test app to check for focus event and FOCUSED state parity

Setting a different patch status to get it off my tracking list.

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

Created attachment 117921 [details] [review]
proposed gtkhtml patch ]I[

for gtkhtml;

I had a chat with Li on #a11y and during some investigation I realized that touching "gail-focus-object" is unnecessary, it all does gail during call of atk_focus_tracker_notify, because when I try to get the object's data for this, then I get exactly the same object as we were trying to set before/after that. From that I believe to changing object's data directly is a violation of good behavior :)

Just for a record, the trace for the warning is this:

+ Trace 206249

From which you can see all the weak_reffing and weak_unreffing is done during call of atk_focus_tracker_notify in gail_set_focus_object, thus gail takes care of this itself, when it wants to.

Thanks for the patch.

Committed to trunk. Committed revision 8956.