User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050607 Firefox/1.0.4 Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.9a1) Gecko/20060402 Firefox/1.6a1 Firefox "Open File" window is not accessibile for GOK. Reproducible: Always Steps to Reproduce: 1. Launch Firefox and GOK. 2. Select top menu File->Open File, "Open File" window pops up. 3. Grab this window with GOK. Actual Results: This window is not accessible to GOK. Expected Results: User could grab this window with GOK successfully.

This should be the native Open File dialog from Gnome. Perhaps GOK is not expecting that in Firefox? Seems like a GOK bug.

Very odd. GOK isn't special casing behaviour for firefox... if the open file window comes up and is accessible then GOK should "see" it. Is at-poke able to access this window? Can you confirm this window is in the foreground? (Note you can use gok to activate different windows)

As I understand it, mozilla is unset-ing the gail GTK_MODULE, thus making gtk+ native widgets inaccessible. If my understanding is correct, mozilla will have to fix this issue. Check with at-poke to see if this dialog is showing up. Probably it isn't Bill

Hi Bill, I checked this window with at-poke, it's not accessible. For known issue, we have to set GTK_MODULES to null, otherwise, we could not test firefox a11y at all.

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

Bill, you're right. Mozilla stripes atk-bridge from GTK_MODULES, that caused this bug.

I need help on this bug. I tried to use gtk_widget_get_accessible to get AtkObject* of the file-picker dialog. But it doesn't work. The Role is INVALID. If mozilla doesn't unset GTK_MODULES, the role of file-picker accessible is DIALOG. How can I get valid AtkObject* of file-picker without setting GTK_MODULES? (If GTK_MODULES sets, mozilla a11y interface can not initialized correctly.) Any ideas? Thanks!

Ginn, any new ideas for this one?

Ginn, the role DIALOG might be appropriate -- what would you like the role to be?

DIALOG role is correct, but I can't get it after mozilla accessibility toolkit is initialized. The GTK file picker is a native widget, not in DOM, so mozilla a11y toolkit don't have corresponding a11y wrapper. It should use gail, but I didn't find a way to make gail and mozilla a11y toolkit work together in a same process. I need to understand libspi and atk-bridge to figure out a solution. Currently, the only "solution" I have is to disable GTK file picker when a11y is enabled. I don't think it's acceptable.

Ginn, I don't know if this helps, but you might look at the the way we handle Flash and PDF plugins in MSAA. http://lxr.mozilla.org/seamonkey/search?string=accessiblewin32 We just have a placeholder accessible that proxies incoming calls back to MSAA. Not sure if that helps, since I don't understand the problem of getting them to work together.

Yes, I'm also trying to make a proxy for the dialog, put it into our accessible tree. But the problem is I couldn't get the dialog's accessible object by gtk_widget_get_accessible.

It looks like Open Office uses its own open dialog, otherwise they'd have the same problem.

gail can work together with mozilla a11y toolkit now (I don't why it didn't work 1.5 months ago) export GTK_MODULES=:gail before start firefox use gtk_widget_get_accessible(file_chooser) will return an AtkObject pointer, the ROLE is DIALOG. so the next step is connect it to the root accessible of firefox

with this patch we don't need to set GTK_MODULES=:gail in shell

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

We still need some work to send window:activate/deactivate, focus events for the dialog, I'm not sure how to do it yet. Maybe we need send children::add/remove for app accessible, we didn't do it for mozilla window, either. Anyway, orca and at-poke can work now. (need to press "tab" once to make orca start working on the dialog) Aaron, do you have comments for this patch?

Comment on attachment 224984 [details] [diff] [review] patch (GOK still not wokring) in nsRootAccessibleWrap ctor, this is not intuitive to me: + nsAccessibleWrap::mIsGailAtkObject = PR_TRUE; Why would all nsRootAccessibleWrap instances be gail atk object. Actually, what does it exactly mean if that is true. This is a difficult patch to understand and review. I'm sure it was difficult to write. maybe you should add more comments to the code so everyone can understand it?

Comment on attachment 224984 [details] [diff] [review] patch (GOK still not wokring) I'll use nsNativeRootAccessibleWrap and CreateNativeRootAccessible to make it more readable. BTW: The overwrite confirm dialog also needs a fix.

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

load gail lib; chain call gail util's add/remove event listener function; add gail window accessible as a child of mozilla app root accessible and emit chidren-changed event to get gail work for it. Verified with GOK.

We need a helper method in gtk2 that can do this for any native dialog. It would take the native dialog as an argument and return the result. 1) get a11y service 2) create native accessible 3) run the dialog and get the result 4) remove native accessible 5) return result This will help with code reusability in the future. I would have thought that there will be other native dialogs.

Why do we define it with an _ in front and then do this? typedef struct _MaiUtilListenerInfo MaiUtilListenerInfo;

+ // use -1 for index This comment appears twice but it does not explain why -1 -- what is that magic value for? + root->AddRootAccessible(NS_STATIC_CAST(nsIAccessible*, this)); Why is the static cast necessary when passing the argument?

The method and variable name used in this line makes it look like it should always be true: if (MAI_IS_ATK_OBJECT(mMaiAtkObject)) I suggest these names: IS_MAI_OBJECT to show we are testing to see if the object was created by Mai mAtkObject to show it can be any ATK object

This line should not have been changed, it got a comma added. - "gnome_accessibility_module_shutdown", NULL, We seem to be mixing 2 and 4 spaces for indent in nsAccessibleWrap: if (gail_add_global_event_listener) { // call gail's function to track gtk native window events Since this else will be empty if MAI_LOGGING is not defined I think it will not compile: else MAI_LOG_DEBUG(("Fail to load lib: %s\n", sGail.libName)); Why is this line no longer needed in nsAppRootAccessible::Init()? g_type_init(); We try no longer to use _retval as an argument ever since on of the superreviewers told us it is not an appropriate name. Use something like aRootAccessible for AddNativeRootAccessible() Please comment this line because it's not obvious why we need it, unless we have it in all constructors: if (!mDOMNode) return I think this is from a different patch: nsMenuFrame::OpenMenu()

Thanks for your review! I'll address your comments. Here're questions and answers. >We need a helper method in gtk2 that can do this for any native dialog. Can you suggest a place for the helper method? Which file should I add this method? >Why do we define it with an _ in front and then do this? > typedef struct _MaiUtilListenerInfo MaiUtilListenerInfo; I'm not clear, the style is copied from gailutil.c. + // use -1 for index I'll fix it. Use real index instead. + root->AddRootAccessible(NS_STATIC_CAST(nsIAccessible*, this)); I'll fix it. STATE_CAST is not needed. - "gnome_accessibility_module_shutdown", NULL, QUESTION: Why do we need the extra comma? >MAI_LOG_DEBUG(("Fail to load lib: %s\n", sGail.libName)); According to nsMai.h, we don't need to quote it with #ifdef MAI_LOGGING >Why is this line no longer needed in nsAppRootAccessible::Init()? >g_type_init(); We call gtk_init() in nsAppRunner.cpp, it is enough, we don't need to call g_type_init again.

Another issue: We need to check nsWindow::sAccessibilityEnabled. Otherwise, a11y will be enabled after file-picker dialog pops once.

(In reply to comment #27) > Can you suggest a place for the helper method? > Which file should I add this method? Not sure, you can add a new a11y helper file if there's not already a file with static helpers. > >Why do we define it with an _ in front and then do this? > > typedef struct _MaiUtilListenerInfo MaiUtilListenerInfo; > I'm not clear, the style is copied from gailutil.c. I don't think we need that style in Mai. > - "gnome_accessibility_module_shutdown", NULL, > QUESTION: Why do we need the extra comma? My mistake. Removing it is the right thing to do. > >MAI_LOG_DEBUG(("Fail to load lib: %s\n", sGail.libName)); > According to nsMai.h, we don't need to quote it with #ifdef MAI_LOGGING That's not what I mean. Sometimes MAI_LOG_DEBUG will resolve to nothing in which case you have a dangling |else|, I think. Don't you need to put it in { } so that still compiles? > >Why is this line no longer needed in nsAppRootAccessible::Init()? > >g_type_init(); > We call gtk_init() in nsAppRunner.cpp, it is enough, we don't need to call > g_type_init again. okay.

This patch addresses a problem in mozilla applications like firefox that do strip GTK_MODULES and don't initialise a11y in other ways (e.g. using gnome_program_init). However the patch modifies core files (nsAppRootAccessible.cpp and nsFilePicker.cpp) which are used in all gecko applications, including embedding applications like Epiphany which already do initialise a11y themselves and are already accessible without this patch. (I tested the filepicker in Epiphany using at-poke and it is accessible). Will this patch interfere with embedding application's a11y?

(Sorry about the accidental resolution, don't know how this happened.)

Hi, chpe With this patch, we're trying to make libgail and mozilla a11y toolkit work together. We have to call atk-bridge init() as last thing. Because at-bridge won't init() twice, even after shutdown(). That's why we have to strip atk-bridge from GTK_MODULES. I think epiphany is not affected by this patch. I'll check it out when I've time. Thanks!

I think epiphany is using its own FilePicker.cpp rather than nsFilePicker.cpp And atk-bridge is initialized before mozilla a11y toolkit initialization. I guess this patch won't affect epiphany.

typedef _Mai* issue will be addressed in another bug

Ginn, I think your forgot to include RunDialog()'s definition in the patch.

I also forgot to address Comment #28 in last patch.

Comment on attachment 233057 [details] [diff] [review] patch v4 Ginn, I still don't see the AccessibilityHelper in gtk2. But, you will need r= from that from a gtk peer anyway, so I'll give r= on the accessibility module changes now. + nsAppRootAccessible *root = nsAppRootAccessible::Create(); + root->AddRootAccessible(*aRootAccessible); Change the name of |root| to |appRoot| because it looks so similar to aRootAccessible as to be confusing. Change the name of |mMaiAtkObject| to |mAtkObject| to show that it is sometimes not from Mai. This will make sense now when you see: IS_MAI_ATK_OBJECT(mAtkObject)

unbelievable, I posted v3 as v4. This one is real.

Comment on attachment 233198 [details] [diff] [review] real v4 Static variables use "s" as a prefix. Why did you change the name of sAccessibilityEnabled to mAccessibilityEnabled

Comment on attachment 233198 [details] [diff] [review] real v4 + if (IS_MAI_OBJECT(mAtkObject)) + MAI_ATK_OBJECT(mAtkObject)->accWrap = nsnull; Put braces around the statement here. + if (accService) { + AtkObject* gailWindow = gtk_widget_get_accessible(GTK_WIDGET(aDialog)); + accService->AddNativeRootAccessible(gailWindow, getter_AddRefs(accessible)); } Move the trailing brace to the next line +gint RunDialog(GtkDialog *aDialog); Add a comment explaining what this does and when it should be used. +PRBool nsWindow::mAccessibilityEnabled = PR_FALSE; As Aaron mentioned, this should be s or possible g, not m. Are there any other dialogs that need RunDialog?

checked in. I didn't find other GtkDialog in mozilla source except Embed and Setup.