Last Comment Bug 669130 - [atk] Page setup dialog is not added to Firefox a11y tree
: [atk] Page setup dialog is not added to Firefox a11y tree
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla9
Assigned To: Ginn Chen
:
:
Mentors:
Depends on: 672671
Blocks: 431221
  Show dependency treegraph
 
Reported: 2011-07-04 01:11 PDT by Ginn Chen
Modified: 2011-09-26 13:08 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.80 KB, patch)
2011-07-05 00:57 PDT, Ginn Chen
no flags Details | Diff | Splinter Review
patch v2 (13.96 KB, patch)
2011-07-06 01:44 PDT, Ginn Chen
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
patch v3 (24.76 KB, patch)
2011-07-10 10:16 PDT, Ginn Chen
tbsaunde+mozbugs: review+
roc: review+
Details | Diff | Splinter Review
patch (1.90 KB, patch)
2011-07-19 20:04 PDT, Ginn Chen
no flags Details | Diff | Splinter Review

Description Ginn Chen 2011-07-04 01:11:05 PDT
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 2011-07-04 01:37:15 PDT
Perhaps we can do something like gail_toplevel_show_event_watcher() / gail_toplevel_hide_event_watcher().
Comment 2 Ginn Chen 2011-07-05 00:57:20 PDT
Created attachment 543893 [details] [diff] [review]
patch

I think it is a much better solution.
Comment 3 alexander :surkov 2011-07-05 02:42:56 PDT
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 2011-07-06 01:44:09 PDT
Created attachment 544170 [details] [diff] [review]
patch v2

comments addressed and some small changes in nsApplicationAccessibleWrap.cpp
Comment 5 Ginn Chen 2011-07-06 01:56:06 PDT
(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 2011-07-06 21:30:27 PDT
(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 2011-07-06 21:34:33 PDT
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) 2011-07-07 08:11:58 PDT
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) 2011-07-07 08:16:41 PDT
> 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 2011-07-10 10:16:59 PDT
Created attachment 545087 [details] [diff] [review]
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) 2011-07-10 16:04:37 PDT
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) 2011-07-11 18:01:40 PDT
Comment on attachment 545087 [details] [diff] [review]
patch v3

Review of attachment 545087 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 14 Stefan 2011-07-14 09:20:19 PDT
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 <ginn.chen@oracle.com>
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 2011-07-19 09:49:59 PDT
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 2011-07-19 20:04:10 PDT
Created attachment 546971 [details] [diff] [review]
patch

Do not init sQuark_gecko_acc_obj staticly.
Comment 17 Ginn Chen 2011-07-19 20:06:41 PDT
Comment on attachment 546971 [details] [diff] [review]
patch

oops, there's already a patch in Bug 672671.
Comment 18 alexander :surkov 2011-09-26 03:19:59 PDT
(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 2011-09-26 05:45:38 PDT
(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) 2011-09-26 13:08:35 PDT
https://hg.mozilla.org/mozilla-central/rev/1a11c6516cda

Note You need to log in before you can comment on or make changes to this bug.