339554 - at-poke gets a focus-event for a blank panel

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Ginn Chen]

set GTK_MODULES=gail:atk-bridge
open at-poke event log, check "focus" event
start firefox, focus a window, or open "Customize Character Encoding" dialog

you will get something like this,
focus(0, 0, None)[Available Character Encodings:, list]
focus(0, 0, None)[Arabic (IBM-864), list item]
focus(0, 0, None)[Available Character Encodings:, list]
focus(0, 0, None)[None, panel]

Without setting GTK_MODULES, this bug is not reproducible.

Comment 1

[Ginn Chen]

gail_container_real_initialize is called when firefox creates a window
and gail_widget_real_focus_gtk is called when the window is focused

We should allow gail do that to firefox managed window.

Comment 2

[Ginn Chen]

Attachment: patch

gok is doing the same thing to avoid self-occlusion.

I also verified this patch with gok, it doesn't break firefox gok functionality.

Comment 3

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

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

I guess this should be harmless...

Comment 4

[Ginn Chen]

Checking in mozdrawingarea.c;
/cvsroot/mozilla/widget/src/gtk2/mozdrawingarea.c,v  <--  mozdrawingarea.c
new revision: 1.17; previous revision: 1.16
done

Comment 5

[parente]

With the trunk nightly from 2006-06-01, now nothing fires a focus event when a dialog opens (e.g. Preferences, Customize character encoding), even when GTK_MODULES is *not* set:

For the preferences dialog:

generic event 'focus:' A:menu item:Preferences:(0) (0)
window event 'window:deactivate' (0) (0) on A:frame:Minefield: with title 'Minefield'
window event 'window:activate' (0) (0) on A:dialog:Minefield Preferences: with title 'Minefield Preferences'

For the Customize character encoding dialog:

generic event 'focus:' A:menu item:Customize List...:(0) (0)
window event 'window:deactivate' (0) (0) on A:frame:Minefield: with title 'Minefield'
window event 'window:activate' (0) (0) on A:frame:Customize Character Encoding: with title 'Customize Character Encoding'

Expected behavior is that the control with the focus after the dialog opens should fire a focus event.

Comment 6

[Evan Yan]

I believe that's another bug, see bug 339210.

Comment 7

[Ginn Chen]

patch backed out

Thanks, parente!

I was messed up with Bug 339634 while testing.

Comment 8

[Ginn Chen]

I didn't find a way to fix it in Mozilla.

What about we fix it in gail?
Let gail_focus_notify/gail_focus_notify_when_idle ignore GtkWidget typed MozContainer.

We can't just let gail down for Firefox, because we'll need it to fix Bug 332660.

Comment 9

[parente]

> What about we fix it in gail?
> Let gail_focus_notify/gail_focus_notify_when_idle ignore GtkWidget typed
> MozContainer.

Is this a good long term solution for gail? What about other projects besides Firefox that might run into the same problem (e.g. OpenOffice)?

Comment 10

[Ginn Chen]

Attachment: patch (return NULL for MozContainer accessible)

Comment 11

[Ginn Chen]

After a discussion with Padraig O'Briain ([e-mail]),
we think the clean way to fix it is to assign a accessible factory for MozContainer, and return NULL for get_accessible.
gail will add a NULL check to avoid emit focus notification.

We need gail's focus_watcher for file_chooser dialog, so we couldn't unload gail or disable its focus_watcher.

Gecko a11y module takes care of accessible tree and events for all mozilla windows, we do not need to use gail to produce AtkObject* for MozContainer.

OOo also has similar problem, but its usage of GTK_CONTAINER is different, the solution is also different.

Comment 12

[Ginn Chen]

Comment on attachment 224670 [details] [diff] [review]
patch (return NULL for MozContainer accessible)

Bill suggested to not return NULL but return a AtkObject which role is ATK_ROLE_REDUNDANT_OBJECT.

Comment 13

[Aaron Leventhal]

Does ATK_REDUNDANT_OBJECT get special treatment in the infrastructure? Or is it the job of an AT to skip over them?

Comment 14

[Ginn Chen]

Attachment: patch v2 (use ATK_ROLE_REDUNDANT_OBJECT)

preproccessor for atk versions requires part fix of Bug 340773;
also need to patch gail.c to accomplish the fix.

Comment 15

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

Shouldn't Aaron review this?

Comment 16

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

+GType
+mai_redundant_object_factory_get_type (void)
+{
+  static GType type = 0;
+
+  if (!type)
+  {
+    static const GTypeInfo tinfo =
+    {
+      sizeof (maiRedundantObjectFactoryClass),
+      (GBaseInitFunc) NULL, /* base init */
+      (GBaseFinalizeFunc) NULL, /* base finalize */
+      (GClassInitFunc) mai_redundant_object_factory_class_init, /* class init */
+      (GClassFinalizeFunc) NULL, /* class finalize */
+      NULL, /* class data */
+      sizeof (maiRedundantObjectFactory), /* instance size */
+      0, /* nb preallocs */
+      (GInstanceInitFunc) NULL, /* instance init */
+      NULL /* value table */
+    };
+    type = g_type_register_static (
+                           ATK_TYPE_OBJECT_FACTORY,
+                           "MaiRedundantObjectFactory" , &tinfo, 0);
+  }
+
+  return type;
+}

You can simplify this by writing it as

+GType
+mai_redundant_object_factory_get_type (void)
+{
+  static GType tinfo = ...;
+  static GType type = g_type_register_static(...);

The compiler will take care of your "if" statement.

Comment 17

[Ginn Chen]

roc: I can't change it because initializer element must be constant, but g_type_register_static is not.

Comment 18

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

That's not true in C++.

Comment 19

[Ginn Chen]

(In reply to comment #18)
> That's not true in C++.
> 

You're right.
But we still can't do it, because we shouldn't call g_type_register_static before gtk_init
It will cause bug 339634. :)

Comment 20

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

It will get executed the first time mai_redundant_object_factory_get_type is called.

Comment 21

[Mike Schroepfer]

Can you give any thoughts on risk/reward for the 1.8.1 branch for this?  How well tested is this patch?

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(I'm concerned because while it seems reasonable, it also seems like a pretty big change to what we're exposing in many areas, not just this one case.)

Comment 23

[Mike Schroepfer]

Given comment 22 - if there a lower-risk variant or a justification for taking the risk on the branch?  If not we are not likely to take this at this stage of the release.

Comment 24

[Aaron Leventhal]

I'm not sure that we need this for Firefox 2 given that it's already not going to work well on Linux. But if Ginn can justify it for the risk, I'm game.

Comment 25

[Ginn Chen]

(In reply to comment #23)
> Given comment 22 - if there a lower-risk variant or a justification for taking
> the risk on the branch?  If not we are not likely to take this at this stage of
> the release.
> 
I'm sorry for the late.
I'm waiting for Bill Haneman's commit of gail's fix to verify it with gail trunk build.

I think this patch is low risk, and it's useful for some distros.
e.g. Ubuntu 6.06 is setting GTK_MODULES=gail:atk-bridge by default for every application.
Using Firefox with gnopernicus or orca will have this issue.

Comment 26

[Bill Haneman]

I don't know why you're waiting for me.
The gail change is in cvs now anyway.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 224967 [details] [diff] [review]
patch v2 (use ATK_ROLE_REDUNDANT_OBJECT)

Based on Aaron's assertion that the need for this is low on the branch, we're going to pass. Feel free to renominate for a dot release or for Gecko 1.9