GNOME Bugzilla – Bug 169480
Opening main menu with keynav not always getting reported
Last modified: 2006-02-27 15:21:56 UTC
Distribution/Version: JDS Rel3 B30 Using gnome 2.6 and gnopernicus 0.10.0 - Start up gnopernicus with speech enabled. - Open a terminal. - Type in a command (e.g. "ls"). - Wait for the ouput. - Press <Control> + <Escape> to bring up the main menu. Sometimes the main menu doesn't get reported by speech. Also, sometimes, when the user presses <Escape> to close the menu, focus switches back to the terminal but the change in focus is not reported. This happens with other applications aswell but is quite noticeable while using the terminal. These problems can be seen with the braille monitor aswell.
This bug is present because terminal has FOCUSED state even when focus is on the main menu. Transferring to vte.
Remus: according to leena gunda at wipro, this bug is also present for gedit, so they wish to move this back to gnopernicus. "(Following comment is added by leena.gunda wipro.com) Was able to simulate the same bug using gedit. Following are the steps: 1. Open gedit, type some text. 2. Highlight the text. Gnopernicus now starts reading the highlighted text. 3. Activate panel main menu. The panel main menu activation is sometimes not reported by gnopernicus. Moving it back to gnopernicus"
Any progress on this bug? It's AP1.
The situation described in comment #1 is anyway a problem: 2 objects with FOCUSED state (a text and a menu-item). I tried to reproduce same problem in gedit, but with no success. gnopernicus has to deal with 2 kind of text objects. Some sends focus events when they have the keyboard input (present in gnome apps). Others don't (present in mozilla). For second category, first caret-moved event received is converted internally by gnopernicus in a focus event. Same scenario is present for terminal. The _real_ focus is in menu, but, the "terminal" (=text area) updates its content. Because of the FOCUSED state, a caret-moved-event (first after the focus for the focused menu/menu-item) is convert to a focus event and presented.
So, the problem (for gnome-terminal) is present only if output is generated while user tries to use the menu.
If the only problem is the one in comment #5, then this isn't actually a bug.
John: please advise if the problem is only seen when output is generated while the user is navigating the menu (Remus' comment #5). Leena, please give more details regarding how to reproduce this problem using gedit.
I had highlighted quite large text in gedit. While gnopernicus was reading the highlighted text, activated the panel menu. My observation was that gnopernicus did not report the panel menu activation (Note: this is not always reproducible, sometimes the activation is reported same as in case of gnome-terminal). > This happens with other applications aswell but is quite noticeable while using the terminal. John: Could you please specify which other applications you are able to see this problem ?
The FOCUSED state is present for text object for gedit too. This seems to be a more general bug in atk/gail (not sure). I never succeed to get an event in gedit for text object after I opened the menu.
Created attachment 39508 [details] [review] proposed test A simple test program. It retains last text object and displays its states when a new event is received. To run it, copy as at-spi/test/simple-at.c.
Created attachment 39509 [details] [review] the result of previous test program The events which present interest are 1, 3, 4, 5, 7. The FOCUSED state is _still_ present for the text object, even if focus is received for another object (which has FOCUSED state).
Remus: I don't understand your comments in #9. Please explain, as I don't see any problem with events from gedit (other than the fact that two objects are FOCUSSED at once).
That's the problem. gnopernicus reacts wrong if this state (FOCUSSED) is present for an unfocussed text object and that text fires caret-moved events (read comment #4 for more details). This behavior (to react to caret-moved event for a text with FOCUSSED state) is needed because of mozilla behavior.
I still don't understand why this causes problems in the cases described above as test cases; event-listener-test shows that the caret-move events are NOT happenning while navigating the menus in gedit, for instance. In the case of a terminal in which output is still coming, I think gnopernicus should work around the problem by prioritizing focus events higher than caret-move events.
Bill, your answer is already in my commnet #9. Some caret-moved events are _converted_ to focus events (see mozilla). So, no change to prioritize those events.
I see. This seems like the wrong thing to do - is there no other way of solving the mozilla problem?
(sounding like the proverbial broken record) this is a very obvious way of solving this problem: require XEvIE and have Gnopernicus shut-up speech when the user presses most keys on the keyboard (including the arrow keys). All wierd logic in Gnopernicus to deal with these situations goes away.
Yes (#17), but as has been said many times before, we cannot make that change at this time. In the meantime I suggest we find an alternative to converting caret-moved events to focus events in gnopernicus, as I think that will cause problems for gtk+ and Java applications as well (since it means that caret-moved "focus" events will always interleave text-changed events, causing all sorts of problems).
I frankly don't see the value of working more on this bug without making "that change at this time". XEvIE is available with XOrg and can be easily used by any distro that wants it. We are past GNOME 2.10 now as well. I think we should either (a) utilize XEvIE and "fix this bug correctly", or (b) wait until we can.
XOrg's current xevie doesn't work right, see Sun-internal bugs about this. So we can't make the switch yet; furthermore, you and I have both said that the extensive code changes required for such a new event model are too high-risk at this time. This bug is marked AP1, making it an "Urgent" accessibility issue according to Calum's list. I don't think we should ignore such bugs if we can address them in a lower-risk fashion. Otherwise we should lower the bug priority (and we shouldn't do that without consulting the QA team).
Let's see if we can remove the FOCUSSED state from the vte widget when menus are posted; I believe that will cause this bug to no longer appear. Leena, your case of using the menus while gnopernicus is speaking may be an "edge case" which we may ignore for now (since it may not really be a bug in the normal sense of the word).
Oana or Dragan, or Ada: Could you please test with the attached gail patch? I think that it may solve this problem of multiple objects exposing STATE_FOCUSED.
Created attachment 39711 [details] [review] patch to avoid this in gail
Created attachment 39742 [details] otput observed with the new gail Hi Bill, I attached the output of Remuse's test program from comment #10. These were the test conditions: 1) I installed the new gail from CVS HEAD (it already had the patch in) 2) I recompiled at-spi with the program from comment #10. 3) I started gedit 4) In a terminal I launched "at-spi/test/simple-at"
For gote one : 5) I pressed <Control> + <Escape> Notice numbers 10, 13, 16 in the output: while a new focus is recived for the menu items, the text window still has the FOCUSED state.
The gail patch can only work when a menu in the same application is used. It cannot work across applications. The fact is, FOCUSSED is perfectly valid for an object in an application which isn't currently topmost.
Basically gnopernicus must do a different kind of check when deciding if something is "the current focussed object". So it's up to gnopernicus to fix the rest of this problem for the case of objects which are in 'background applications.' In such cases, the state FOCUSSED may remain on the object, this is normal.
I recommend that gnopernicus track the last-focussed At-spi object in the actual event handler for 'focus:' (not in the idle routine), and compare with this object in the other places where you decide if an object has the actual focus. As I've said, it's possible (and in fact common) for every AccessibleApplication to have one element with state FOCUSSED at all times, not just the "foreground" application.
I can confirm that the gail patch works ok, in the same application. I tested with gedit.
Transferring bug to gail.
Transferring back to gnopernicus, as gail _cannot_ address the remaining issue.
Created attachment 45163 [details] [review] proposed patch for gnopernicus This patch converts a caret-moved events into a focus event only if last object detected as being focused doesn't contain this state anymore.
I don't quite understand how the proposed patch solves the problem as well or better than my suggestion in comment #28; why don't you just check the source of the caret-moved event (or its parent, in the case of multiple text children of a focussed text container) to see if it is the last focussed object?
Gnopernicus has a variable called srl_last_focus. This contains always last object for which gnopernicus just reported a focus event. The check you also suggested is present and it is not in the idle. >why don't you just check the source of >the caret-moved event (or its parent, in the case of multiple text children of a >focussed text container) to see if it is the last focussed object? What do you mean by "last focussed object"? Last object for wich a focus event was received fro at-spi? Or last object considered as focused by gnopernicus?
Remus: you said "the check you also suggested is present and it is not in the idle", but I don't see that check in the patch. By "last focussed object" I mean either; you could choose to implement the test by checking against the object last reported as focussed by at-spi, or you could also check against the object last reported as focussed internally by srcore, with slightly different logic.
The check is present in the cvs version.
The patch doesn't have this check, and the check ought to be a new addition. So we aren't understanding each other here.
Remus - if they check is in CVS already, why is this bug occurring? Can you point to the section of code in CVS with the check?
in SRLow.c, line 1813 static gboolean srl_event_is_reported (SRLEvent *event) { ........... else if (srle_is_for_focused_acc (event) || srle_is_for_watched_acc (event)) { .......... } ......... }
The check in code above is done against last 'focused' object for gnopernicus.
In case of this bug, the events received by gnopernicus are: [launch 'ls -alR' // for a lot of output] --text-caret-moved (for the text object, with FOCUSED state) --text-changed:insert (for the text object, with FOCUSED state) --..... (a lot of caret-moved and text-insert events) [press F10 to activate the menu] --focus: for 'File' menu (object with FOCUSED state) --text-caret-moved (for the text object, still having FOCUSED state) (*) --text-changed:insert (for the text object, still having FOCUSED state)(**) --..... (a lot of caret-moved and text-insert events) (**) The event (*) is converted by gnopernicus into a focus event. From now, the focused object is the text (even if the menu is opened). All (**) event are now for focused object, so, they get presented to used (code in comment #38).
Yes, I think this is the confusion/miscommunication. When you press F10, you note in comment #41 above that you get a focus event for the 'File' menu. At that point the 'File' menu should be cached as the "last object with focus", and when you get a text-caret-moved event (in the "press F10" block above in comment #41) it should be rejected because it isn't from the cached "object that last sent a focus event"). In code, in the function srl_event_is_reported() function, perhaps the first thing to do is test to see whether the event type is a FOCUS event, and if so, stuff the source of that event into a static variable. Then, in the logic you cite in comment #39 (or perhaps somewhere else in that same function), compare the object that sent the event you are processing with the cached static last-focus-event-sending-object, and if they are different, ignore this event. If they are the same, the continue on with the rest of your logic - does this event have the FOCUSED state in its StateSet, etc. Make sense?
Peter, your scenario will work for all gtk apps, but not for mozilla. In mozilla, for text objects no "focus:" event is received. Because of that, all caret-moved events will be rejected and user will never get feedback. The real issue is that a caret-moved event is converted to a focus event by gnopernicus. The detection of the context when this change is needed is the problem. Because of mozilla, if the caret-moved event is not for last locused object (for gnopernicus) but for an object whith FOCUSED state it is converted to a focus event. This heuristic fails in this case: 2 objects with focused state a menu and a text, and second fires caret-moved events (same case as in mozilla).
The root of the problem seems to be the patch for bug #134003, comment #27. It contains the code: if (GTK_WIDGET_HAS_FOCUS (widget)) { AtkObject *focus_obj; focus_obj = g_object_get_data (G_OBJECT (accessible), "gail-focus-object"); if (focus_obj == NULL) atk_state_set_add_state (state_set, ATK_STATE_FOCUSED); (*) } Line (*) seems to be the problem.
Comment on attachment 39711 [details] [review] patch to avoid this in gail With line specified in comment #44 removed, this patch is not anymore necessary.
we can't revert that patch, it is correct! When a gtk widget has focus, and emits a focus event, it must set ATK_STATE_FOCUSED. transferring back to gnopernicus.
I'm going to look at this more closely this afternoon, in case I'm misunderstanding. I don't know what the side-effects of setting STATE_FOCUSSED when there is no focus object might be.
>focus_obj = g_object_get_data (G_OBJECT (accessible), "gail-focus-object"); This object is set with g_object_set_qdata. I put debug messages before every call that may set this value in gail/gail/gail.c file. This function was called only for combo-box (in gedit/Open dilaog). So, The problem is not neccessary the code in my comment #44, but the code this rely on. >if (GTK_WIDGET_HAS_FOCUS (widget)) Why this check is done for gtk and not for atk. It can be replaced with something like if (gtk_widget_get_accessible (widget) == atk_get_focus_object ()) (not checked and not sure about names). So, I _still_ belive this is a gail/gtk/etc bug.
Created attachment 45287 [details] [review] patch for gail testing This patch displays a message before any call that may change the value returned by g_object_get_data (G_OBJECT (accessible), "gail-focus-object").
Created attachment 45288 [details] [review] patch for gail This patch contains the idea sustained at the end of my comment #48. Probably a middle way between what exists and this patch is the best solution.
Bill, how should an object react to user action(s) if it has FOCUSED state?
If an object has the FOCUSED state, it should respond to user keyboard action *IF* it is in the foreground application.
when I say 'respond to user keyboard action', perhaps more precisely it "should be the recipient of user keyboard action". Exceptions would be keystrokes which were "system global shortcuts" such as Alt-Tab. An object which is FOCUSED may react to user keystrokes it receives, or it may ignore them, if no corresponding action is defined. So for instance, a focusable 'label' may receive keystrokes but do nothing with them (unless they "Tab", "Shift-Tab", "right-arrow", etc. in which case focus will move within the focus chain).
Comment on attachment 45288 [details] [review] patch for gail the test for "if focus_obj == NULL" must be retained, this patch discards it. The replacement of the GTK_WIDGET_HAS_FOCUS with atk_get_focus_object() does not appear to be correct.
Note that in the CVS version of gailwidget.c, we already test to see if the widget that's about to have STATE_FOCUSED set is the focus widget: if (GTK_WIDGET_HAS_FOCUS (widget) && (widget == focus_widget)) { AtkObject *focus_obj; focus_obj = g_object_get_data (G_OBJECT (accessible), "gail-focus-object"); if (focus_obj == NULL) atk_state_set_add_state (state_set, ATK_STATE_FOCUSED); } So, why aren't you seeing this code in your version of gail?
>the test for "if focus_obj == NULL" must be retained, this patch discards it. >The replacement of the GTK_WIDGET_HAS_FOCUS with atk_get_focus_object() does >not appear to be correct. As I said in comment #50, a middle way is the best choise, the intention of my patch is just to prove that it works.
>So, why aren't you seeing this code in your version of gail? I have an older version of gail.
>If an object has the FOCUSED state, it should respond to user keyboard action >*IF* it is in the foreground application. In our case: 1. gnome-terminal runs (it is the _foreground app/window) 2. a commnad with a lot of output runs (ls -alR) [the text object _has_ _FOCUSED_ state and _responds_ to "user keyboard action"] 3. F10 is pressed, the menu is now displayed ["File" menu _has_ _FOCUSED_ state and and _responds_ to "user keyboard action"] [the text object _has_ _FOCUSED_ state, but, _doesn'trespond_ to "user keyboard action"] So, as I already said in comment #4 2 objects with FOCUSED state is not a _normal situation".
Your case in comment #58 no longer occurs with the new gail, as far as I know. This is what the previous discussion, from comments 4 through 23, was about.
I don't know what you mean by 'a middle way'. The gail patch is not appropriate, and in fact should not be necessary (since the current libgail already does a check for "is this the focus object").
"midle way" = something containing code from what exists (commented in my patch) and from my patch. With the patch, the bug is no longer present in case of moving focus from the terminal to another window (case not covered by patch in comment #23).
I don't believe that any of your patch can be applied without causing regressions.
Even if I and Dragan (comment #25) were able to reproduce the bug Bill's patch, now I am not. Really strange. I noticed once 2 versions of the gail library on my computer. Also, Bill said in comment #26: >The gail patch can only work when a menu in the same application is used. It >cannot work across applications. the patch seems to work for these situations too. Also, I the focus_widget and atk_get_focus_object are same object. So, my proposed check performs same action as Bill's patch. Probably the bug is no longer present. I will recheck on a fresh system before closing it.
In voice discussion, BAUM says that Bill/Peter's suggestion will fail with Mozilla because individual text objects in Mozilla don't fire focus events, so Gnopernicus has to 'convert' caret events to focus events, which means that caching the Gnopernicus focus event fails to fix *this* bug, and caching only atk focus objects will bring an issue with Mozilla. Bill/Peter believe that mozilla should at least fire focus events for the HTML container. So here's my suggestion: 1. on every atk_focus event, cache the atk focus object 2. on every caret event, do NOT convert the caret event to a focus event; instead do the following: - if (source of the caret event == cached atk focus object) { if (source of caret event has ATK_STATE_FOUCSED) { present caret event } } else if (source of caret event has ATK_STATE_FOUCSED) { search ancentors: if (ancestor == cached atk focus object) { } present caret event } If this fails with mozilla, it is a mozilla bug. The HTML container in mozilla at least should fire a focus event when you can move the caret within it.
Peter, your algorithm looks good to me. I agree about the assessment that failure to fire the necessary focus event from the HTML container would be a mozilla bug, and should be fixed in mozilla.
The focus event is fired for HTML container for mozilla and yelp.
Created attachment 45429 [details] [review] proposed patch for gnopernicus
Created attachment 45431 [details] [review] proposed patch1
*** Bug 300191 has been marked as a duplicate of this bug. ***
Created attachment 45433 [details] [review] proposed patch2
Created attachment 45434 [details] [review] proposed patch3
First patch caches the last focus object from at-spi. When a caret-moved event is received, the check for an accessible ancestor is made using this cached value and not checking for FOCUSED state. First caret-moved event for a child (or a child of a child etc) of last at-spi focus object is _still_ converted to a focus event by gnopernicus. The second patch is same as previous, but, before checking if an ancestor is last at-spi focus object checks if source of event has FOCUSED state. This patch works with mozilla, but fails to work with yelp. The third patch is same as the second, but no caret-moved event is converted to focus event. The only difference (for user) between second and third patch is that the last reports always the current word when move from one object to another and the second reports the current line. Peter, Bill, which one do you prefer?
Remus - you mention 3 patches, but I count four (comments #67, #68, #70, and #71). Should I ignore the patch in comment #67, or look at all four, or should I look at #68 and #69 and #70 as they *apply* to the "base" patch #67)? In all four patches you modify srl_set_last_focus() with the test 'if (srle_has_type (event, SRL_EVENT_FOCUS))'. This is not what I was suggesting in comment #64 (please re-read item #1). Cache the last at-spi focus event NO MATTER WHAT. You cannot assume the FOCUS state will be set at the time you receive the focus event; requiring that will cause problems. Beyond that, what I'd expect to see in this patch is a change to srl_event_is_reported() that explicitly checks to see that the source of a CARET event is either the cached at-spi last focus object, or a child of it. E.g.: srl_text_event_is_reported (SRLEvent *event, SRLLastInfo info)) { ... else if (srle_has_type (event, SRL_EVENT_TEXT_CARET_MOVED)) { // NEW CODE HERE if (event == cached-last-focus && srle_has_type (event, SRL_EVENT_FOCUS)) { rv = srl_info_is_caret_moved (info, info2); // NEW FUNCTION IN ELSE BELOW } else if (get_ancestor_object(event) && srle_has_type (event, SRL_EVENT_FOCUS) { rv = srl_info_is_caret_moved (info, info2); } } ... } Make sense now?
>Remus - you mention 3 patches, but I count four (comments #67, #68, #70, and >#71). Should I ignore the patch in comment #67, or look at all four, or should >I look at #68 and #69 and #70 as they *apply* to the "base" patch #67)? #67 is marked as obsolite. The 3 patches are #68, #69, #70. >In all four patches you modify srl_set_last_focus() with the test 'if >(srle_has_type (event, SRL_EVENT_FOCUS))'. This is not what I was suggesting in >comment #64 (please re-read item #1). Cache the last at-spi focus event NO >MATTER WHAT. You cannot assume the FOCUS state will be set at the time you >receive the focus event; requiring that will cause problems. You are talking about code: +static gboolean srl_set_last_focus (Accessible *acc) { if (srl_last_focus) @@ -1961,6 +1974,9 @@ srl_assert (event); + if (srle_has_type (event, SRL_EVENT_FOCUS)) + srl_set_last_at_spi_focus ((Accessible*)srle_get_acc (event)); + The function srl_set_last_at_spi_focus ((Accessible*)srle_get_acc (event)) is NOT called in srl_set_last_focus. This call is in FIRST function called after the callback called by at-spi (in srl_process_event). The event has type SRL_EVENT_FOCUS _ONLY_ for focus events received from at-spi. Lines starting with 1961 ARE NOT part of srl_set_last_focus. This is the diff, not the _ENTIRE_ code. >Beyond that, what I'd expect to see in this patch is a change to >srl_event_is_reported() that explicitly checks to see that the source of a >CARET >event is either the cached at-spi last focus object, or a child of it. E.g.: > > srl_text_event_is_reported (SRLEvent *event, SRLLastInfo info)) { > ... > else if (srle_has_type (event, SRL_EVENT_TEXT_CARET_MOVED)) { > // NEW CODE HERE > if (event == cached-last-focus && srle_has_type (event, SRL_EVENT_FOCUS)) { > rv = srl_info_is_caret_moved (info, info2); > // NEW FUNCTION IN ELSE BELOW > } > else if (get_ancestor_object(event) && srle_has_type (event, >SRL_EVENT_FOCUS) > { > rv = srl_info_is_caret_moved (info, info2); > } > } > ... > } This breaks a little the original design. First call, srl_precess_event makes all necessary things to report or to not report an event. It calls srl_event_is_reported. This function decides if an event is or is not reported. In this function, for current focused object other supplementary functions are called. For text events srl_text_event_is_reported is called. The purpose of this function is to reject the event which are side-effect of other. For example, in case of typing a text, for every char typed 2 events are received text-insert and caret-moved. Only text-insert is reported. The second event (a side-effect of typing) is rejected by this function srl_text_event_is_reported). So, if that function is called, gnopernicus detects that the event is for current gnopernicus focus object, what is wrong. The decision should be before this call. As your code suggests a check against last at_spi focus object has to be done. This is done in all my patches. In patch #68 for example, the path is: srle_change_type (SRLEvent *event) { .............. else if (srle_has_type (event, SRL_EVENT_TEXT_CARET_MOVED)) { if (!srle_is_for_focused_acc (event) && /* no need to change type */ !srle_is_for_watched_acc (event) && /* no need to change type */ srle_is_for_focused_ancestor_acc (event)) event->type = SRL_EVENT_FOCUS2; } ........... } srl_event_is_reported (SRLEvent *event) { ........... else if (srle_has_type (event, SRL_EVENT_FOCUS2)) rv = TRUE; else if (srle_is_for_focused_acc (event) || srle_is_for_watched_acc (event)) { .............. else if (srle_has_type (event, SRL_EVENT_TEXT_CHANGED_INSERT) || srle_has_type (event, SRL_EVENT_TEXT_CHANGED_DELETE) || srle_has_type (event, SRL_EVENT_TEXT_CARET_MOVED) || srle_has_type (event, SRL_EVENT_TEXT_SELECTION_CHANGED)) rv = srl_text_event_is_reported (event, *last_info); ............ } } srl_process_event (SRLEvent *event) { ................... srle_change_type (event); .................... if (srle_has_type (event, SRL_EVENT_FOCUS) || srle_has_type (event, SRL_EVENT_FOCUS2)) srl_set_last_focus ((Accessible*)srle_get_acc (event)); ........................ process = srl_event_is_reported (event); if (process) srl_report_event (event); } so, in case that the source of the event is not itself last_at_spi focus or has no ancestor which is last_at_spi focus, the event WILL never be presented. So, your suggestion is implemented.
In my comment above I said that the 3 patches are #68, #69, #70. They are #68, #70, #71.
This bug is no longer present with patch from commnet #23. But I agree that the idea to store the last object for which an focus event was received is better than to check for FOCUSED state. Reducing the priority of this bug.
The ideea with storing last at-spi focus was applied for bug #172083. So, this bug is fixed.