669130 - [atk] Page setup dialog is not added to Firefox a11y tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Ginn Chen]

On Linux, enable a11y, start Page Start dialog from File->Page Setup ...

You cannot find the dialog in Firefox a11y tree in accerciser.

The problem is we're using gtk_print_run_page_setup_dialog().
So we don't have a chance to do the trick of RunDialog().

This is a minor issue because
1) The dialog still works with a11y tools, e.g. Orca.
2) Page Setup can be accessed from "Print ..." dialog, page setup tab.


I hope we can have a generic fix for the gtk dialogs rather than re-implement gtk_print_run_page_setup_dialog() in our code.

Comment 1

[Ginn Chen]

Perhaps we can do something like gail_toplevel_show_event_watcher() / gail_toplevel_hide_event_watcher().

Comment 2

[Ginn Chen]

Attachment: patch

I think it is a much better solution.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 543893 [details] [diff] [review]
patch

please fix style:
1) type* obj; vs type *obj;
2) use explicit casting

>+
>+        quark_gecko_acc_obj = g_quark_from_static_string("GeckoAccObj");
>+        g_signal_add_emission_hook(g_signal_lookup("show", GTK_TYPE_WINDOW),
>+          0, toplevel_event_watcher, (gpointer)nsIAccessibleEvent::EVENT_SHOW, (GDestroyNotify) NULL);
>+        g_signal_add_emission_hook(g_signal_lookup("hide", GTK_TYPE_WINDOW),
>+          0, toplevel_event_watcher, (gpointer)nsIAccessibleEvent::EVENT_HIDE, (GDestroyNotify) NULL);

should you pass quark_gecko_acc_obj as second argument or what quark_gecko_acc_obj is for?
should you remove emission hook when a11y shutdowns?
does it trigger for gecko dialogs only and if yes then how does work?

Comment 4

[Ginn Chen]

Attachment: patch v2

comments addressed and some small changes in nsApplicationAccessibleWrap.cpp

Comment 5

[Ginn Chen]

(In reply to comment #3)
> should you pass quark_gecko_acc_obj as second argument or what
> quark_gecko_acc_obj is for?

quark_gecko_acc_obj is static in this file, so I don't need to pass it.
It is an uniquely identify for the string "GeckoAccObj".
I need a GQuark to save according GeckoAccObj pointer into AtkObject.

> should you remove emission hook when a11y shutdowns?

I don't think atk-bridge/gail/... can really be shutdown/init at runtime.
So unless we actually support this feature, I'll leave the hooks.
We can use quark_gecko_acc_obj to avoid adding hooks again.

gail also leaves hooks in their code.

> does it trigger for gecko dialogs only and if yes then how does work?

The hook is triggered for all GTK window show/hide event, e.g. Mozilla window/dialog, menu window, combobox dropdown, tooltip, GTK dialog, etc.

(!IS_MAI_OBJECT(child) && (atk_object_get_role(child) == ATK_ROLE_DIALOG))
makes sure we only operate on GTK native dialogs, e.g. page setup, file chooser, print dialog, etc.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (In reply to comment #3)

> > should you remove emission hook when a11y shutdowns?
> 
> I don't think atk-bridge/gail/... can really be shutdown/init at runtime.

I don't get answer, sorry. I meant if there's a function that adds a hook and there's a function to remove the hook then why we don't remove it when we don't need it anymore, for example, when Firefox is closed. Is it removed automatically when Firefox closes or hangs until you reboot the system?

> So unless we actually support this feature, I'll leave the hooks.
> We can use quark_gecko_acc_obj to avoid adding hooks again.
> 
> gail also leaves hooks in their code.
> 
> > does it trigger for gecko dialogs only and if yes then how does work?
> 
> The hook is triggered for all GTK window show/hide event, e.g. Mozilla
> window/dialog, menu window, combobox dropdown, tooltip, GTK dialog, etc.
> 
> (!IS_MAI_OBJECT(child) && (atk_object_get_role(child) == ATK_ROLE_DIALOG))
> makes sure we only operate on GTK native dialogs, e.g. page setup, file
> chooser, print dialog, etc.

That's not what I meant to ask, sorry. Why are only native windows open by Firefox triggers the installed hook? Is this hook local and we are not notified by native windows open by other applications?

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 544170 [details] [diff] [review]
patch v2

r=me for gecko part, I'd like if someone will look at gail calls. Trevor, can you be this person?

Also I think we'd need r+ from Robert since we invade his area.

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 544170 [details] [diff] [review]
patch v2

>+
+        if (!quark_gecko_acc_obj) {
>+          quark_gecko_acc_obj = g_quark_from_static_string("GeckoAccObj");
gobject doesn't let us do that statically? :(

>+          g_signal_add_emission_hook(g_signal_lookup("show", GTK_TYPE_WINDOW),
>+            0, toplevel_event_watcher,
>+            reinterpret_cast<gpointer>(nsIAccessibleEvent::EVENT_SHOW), NULL);
>+          g_signal_add_emission_hook(g_signal_lookup("hide", GTK_TYPE_WINDOW),
>+            0, toplevel_event_watcher,
>+            reinterpret_cast<gpointer>(nsIAccessibleEvent::EVENT_HIDE), NULL);

I think we really should remove these on shutdown.  Its not hard, and even if we can't turn accessibility all the way off yet, we can atleast make the performance impact less.

Comment 9

[Trevor Saunders (:tbsaunde)]

> I don't get answer, sorry. I meant if there's a function that adds a hook
> and there's a function to remove the hook then why we don't remove it when
> we don't need it anymore, for example, when Firefox is closed. Is it removed
> automatically when Firefox closes or hangs until you reboot the system?

its local to our process, so it necessarily goes away when our process goes away.

> > gail also leaves hooks in their code.

that doesn't make it a good thing to leave more.

> That's not what I meant to ask, sorry. Why are only native windows open by
> Firefox triggers the installed hook? Is this hook local and we are not
> notified by native windows open by other applications?

yes, its only for windows in our process

Comment 10

[Ginn Chen]

Attachment: patch v3

1) static GQuark sQuark_gecko_acc_obj = g_quark_from_static_string("GeckoAccObj");
2) Remove hook in nsApplicationAccessibleWrap::Unload()
3) Add s- prefix for all static variables in this file.
4) Remove trailing whitespaces.

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 545087 [details] [diff] [review]
patch v3

> 
>-static GHashTable *listener_list = NULL;
>-static gint listener_idx = 1;
>+static GHashTable *sListener_list = NULL;

type* forAlex

>+static GHashTable *sKey_listener_list = NULL;

same

>+static PRBool sToplevel_event_hook_added = PR_FALSE;

just use bool and true / false

r=me thanks

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 545087 [details] [diff] [review]
patch v3

Review of attachment 545087 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 13

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/34b0b3bc6984

Comment 14

[Stefan]

When I start firefox I get the message

(process:16712): GLib-CRITICAL **: g_slice_set_config: assertion `sys_page_size == 0' failed

on the console.

The first bad revision is:
changeset:   72769:34b0b3bc6984
user:        Ginn Chen <[e-mail]>
date:        Thu Jul 14 09:58:32 2011 +0800
summary:     Bug 669130 [atk]Use emission hook for show/hide signal to add/remove GTK+ native a11y dialog to children of Mozilla application accessible. r=trev.saunders,roc

Comment 15

[arno renevier]

This happens because sQuark_gecko_acc_obj is static and therefore call to g_quark_from_static_string and then g_slice init part is made as soon as libxul.so is dlopened.

But when g_slice_set_config is called inside XRE_Main, to disable, g_slice allocator, g_slice refuses because it has already been initialized.

So, the result is an regression of bug #431221

Comment 16

[Ginn Chen]

Attachment: patch

Do not init sQuark_gecko_acc_obj staticly.

Comment 17

[Ginn Chen]

Comment on attachment 546971 [details] [diff] [review]
patch

oops, there's already a patch in Bug 672671.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Ginn Chen from comment #13)
> http://hg.mozilla.org/mozilla-central/rev/34b0b3bc6984

Ginn, it appears you forgot to remove nsAccessibilityHelper.h/cpp files

Comment 19

[Ginn Chen]

(In reply to alexander surkov from comment #18)
> (In reply to Ginn Chen from comment #13)
> > http://hg.mozilla.org/mozilla-central/rev/34b0b3bc6984
> 
> Ginn, it appears you forgot to remove nsAccessibilityHelper.h/cpp files

Thanks!

Now I removed them.
http://hg.mozilla.org/integration/mozilla-inbound/rev/1a11c6516cda

Comment 20

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/mozilla-central/rev/1a11c6516cda