235719 - mozilla toplevel windows don't generate at-spi window: events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Bill Haneman]

toplevel windows are expected to give notification when they are created,
restored, maximized, minimized, activated, and deactivated.  mozilla toplevel
windows do not emit these notifications (at least, they do not emit :activate
and :deactivate).  This can be confirmed by:

* launch mozilla on an accessibility-enabled GNOME desktop
* launch ./event-listener-test in an xterm window
  [NOTE: do not launch event-listener-test in a gnome-terminal, as it will
   recursively notify itself of text change events in the output terminal window]
* note that when mozilla windows are activated (i.e. brought to the front) and
deactivated, no "window:activate" and "window:deactivate" events are emitted as
expected.

For contrast, note the events emitted when GNOME windows such as gedit or
gnome-terminal are activated, deactivated, etc.

This really breaks assistive technologies since ATs rely on these events to
detect the current application context.

Comment 1

[Kyle Yuan]

-> Louie

Comment 2

[Kyle Yuan]

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

Comment 3

[Bill Haneman]

The previous "dup" comment was incorrect, I believe.

Comment 4

[Kyle Yuan]

Yes, it should be duplicated with bug 235687.

Comment 5

[Louie Zhao]

Attachment: patch v1

Comment 6

[Louie Zhao]

Attachment: patch v2

Comment 7

[Kyle Yuan]

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

Louie, does the new patch make mozilla work with GOK?

Comment 8

[Louie Zhao]

This patch only fix the problem of sending "window:activate" and
"window:deactivate" events. Mozilla still can't work with GOK (in the latest
build of GNOME)

Comment 9

[Bill Haneman]

Once this patch is in cvs (HEAD?) I can diagnose the other GOK issues. 
Mozilla's menu and toolbar structure is a bit unusual so GOK patching might be
required, or there could be other moz ATK bugs at work.

Comment 10

[Kyle Yuan]

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

>+    nsCOMPtr<nsIAccessible> rootAcc;
>+    DispatchAccessibleEvent(getter_AddRefs(rootAcc));
>+
>+    if (rootAcc) {

Just one question, why do you try to get rootAcc through
DispatchAccessibleEvent here, rather than use mRootAccessible directly?

Comment 11

[Kyle Yuan]

(In reply to comment #9)
> Once this patch is in cvs (HEAD?) I can diagnose the other GOK issues. 
> Mozilla's menu and toolbar structure is a bit unusual so GOK patching might be
> required, or there could be other moz ATK bugs at work.

Bill, it will be great if you can help us on this, but I doubt that Peter can
wait us for fixing this bug in mozilla trunk. Could you just tell us what
criteria GOK needed to activate its "Toolbar" and "Menu" button?

Comment 12

[Louie Zhao]

Only Top level nsWindow has mRootAccessible. But activate and deactivate event
are  handled by normal nsWindow whose mRootAccessible is NULL. Using
DispatchAccessibleEvent, preshell will using AccessibilityService to get root
accessible (we get the right one through cache if it has been created). Please
refer to
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsPresShell.cpp#5990

Comment 13

[Bill Haneman]

Hi Kyle:
It's a pretty complex algorithm - 
* GOK sees the window:activate
* it rebuilds the component tree, traversing into "interesting" components
  and adding them to the component list
* it then checks the "interesting" list against a match criteria to determine
whether they should be placed on the keyboard.

If GOK isn't showing the 'Menus', 'Toolbars', and 'UI Grab' buttons as active
when  mozilla is focussed, and GOK is receiving the 'window:activate' event
normally, then the moz components aren't making it into the component tree.  

Are the bottom three buttons in GOK getting "re-sensitized" (that is, not
"greyed out") ?  I can give more diagnostic info if you can report this answer.

It also may be helpful to rebuild GOK with --enable-logging-normal in the
autogen line and run it in an xterm (don't run it in gnome-terminal, you'll
probably kill vte ;-)

Comment 14

[Louie Zhao]

The bottom three buttons in GOK is "greyed out" when mozilla gets focused.

Comment 15

[Michael Lew]

On AIX, the first patch seemed to fix the problem with the Menus button 
becoming active.  It does not make the toolbar and UI Grab items active.  This 
is especially a problem when bringing up the profilemanager for instance. 

The first patch does not fix/address the problem if there are are 2 mozilla 
windows open and you are using GOK.  That problem seems to be similar in that 
instead of only sending back the information for the top window, the 
information for all Mozilla windows shows up so that you have 2 (or more 
depending on how many windows you have open) of each menu item.  

Comment 16

[Bill Haneman]

Michael: your report suggests that the window: events might be emitted on the
wrong object (for instance the parent application object, whose children have
their own menus).

Comment 17

[Bill Haneman]

The other reason I see already why mozilla and GOK don't work together at the
moment is the fact that the toplevel windows don't have ATK_STATE_SHOWING, which
they need in addition to ATK_STATE_VISIBLE ("visible" means mapped, "showing"
means actually on-screen).  Gok will not descend into containers that are not
showing, as a rule (it makes exceptions for menus however, but won't see them if
they are inside containers that aren't showing).

These windows' ATK_ROLE should be ATK_ROLE_FRAME, by the way, see atk docs:
http://developer.gnome.org/doc/API/2.0/atk/AtkObject.html#AtkRole

It's also unusual for menus to live inside toolbars, so this might be a minor
issue as well, though it would be reasonable to change GOK so that it searches
inside toolbars for menus if this turned out to be blocking things.

Regards,
Bill

Comment 18

[Kyle Yuan]

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

a few comments:
1) I agree with what you said in comment 12, but please use docAcc or winAcc
instead of rootAcc which is a confusing name.
2) looking at at-spi/bridge.c, there are many other kind of window event,
create, destroy, minimize, maximize, restore. Any chance to implement all of
them?
3) Please address what Michael said in comment 15, i.e. whether the window:
events are omitted from a correct widget.

Comment 19

[Louie Zhao]

Attachment: patch v3

This is a new patch which addresses the issues below.
In the previous patch, when "deactivate", signal are emmitted on
nsDocAccessibleWrap instead of nsRootAccessibleWrap. In this patch, all
"activate" and "deactivate" events are emmited on the same
nsRootAccessibleWrap. In addition, this patch has created other signals such as
"maximize", "minimize" for MaiAtkObject.

Comment 20

[Bill Haneman]

the code recently sent my way (mozilla-1.6-a11y-03-11.tgz) "CSUN" is wrong. 
These window events should NOT be fired on the mozilla topmost accessible (i.e
the one that is supposed to implement AtkApplication), but on the "toplevel"
"window" AtkObject, which should have ATK_ROLE_FRAME.  Please double check the
patches above to make sure they are doing the right thing; you may wish to run
./event-listener-test and compare accessible addresses to confirm that window
events are coming from the same relative objects in Mozilla as from other
applications.  Also please compare at-poke output to see the differences in
AtkState and AtkRole.

Comment 21

[Louie Zhao]

(In reply to comment #20)
> the code recently sent my way (mozilla-1.6-a11y-03-11.tgz) "CSUN" is wrong. 
> These window events should NOT be fired on the mozilla topmost accessible (i.e
> the one that is supposed to implement AtkApplication), but on the "toplevel"
> "window" AtkObject, which should have ATK_ROLE_FRAME.  

These events are emitted on "toplevel" AtkObject, that is nsRootAccessibleWrap,
  not the topmost accessible. In build mozilla-1.6-a11y-03-11.tgz, we haven't
set ATK_ROLE_FRAME to "toplevel" AtkObject. But this is fixed by the latest
patch and in mozilla-1.6-a11y-03-12.tgz. For AtkState, ATK_STATE_SHOWING is set. 

Comment 22

[Philip K. Warren]

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

>Index: accessible/src/atk/nsAccessibleWrap.cpp
>===================================================================
>RCS file: /export/src/cvs/mozilla1.6/accessible/src/atk/nsAccessibleWrap.cpp,v
>retrieving revision 1.2
>diff -u -r1.2 nsAccessibleWrap.cpp
>--- accessible/src/atk/nsAccessibleWrap.cpp	2004/02/18 10:36:50	1.2
>+++ accessible/src/atk/nsAccessibleWrap.cpp	2004/03/11 09:20:23
>@@ -55,6 +55,18 @@
> 
> /* MaiAtkObject */
> 
>+enum {
>+  ACTIVATE,
>+  CREATE,
>+  DEACTIVATE,
>+  DESTROY,
>+  MAXIMIZE,
>+  MINIMIZE,
>+  RESIZE,
>+  RESTORE,
>+  LAST_SIGNAL
>+};
>+
> /**
>  * This MaiAtkObject is a thin wrapper, in the MAI namespace, for AtkObject
>  */
>@@ -73,6 +85,8 @@
>     AtkObjectClass parent_class;
> };
> 
>+static guint mai_atk_object_signals [LAST_SIGNAL] = { 0, };
>+
> #ifdef MAI_LOGGING
> PRInt32 sMaiAtkObjCreated = 0;
> PRInt32 sMaiAtkObjDeleted = 0;
>@@ -554,6 +568,72 @@
>     aClass->initialize = initializeCB;
> 
>     gobject_class->finalize = finalizeCB;
>+
>+    mai_atk_object_signals [ACTIVATE] =
>+    g_signal_new ("activate",
>+                  MAI_TYPE_ATK_OBJECT,
>+                  G_SIGNAL_RUN_LAST,
>+                  0, /* default signal handler */
>+                  NULL, NULL,
>+                  g_cclosure_marshal_VOID__VOID,
>+                  G_TYPE_NONE, 0);
>+    mai_atk_object_signals [ACTIVATE] =
>+    g_signal_new ("create",
>+                  MAI_TYPE_ATK_OBJECT,
>+                  G_SIGNAL_RUN_LAST,
>+                  0, /* default signal handler */
>+                  NULL, NULL,
>+                  g_cclosure_marshal_VOID__VOID,
>+                  G_TYPE_NONE, 0);
>+    mai_atk_object_signals [ACTIVATE] =
>+    g_signal_new ("deactivate",
>+                  MAI_TYPE_ATK_OBJECT,
>+                  G_SIGNAL_RUN_LAST,
>+                  0, /* default signal handler */
>+                  NULL, NULL,
>+                  g_cclosure_marshal_VOID__VOID,
>+                  G_TYPE_NONE, 0);

Did you mean to use ACTIVATE/CREATE/DEACTIVATE/DESTROY here to index into the
array instead of ACTIVATE in all places?

Comment 23

[Kyle Yuan]

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

>+  enum { ROLE_RULER = 72U };               // ATK_ROLE_RULER
>+  enum { ROLE_AUTOCOMPLETE = 74U };        // ATK_ROLE_AUTOCOMPLETE

Please remove those ROLE stuff from this patch, they should belong to bug
237086. And we need more job to do than just make RootAccessible to ROLE_FRAME.
Other ATK roles also need to be well addressed.

>+  printf("nsDocAccessible::GetState\n");

Please either remove this line or put it in #ifdef DEBUG/#endif

>+  PRUint32 state;
>+  nsAccessible::GetState(&state);
>+  if (!(state & STATE_INVISIBLE)) {
>+    *aState |= STATE_SHOWING;
>+  }

As I said before, we need a beeter job for the STATE_SHOWING thing. So please
do it in bug 237089.

>+void
>+nsWindow::GetRootAccessible(nsIAccessible** aAccessible)
>+{
>+    nsCOMPtr<nsIAccessible> docAcc;
>+    DispatchAccessibleEvent(getter_AddRefs(docAcc));
>+    PRUint32 role;
>+
>+    while (docAcc) {
>+        docAcc->GetRole(&role);
>+        if (role == nsIAccessible::ROLE_FRAME) {
>+            *aAccessible = docAcc;
>+            NS_ADDREF(*aAccessible);
>+            break;
>+        } 
>+        docAcc->GetParent(getter_AddRefs(docAcc));
>+    }
>+}

The rootAcc and docAcc is confusing. I'm okay if you want to use rootAcc here,
but please use them consistently.

Comment 24

[Louie Zhao]

 
> Did you mean to use ACTIVATE/CREATE/DEACTIVATE/DESTROY here to index into the
> array instead of ACTIVATE in all places?
> 
These codes create signal ACTIVATE/CREATE/DEACTIVATE/DESTROY for MaiAtkObject.
You can refer to the implementation of "gail/gailwindow.c".

Comment 25

[Louie Zhao]

Attachment: patch v4

> Please remove those ROLE stuff from this patch, they should belong to bug
> 237086. And we need more job to do than just make RootAccessible to
ROLE_FRAME.
> Other ATK roles also need to be well addressed.

Agree, I have moved unrelated ATK_ROLE and will fix it in bug 237086.

> Please either remove this line or put it in #ifdef DEBUG/#endif

This is for temporary debugging. Have got rid of it.

> As I said before, we need a beeter job for the STATE_SHOWING thing. So please

> do it in bug 237089.

Move this part and will fix it in bug 237089.

> The rootAcc and docAcc is confusing. I'm okay if you want to use rootAcc
here,
> but please use them consistently.

In nsWindow, we can only get nsDocAccessible instead of nsRootAccessible
directly when "deactivate". Func "GetRootAccessible" is to get nsRootAccessible
by going throught parent chain of nsDocAccessible. This is why "docAcc" is used
in "GetRootAccessible".

Comment 26

[Kyle Yuan]

Comment on attachment 143859 [details] [diff] [review]
patch v4

r=kyle

Comment 27

[Henry Jia]

Comment on attachment 143859 [details] [diff] [review]
patch v4

sr=Henry

Comment 28

[Philip K. Warren]

(In reply to comment #24)
>  
> > Did you mean to use ACTIVATE/CREATE/DEACTIVATE/DESTROY here to index into the
> > array instead of ACTIVATE in all places?
> > 
> These codes create signal ACTIVATE/CREATE/DEACTIVATE/DESTROY for MaiAtkObject.
> You can refer to the implementation of "gail/gailwindow.c".

The latest patch does the following:

+    mai_atk_object_signals [ACTIVATE] =
+    g_signal_new ("activate",
...
+    mai_atk_object_signals [ACTIVATE] =
+    g_signal_new ("create",
...
+    mai_atk_object_signals [ACTIVATE] =
+    g_signal_new ("deactivate",
...

so you are storing the new signals in the first entry in the array each time,
overwriting the previous signal. In gailwindow.c, they do the following:

  gail_window_signals [ACTIVATE] =
    g_signal_new ("activate",
  ...
  gail_window_signals [CREATE] =
    g_signal_new ("create",
  ...
  gail_window_signals [DEACTIVATE] =
    g_signal_new ("deactivate",
  ...

Comment 29

[Louie Zhao]

Attachment: Patch v5

Sorry for the careless. This is the right one. Thanks for remindering.

Comment 30

[Philip K. Warren]

Comment on attachment 144007 [details] [diff] [review]
Patch v5

>Index: accessible/public/nsIAccessible.idl
...
>+  // Rerepesent top level window

"Represent"

>Index: accessible/src/atk/nsDocAccessibleWrap.cpp
...
>+    case nsIAccessibleEvent::EVENT_ATK_WINDOW_DEACTIVATE:
>+        MAI_LOG_DEBUG(("\n\nReceived: EVENT_ATK_WINDOW_ACTIVATED\n"));

This should print EVENT_ATK_WINDOW_DEACTIVATED instead of
EVENT_ATK_WINDOW_ACTIVATED.

Comment 31

[Louie Zhao]

Attachment: patch v6

updating patch v5

Comment 32

[Philip K. Warren]

The latest patch fixes the small nits in comment 30, but seems to have reverted
to the problem described in comment 28.

Comment 33

[Louie Zhao]

Attachment: path v7

This is the right one.

Comment 34

[Michael Lew]

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

Comment 35

[Philip K. Warren]

Comment on attachment 144486 [details] [diff] [review]
path v7

Looks good. Thanks for making the previous changes.

Comment 36

[Henry Jia]

Comment on attachment 144486 [details] [diff] [review]
path v7

sr=Henry

Comment 37

[Henry Jia]

Comment on attachment 143859 [details] [diff] [review]
patch v4

remove r= & sr=

Comment 38

[Louie Zhao]

Thanks for sr & r.

Comment 39

[Philip K. Warren]

Comment on attachment 144486 [details] [diff] [review]
path v7

This is an important accessibility fix to allow Mozilla to work properly with
the GNOME onscreen keyboard.

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 144486 [details] [diff] [review]
path v7

a=asa (on behalf of drivers) for checkin to 1.7

Comment 41

[Philip K. Warren]

Checked in.

Checking in accessible/public/nsIAccessible.idl;
/cvsroot/mozilla/accessible/public/nsIAccessible.idl,v  <--  nsIAccessible.idl
new revision: 1.24; previous revision: 1.23
done
Checking in accessible/public/nsIAccessibleEvent.idl;
/cvsroot/mozilla/accessible/public/nsIAccessibleEvent.idl,v  <-- 
nsIAccessibleEvent.idl
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/atk/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessibleWrap.cpp,v  <-- 
nsAccessibleWrap.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in accessible/src/atk/nsAppRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAppRootAccessible.cpp,v  <-- 
nsAppRootAccessible.cpp
new revision: 1.9; previous revision: 1.8
Checking in accessible/src/atk/nsDocAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsDocAccessibleWrap.cpp,v  <-- 
nsDocAccessibleWrap.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/atk/nsRootAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsRootAccessibleWrap.cpp,v  <-- 
nsRootAccessibleWrap.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in accessible/src/atk/nsRootAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/atk/nsRootAccessibleWrap.h,v  <-- 
nsRootAccessibleWrap.h
new revision: 1.5; previous revision: 1.4
done
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.98; previous revision: 1.97
done
Checking in widget/src/gtk2/nsWindow.h;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.h,v  <--  nsWindow.h
new revision: 1.43; previous revision: 1.42
done

Comment 42

[Jesse Ruderman]

I think this checkin is responsible for redwood's Tp going up 15%.  Redwood is a
Firefox build machine that uses xft and gtk2.

http://axolotl.mozilla.org/graph/query.cgi?testname=pageload&tbox=redwood&autoscale=1&days=7&avg=1&showpoint=2004:03:25:11:16:13,1069

Comment 43

[Kyle Yuan]

That's weird, the accessibility feature is turned off by default. I just saw
those two warnings in redwood's build log. Not sure whether it is the cause of
the performance regression. But anyway, Louie should take a look at this problem.

  (Gecko:10313): GLib-GObject-WARNING **: gsignal.c:1085: unable to lookup
signal "deactivate" of unloaded type `MaiAtkObject'
  
  (Gecko:10313): GLib-GObject-CRITICAL **: file gsignal.c: line 2491
(g_signal_emit_valist): assertion `signal_id > 0' failed

Comment 44

[Jesse Ruderman]

Performance regression -> bug 238957

Comment 45

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

Accessibility is on by default.