332662 - popup menu could not be grabbed by GOK.

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect)

Description

[Tim Miao]

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

Downdrop list menuitems could not be grabbed by GOK.

Reproducible: Always

Steps to Reproduce:
1. Launch firefox and gok.
2. Visit more than 3 different websites.
3. Select UI Grab->Back->Back(Second Back button) to expand downdrop list menu.

Actual Results:  
This downdrop list menu can not be grabbed by GOK.

Expected Results:  
Firefox downdrop list menu should be grabbed by GOK.

Comment 1

[Philip Withnall (unavailable)]

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

Comment 2

[Nian Liu(n/a in a long time)]

the accessible node is created.

I doubt how gok can grab the dynamic node.

Tim, can you give a scenario that gok can grab similar node?

Comment 3

[Nian Liu(n/a in a long time)]

includes right click popup

Comment 4

[Aaron Leventhal]

Just a guess, but mozilla doesn't seem to fire a window_activate and for gedit i see one.

Comment 5

[Nian Liu(n/a in a long time)]

at-poke events log shows strange:

trace window->active, object->children-changed, state-changed
1.open a new gnome-terminal(or any gtk/gnome application). right click to popup menu

children changed event (object:children-changed:add) 0 0 parent: A|ap:application:gnome-terminal: child: A|co:frame:neoliu@dizzy:~:
generic event 'object:state-changed:visible' A|co:window::(1) (0)
generic event 'object:state-changed:iconified' A|co:window::(0) (0)
generic event 'object:state-changed:focused' A|co|te:terminal:Terminal:neoliu@dizzy:~(0) (0)
generic event 'object:state-changed:focused' A|co|te:terminal:Terminal:neoliu@dizzy:~(0) (0)
generic event 'object:state-changed:focused' A|co|te:terminal:Terminal:neoliu@dizzy:~(0) (0)
generic event 'object:state-changed:active' A|co:frame:neoliu@dizzy:~:(0) (0)
window event 'window:activate' (0) (0) on A|co:window:: with title ''
generic event 'object:state-changed:active' A|co:window::(1) (0)

there is no menu related event.

2.use THAT terminal and right click to popup again

children changed event (object:children-changed:remove) 0 0 parent: A|co:window:: child: <Null>
generic event 'object:state-changed:defunct' A|co|se:menu::(1) (0)
generic event 'object:state-changed:defunct' A|co:window::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:separator::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:separator::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:separator::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te|se:menu::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:check menu item::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te:separator::(1) (0)
children changed event (object:children-changed:remove) 0 0 parent: A|co|se:menu:: child: <Null>
generic event 'object:state-changed:defunct' A|co|ac|te|se:menu::(1) (0)
children changed event (object:children-changed:add) 0 0 parent: A|ap:application:gnome-terminal: child: A|co:frame:neoliu@dizzy:~:
generic event 'object:state-changed:visible' A|co:window::(1) (0)
generic event 'object:state-changed:iconified' A|co:window::(0) (0)
generic event 'object:state-changed:focused' A|co|te:terminal:Terminal:neoliu@dizzy:~(0) (0)
generic event 'object:state-changed:focused' A|co|te:terminal:Terminal:neoliu@dizzy:~(0) (0)
generic event 'object:state-changed:focused' A|co|te:terminal:Terminal:neoliu@dizzy:~(0) (0)
generic event 'object:state-changed:active' A|co:frame:neoliu@dizzy:~:(0) (0)
window event 'window:activate' (0) (0) on A|co:window:: with title ''
generic event 'object:state-changed:active' A|co:window::(1) (0)

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

The relevant gok logic is in the source file gok-spy.c (search for 'popup')...  For popups IIRC gok expects to get a window deactivate event on whatever window had focus, and a focus event on something with a menu or menu-item role, if these conditions are met, the expected behaviour is for gok to auto-branch to a new keyboard representing the popup.  (Sorry very busy here)

Comment 7

[Nian Liu(n/a in a long time)]

atk_object_get_role returns ATK_ROLE_MENU, then call
atk_object__notify_state_change(accWrap->GetAtkObject(),
                                       ATK_STATE_FOCUSED, TRUE)

gok doesn't get into  gok_spy_process_focus

did I miss something here?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Nian,

Can you confirm there is no "pointer grab" while the popup is posted?  I'm not sure why gok_spy_process_focus would not be entered.

Another thing you might be able to try is at-spi/test/even-listener-test to see if that catches the event.

Comment 9

[Nian Liu(n/a in a long time)]

Attachment: patch

atk_focus_tracker_notify does the trick.
still not sure why atk_object_state_change not work

Comment 10

[Aaron Leventhal]

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

Please declare parent right when it's used, like:
AtkObject *parent = atk_object_get_parent(accWrap->GetAtkObject());

You're sure it does not leak, correct?

I don't think the parent ATK object will always be the previously activated window. For example, DHTML menus are just in the DOM tree. See http://www.mozilla.org/access/dhtml/spreadsheet
With menus like that the parent could be anything.
So, the code to get the previously activated window may need to up the ancestor chain until it reaches a window, or do something more complex like that. If you do something like that you'll want to call it something besides parent.

Please have Ginn Chen review the next patch because I will be on vacation next week.

Comment 11

[Nian Liu(n/a in a long time)]

Attachment: patch addressed aaron's comments

Comment 12

[Ginn Chen]

Comment on attachment 223261 [details] [diff] [review]
patch addressed aaron's comments

1) if we emit deactivate signal here, when will the window get activate signal?

2) do we need care about EVENT_MENUPOPUPEND?

3) do not use g_signal_emit(obj, g_signal_lookup())
use g_signal_emit_by_name, or split it into 2 lines, see Bug 327530

4) When you need to declare a variable in a switch, put the block in braces, i.e. case { } break;

5) don't use myPtr != nsnull or myPtr == nsnull

6) do add spaces to empty line

Comment 13

[Nian Liu(n/a in a long time)]

Attachment: patchv3

address ginn's comment

Comment 14

[Ginn Chen]

I found sometimes atk_object_get_parent(focusedObj) returns 0, and then we fail to send out windowdeactivate event.
e.g. click the down arrow right to location bar repeatedly.

Neo, can you investigate it?

Also we don't want to fire activate signal to mActivatedWindow in EVENT_MENUPOPUPEND case, if we fail to get mActivatedWindow in EVENT_MENUPOPUPSTART case.

Comment 15

[Nian Liu(n/a in a long time)]

Attachment: patch

1. not only make window active, also notify atk that it's focused.
2. clear mActivatedWindow for each popup

Comment 16

[Ginn Chen]

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

For EVENT_MENUPOPUPEND, should fire focus event to the focused widget instead of mActivatedWindow

I'm not sure if it is good to add another private member to record focused object.
We didn't use atk_get_focus_object in mozilla before.

Comment 17

[Nian Liu(n/a in a long time)]

Attachment: patch

agree, test seems ok.

Comment 18

[Aaron Leventhal]

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

I am still skeptical about this patch. Does it handle nested menu events when there is a sub-sub menu? It looks like the second menupopupstart would fire an extra deactivate for the document object.

Also I would like some comments in the code since it is not obvious to new people what the while() loop does. I'm concerned that as new roles get added for new types of top level objects, the code won't get updated.

Comment 19

[Aaron Leventhal]

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

what if the previously focused object has been destroyed before the menu goes away, i think it will crash

Comment 20

[Aaron Leventhal]

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

what if the previously focused object has been destroyed before the menu goes away, i think it will crash

Comment 21

[Aaron Leventhal]

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

Because of this I recommend using gLastFocusedNode instead of caching, as Ginn also mentioned.

Comment 22

[Aaron Leventhal]

Note: bug 339237 may be affecting your current results.

Comment 23

[Nian Liu(n/a in a long time)]

(In reply to comment #21)
> (From update of attachment 224154 [details] [diff] [review] [edit])
> Because of this I recommend using gLastFocusedNode instead of caching, as Ginn
> also mentioned.
> 

lxr gLastFocusedNode and find it's set value in nsRootAccessible.cpp 431, 721 and 814. and hence
gLastFocusedNode is not reliable in two scenerio:
1.after popupshows and focus on a menuitem. close popup, crash
2.same as cache in patch. dom node destroyed, close popup, crash

anyway we can get focused node in a certain window? that will solve both cache and gLastFocusedNode issue.

Comment 24

[Nian Liu(n/a in a long time)]

(In reply to comment #18)
> (From update of attachment 224154 [details] [diff] [review] [edit])
> I am still skeptical about this patch. Does it handle nested menu events when
> there is a sub-sub menu? It looks like the second menupopupstart would fire an
> extra deactivate for the document object.
haven't found a test case. however, second popup should be ok since first popup is focused before, second popup only deactive first popup not document obj.
> 
> Also I would like some comments in the code since it is not obvious to new
> people what the while() loop does. I'm concerned that as new roles get added
> for new types of top level objects, the code won't get updated.
> 
via which role and how? 

Comment 25

[Nian Liu(n/a in a long time)]

(In reply to comment #22)
> Note: bug 339237 may be affecting your current results.
> 

i'll see what i can do for that

Comment 26

[Aaron Leventhal]

Nian, what is the rule of what kinds of objects fire window: events?

We are saying that context menus should, but what is the rule and where is it defined? If we don't have a good definition for that rule from somewhere, then how do we really know if we're deactivating the right thing?

Comment 27

[Aaron Leventhal]

I have the nsRootAccessible part of this fix in bug 339237, under review. However, it now uses popuphiding instead of DOMMenuInactive.

Comment 28

[Nian Liu(n/a in a long time)]

Attachment: patch

Comment 29

[Nian Liu(n/a in a long time)]

Attachment: patch

my file mixed a lot.

Comment 30

[Aaron Leventhal]

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

Nian, sorry could you ask Ginn to review this one? I need to write this article ...

Comment 31

[Ginn Chen]

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

avoid using = in if-expression
+        AtkObject* tmp = atk_get_focus_object();
+        if (tmp=GetAtkWindow(tmp))
Why not just
AtkObject* tmp = GtkAtkWindow(atk_get_focus_object());
if (tmp) ...

Same problem
+           AtkObject* obj = tmp->GetAtkObject();
+           if (obj = GetAtkWindow(obj)) {
We also don't want to call tmp->GetAtkObject() twice.

+         rv = NS_OK;
+      }
+
need a "break;" here


I'm not clear about the logic of get focusableNode in EVENT_MENUPOPUPEND.

Comment 32

[Nian Liu(n/a in a long time)]

postpone the fix and look into focus, activate/deactivate related issue in deep

Comment 33

[Nian Liu(n/a in a long time)]

346621 is a super set for this

*** This bug has been marked as a duplicate of 346621 ***

Comment 34

[Nian Liu(n/a in a long time)]

Attachment: patch

popup node logic is that try to find the focusable node which popup popups(popup node itself can be any node in a DOM window).

focusable node can be an element node or a Document

Comment 35

[Ginn Chen]

I found it quite often fails at
+        focusController->GetPopupNode(getter_AddRefs(popupNode));
+        if (popupNode) {

It's random.

Comment 36

[Nian Liu(n/a in a long time)]

(In reply to comment #35)
> I found it quite often fails at
> +        focusController->GetPopupNode(getter_AddRefs(popupNode));
> +        if (popupNode) {
> 
> It's random.
> 

forward/backward is a menupopup not a popup, so there is no popup node for them. the patch solves the bug description and try to recover the focus as much as possible when popup hide.

there is no perfect way to solve menupopup issue. think about submenus. popup node stack may be a solution. however, it's an questionmark that whether it is worth to implement it.

i suggest to check in the patch.

Comment 37

[Ginn Chen]

(In reply to comment #36)
> i suggest to check in the patch.
> 

nope, 
send "window:deactivate" and don't send "window:activate" back will cause a major issue with gok.
also I'm wondering if we should emit "window:activate" for the popup window, too.

Comment 38

[Bill Haneman]

Ginn is right - the AT-SPI and ATK docs indicate that window:activate/window:deactivate determine the "currently active window".  If there is no currently active window (i.e. no window:activate) then the desktop is effectively inaccessible, i.e. this is the situation that occurs when an app which doesn't implement AT-SPI has focus.  So absence of active toplevel implies "inaccessible".

"window:" events should only be fired by toplevel window objects.  In most implementations this means toplevels as seen by the X platform, whether managed or unmanaged.  Popups are usually unmanaged X toplevels, though there may be exceptions.  If a popup window is just another child X window (as opposed to a toplevel which is unmanaged), then "window:" events aren't _strictly_ necessary since the API docs don't explicitly require this.  However in this latter case, GOK is unlikely to react as expected, since its UI logic is mostly built around the concept of the currently active toplevel (which seems sensible).   However GOK does react to "children-changed:add" events which are fired on certain types of containers - those which GOK considers "primary containers".  The container types (i.e. ROLES) which GOK currently recognizes as primary are: ROLE_HTML_CONTAINER, ROLE_SCROLL_PANE, ROLE_FRAME, ROLE_DIALOG, ROLE_ROOT_PANE, and ROLE_APPLICATION.  If a popup child is added to any of the above container types, GOK should see it and react appropriately.  However GOK's logic in the latter case will still differ from the "popup window" case since in the latter case, new content is being _added_ to the existing GUI context which includes the enclosing toplevel, whereas in the popup window case, the old GUI context is being temporarily replaced by a new context consisting only of the popup window.

Comment 39

[Aaron Leventhal]

David, can you look at this and tell us if we still need to do something in Firefox 3, or whether it works now, or what?

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

Working on it.  So far the second back button (see original bug description) is no longer showing up so I'll need to play with accerciser/at-poke to diagnose.  (Noticing other newish gok-ff3 weirdness though... unnecessary "hits" on UI Grab... nothing major... appears GOK thinks some links are editable)

Comment 41

[David Bolter [:davidb] (NeedInfo me for attention)]

Note the second back button is available to me in alpha 7 and I can confirm the bug remains there. What about exposing the xul back button widget that causes the popup as a menu?  (I'm guessing that would be non-trivial because of the way the popup is built?)

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh just noticed bug 279703 -- will need to catch up on that work and see what we need.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

Update: GOK currently does not see the "second" back button -- the one with the dropdown list.

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

Gerd can you confirm?

Comment 45

[alexander :surkov (:asurkov)]

Can you look at this please?

Comment 46

[alexander :surkov (:asurkov)]

David citing:
Closing as WONTFIX since GOK is stale and it is clear we won't have time to fix
this anyways. (Caribou is the new GOK).