GNOME Bugzilla – Bug 542833
Flat review in Thunderbird is largely broken
Last modified: 2008-09-11 23:22:44 UTC
We already have a couple of bugs related to flat review not working in Thunderbird. I just found a couple more issues. :-( While normally we really don't like "uber bugs", I think this is a case where we need to do a full analysis of what the heck is going on (or not going on) and do a clean fix rather than lots of little fixes/hacks/bandaids. If you find a Thunderbird issue with flat review that isn't covered in the comments, please add it as a comment. Thanks!
From Rudolf in bug 542246: ----------------------------- Hi again, I just tested the flat review in thunderbird's message list: Observation: I tabbed into the message list. When the message subject shows I * press flat-review up on the braille display -> Nothing happens * press down on the braille display -> Braille display is positioned on the 2nd line of the screen (i.e., one line under the menu bar.) * Flat review then works fine A possible explaination would be, that the flat review cursor is not "taken along" when arrowing through the message list, but remains on the top row (i.e., on the menu bar). This would explain, why nothing happens, when pressing flat-review-up the first time. On the other hand, the pan left/right keys on the braille display work fine when arrowing through the messages. I'm using orca and thunderbird builds of today. Cheers, Rudolf
*** Bug 542246 has been marked as a duplicate of this bug. ***
From Rudolf in bug 542249: ----------------------------- Hi again, I noticed a 2nd problem with flat review in the Thunderbird message list. I'm filing it separately, as it is not related to the previous report. In folders with many messages (~1500) pressing a flat-review key on the braille display effectively crashes orca, because the request takes immensely long to fulfil. I killed orca when nothing happend for 5 min. It seams, that orca is going to the entire table: ---------> QUEUEING EVENT object:property-change:accessible-name flat_review.getZonesFromAccessible (name=Franz Bozsak role=table cell) flat_review.getZonesFromAccessible (name= role=table cell) ---------> QUEUEING EVENT object:property-change:accessible-name flat_review.getZonesFromAccessible (name=07/23/2007 08:47 PM role=table cell) flat_review.getZonesFromAccessible (name= role=table cell) flat_review.getZonesFromAccessible (name= role=table cell) flat_review.getZonesFromAccessible (name= role=table cell) ---------> QUEUEING EVENT object:property-change:accessible-name flat_review.getZonesFromAccessible (name=[AK Bildung] [Fwd: [LAK-BaWue] Studieng ebühren im Landtag in Baden-Württemberg --- CDU fragt nach guten Erfahrungen] ro le=table cell) flat_review.getZonesFromAccessible (name= role=table cell) ---------> QUEUEING EVENT object:property-change:accessible-name flat_review.getZonesFromAccessible (name=Björn role=table cell) flat_review.getZonesFromAccessible (name= role=table cell) and so on. These message continue until I kill the orca process. In small folders (with ~10 messages) flat review in the message list works as described in my previous report. Thanks for looking into it! Cheers, Rudolf
*** Bug 542249 has been marked as a duplicate of this bug. ***
Other issues: 1. Flat review sometimes fails to see the body of a message. This seems to be the case when the message is plain text, but I'll need to investigate further. 2. Flat review find fails most of the time (both for message content and for headers).
Assigning this to me and setting a target of 2.24. I'm not sure if we'll be able to fix all the issues by then (though I'll try). However, if the problems are the result of thunderbird bugs, we should identify those now and file them with Mozilla. So the 2.24 target is, at a minimum, the target for completing a full analysis of the issues.
Created attachment 116386 [details] [review] issue 1: only pursue objects which are on the screen I tried making my thunderbird window quite small (i.e. so that the list of messages was larger than could be displayed without scrolling). Currently, flat review allows you to review those items which are not visible on the screen. If we check for things with have STATE_SHOWING (we do) *and* STATE_VISIBLE (we don't), this problem goes away. I haven't done thorough testing of this, nor have I regression tested it yet. That said.... Will, does this strike you as a reasonable approach?
(In reply to comment #7) > Created an attachment (id=116386) [edit] > issue 1: only pursue objects which are on the screen > > I tried making my thunderbird window quite small (i.e. so that the list of > messages was larger than could be displayed without scrolling). Currently, flat > review allows you to review those items which are not visible on the screen. If > we check for things with have STATE_SHOWING (we do) *and* STATE_VISIBLE (we > don't), this problem goes away. > > I haven't done thorough testing of this, nor have I regression tested it yet. > That said.... Will, does this strike you as a reasonable approach? It *seems* like it should be fine, though I believe SHOWING should be sufficient. But, if what you've done works, then I say go with it. A more conservative thing might be to isolate the pursueForFlatReview mod to Gecko's script, though. Thanks!
> It *seems* like it should be fine, though I believe SHOWING should be > sufficient. Actually, showing is in part the problem. items that are scrolled off the screen have STATE_SHOWING, so we include them. They lack STATE_VISIBLE. This doesn't solve the issue for trees with a gazillion items, however. :-( I'll try to look at that tomorrow.
Created attachment 116399 [details] [review] proof of concept patch to address the biggest issues Okay.... :-) This is a proof of concept patch. What it does primarily is add a new method - getShowingCells() - to flat_review.py. This method is only invoked if we have a root of ROLE_TREE_TABLE which doesn't manage its descendants and does have more than 50 children. To make it more conservative/specific, it looks to see if the first child is of ROLE_LIST and checks along the way that what we're considering as a showingCell worthy of pursuing for flat review according to the active script. Failing these things, the code that's currently in place to do a child-by-child comparison kicks in as usual. Here's what this (along with a change to the Gecko script's) pursueForFlatReview() buys us based on my testing with a mailbox of over 3500 messages: 1. We only review what's on the screen (currently, as I mentioned before, we review all sorts of things which are not on the screen). Given a tree table with 3500 messages, that's a lot of extra stuff we shouldn't be reviewing. 2. We no longer do the brute force approach with the children in the very specific case described above. 3500 rows of 8 columns each is 28000 cells we were considering for pursuing for flat review. Given that they have STATE_SHOWING in Thunderbird, we went one step further and pursued them. But even when I added the check for STATE_VISIBLE, the mere act of considering 28000 children was enough to functionally hang Orca as Rudolf described. Instead of checking each and every child, we try to narrow it down: * Selected items are fairly likely to be visible (worth a try anyway) * Failing that and/or even the presence of a selected item, we should spot check every screen- (technically scrollpane-) full rather than every row until we find something visible. Then we can back up a bit and search down row by row. * Until we have found the first visible row, there's no point in checking every column -- or even every visible column -- to see if it's worth pursuing. * Having done the above, we should stop as soon as we've found a row that's not visible -- which should be safe given that this only applies to tree tables which do not manage their descendants. In addition, removing the document frame check from the Gecko script's pursueForFlatReview() makes the following broken thing less broken: 1. Open a message 2. Shift Tab to the last header field 3. Try to flat review the message content We typically skip right over it. This change doesn't fix all flat review of message content, but we skip over far less. It also doesn't fix all flat review find issues, but without the patch flat review find is a lot more broken. Because that document frame check was in there to keep us from flat reviewing content from pages in other tabs, I looked at Accerciser and also tested this patch. Content in other tabs does not have both STATE_SHOWING and STATE_VISIBLE and I couldn't cause the bug to occur. <shrug> DISCLAIMERS: * Is pylinted * Is NOT regression tested * Was mostly done between 11 PM and 2 AM * Without much sleep * Using math * Math done in my head I'll properly test it, re-examine the logic, use a calculator, etc. But in the meantime.... Will, I know it's a big change, and I know my natterings above probably aren't encouraging you to give this patch the once-over. ;-) But I think the concept -- and possibly even the logic -- is sound. It improves performance and stops a functional hang. Therefore, if you wouldn't mind doing a preliminary review that would be awesome! Thanks much in advance!!
Created attachment 116401 [details] [review] minor adjustment to the previous proof of concept Oy. I just took one last "before I go to bed" look and caught a minor error. We should only do the initial, incremental search in the absence of a visible, selected item. And that was the intent. But in the previous revision we were doing it no matter what. *sigh* I think this one is right(er). :-) Going to bed now. :-)
Hi Joannie, Will, thanks alot for the patch. This improves the situation alot. Flat review is now fluent except for the first time I press a flat review key. The reaction to this first time is still about 5-10 secs in A larg folder. My impression is, though I don't understand why from the comments above, that the reaction time to arrow keys has also improved. A problem that still exists is that flat review does not start on the currently highlighted message but at the top of the window. Thanks again, Regards, Rudolf
Rudolf, thanks so much for testing this -- especially in it's current, proof-of-concept state! :-) That's great about the reaction time improvement. I'm certainly not going to look a gift horse in the mouth. :-) Were I to hazard a guess.... Perhaps by not including things that are off-screen in the flat review context, we're doing less processing to figure out where we are and "what's next". Or not. I'd have to look at the code closer as I've been away from flat review for a while. Anyhoo.... The delay given a lot of items, and starting from the top of the window rather than from the currently selected item, are noted -- and, sadly, also present in Evolution, and OpenOffice, and probably a number of other applications. :-( Ultimately we need to take a look at flat review across the board. My goal for this particular bug, though, is to get things working as best as we can for Thunderbird before the code freeze. In that spirit, my to-do list beyond finalizing the above is: 1. Why are we skipping over parts of the text in the document frame? 2. Why is flat review find failing to find certain items? With the current patch, this failure seems to take place mostly outside of the document content (i.e. toolbar buttons). Rudolf (and others), are there other unique-to-Thunderbird flat review issues which you are aware of? Thanks again!!
Created attachment 116479 [details] [review] revision 4 (or revision 2 of the proof of concept, should you prefer) :-) Will and I discussed the proof of concept today. Thanks again Will! I think this version does what we discussed, namely: * Moves getShowingDescendants() from flat_review.py to default.py * Adds a getShowingDescendants() to Gecko.script.py to do the work from the proof of concept. * Gets the headers using the table interface rather than looking the first child of the tree table. * Starts in medias res with the first visible row we can find (be it by selected rows or by scrolling the height of the tree table). Then appends everything that is visible after that row, and finally prepends everything that is visible before that row. * Includes requested/discussed comments, null checks, to-dos, printing of exceptions. Questions: * I moved the visible() method out of flat_review.py. The new default script's getShowingDescendants() needed it. We could call it via the flat review context, but it occurred to me that the definition of visible() might vary from script/toolkit to script/toolkit due to wacked-out and/or bogusly-nil extents (right???). Is this okay, or should it remain in flat_review.py? * What is this GRID_SIZE (defaults to 7) of which the flat review code speaks? ;-) The associated comment is "A minimal chunk to jump around should we not really know where we are going". Story of my life, but anyway.... I see how it is used in getShowingDescendants() :-). But it *only* seems to be used in getShowingDescendants(), and it also seems to be something which might one day prove to be script/toolkit-specific(???). So, does it still belong in flat_review.py? For now, it's still there, and passed along as a new argument to getShowingDescendants(). Pylinted and functionally tested. Gonna toss the regression tests at them now.
Joanie, I've done a little quick and dirty testing of the patch. - Flat review easily crosses back and forth between the headers and the message body. - Flat-review still does not follow focus in the message list--it always starts in the menu bar. - Flat review does not update the page if the message is longer than one screen in length. to reproduce, open a message, read a bit with flat review, press page down, then examine text using flat review. the flat review "cursor" is still on the same text as before, but the actual cursor shows very different text. If you want a debug log, let me know. I'll do some more testing later on in the day. Thanks for the effort!
One thing that I just realized hasn't been mentioned wrt flat review in Thunderbird is that pressing keypad-minus does not route flat review to current focus. At present, both with and without the patch, pressing keypad-minus routes flat review to the menu bar, both in the message view and message list. The only place where pressing keypad-minus does work is in the mailbox list.
Sorry, one last thing I forgot to mention. When you are reading a long email (multiple screens full) and you have paged down one page, pressing keypad-minus does refresh the flat review buffer. the problem is that flat review is back in the menu bar, so you have to navigate across the headers to get to the new content. On a related note, it would be nice if Orca could indicate that it is crossing a boundary between the headers and the message body. it is not a problem at the start of the message, but once you are down a screenful or more, it gets a little disorienting to leave the headers and arrive in the middle of a sentence.
I've done some more testing. I see only three other issues: - When moving by word in flat review within the message view window, the flat review "cursor" (for lack of a better name) always lands on the first character of the word, no matter which direction is being traversed. This is inconsistent with word navigation with the carat. - Navigation using Orca_modifier-keypad-4 and Orca_modifier-keypad-6 is inconsistent in the message view window. Sometimes it moves to the end of the current line and other times it moves to the end of the current sentence. Occasionally, it moves to a random location within the line after having been pressed enough. I just looked on the wiki, and the proper functionality for these key chords is not listed. Additionally, Orca_modifier-keypad-1 and Orca_modifier-keypad-3 do not seem to have any functionality. - There are four "push buttons" that should be identified in the main Thunderbird window. Two are on the line above the first directory/message (for me, to the right of the "All Folders" label, and two are on the status bar at the bottom of the window. I'm guessing that this is the expected behavior (it has been too long since I have read the documentation ;-), but all strings in the Thunderbird main window are treated as objects. Thus, to move from a folder name to a message subject to the message sender, the user only needs to press the keypad-3 key twice. As a user, this is a bit annoying because, to spell a single word in a message subject, for example, you have to double-press keypad-5 and hear every word in the subject spelled out. I'm guessing that this results from how AT-SPI provides the data in these trees and tables, but I raise it in case this is not the expected behavior. Hope this helps.
(In reply to comment #14) > * Moves getShowingDescendants() from flat_review.py to default.py Looks good. > * Adds a getShowingDescendants() to Gecko.script.py to do the work from the > proof of concept. Looks good. > * Gets the headers using the table interface rather than looking the first > child of the tree table. Looks good. > * Starts in medias res with the first visible row we can find (be it by > selected rows or by scrolling the height of the tree table). Then appends > everything that is visible after that row, and finally prepends everything that > is visible before that row. Looks good. > * Includes requested/discussed comments, null checks, to-dos, printing of > exceptions. Looks good. > Questions: > > * I moved the visible() method out of flat_review.py. The new default script's > getShowingDescendants() needed it. We could call it via the flat review > context, but it occurred to me that the definition of visible() might vary from > script/toolkit to script/toolkit due to wacked-out and/or bogusly-nil extents > (right???). Is this okay, or should it remain in flat_review.py? What you did is good. > * What is this GRID_SIZE (defaults to 7) of which the flat review code speaks? > ;-) The associated comment is "A minimal chunk to jump around should we not > really know where we are going". Story of my life, but anyway.... I see how it > is used in getShowingDescendants() :-). But it *only* seems to be used in > getShowingDescendants(), and it also seems to be something which might one day > prove to be script/toolkit-specific(???). So, does it still belong in > flat_review.py? For now, it's still there, and passed along as a new argument > to getShowingDescendants(). When dealing with these things that manage their descendants, the code attempts to not look at the children directly (since there can be gazillions of them), but instead breaks the rectangular region into a grid, kind of like laying graph paper on top of it. It then traverses this grid and calls getAccessibleAtPoint to see if there's an object there. GRID_SIZE is the number of pixels wide/tall of each cell in the graph paper. So, for where it belongs, I guess probably with the chunk of code that uses it. :-) That is, it would probably be OK to not pass it around, but just stuff it in the new default.py:getShowingDescendants. What do you think? > Pylinted and functionally tested. Gonna toss the regression tests at them now. If all looks groovy, I say check it in and I'll trust you to do the appropriate thing with GRID_SIZE.
I just found another problem with flat review. In Thunderbird dialog boxes, falt review does not speak the contents of combo boxes. (Sorry Joanie, I know how much you love combo boxes... ;-)
I think Joanie's patch here helps with one of the most important things, which is if someone presses a flat review key, Thunderbird currently hangs for a long time. The other flat review issues can be treated as separate bugs. BTW, David, the flat review functionality was really intended to be an 'escape' mode for ill-behaved applications. The preferred method for interacting with applications is the same method that any keyboard user would use -- just use the built-in keyboard navigation techniques of the application/toolkit and Orca should hopefully present the proper information to you. This is not to say that we won't address flat review problems. We will, but this is just a reminder that flat review really is not intended to be the primary operating mode of Orca.
Will, In fact, i rarely use flat review except in gnome-terminal. however, in opening this bug, Joanie requested that all problems found with flat review in Thunderbird be added as comments to this bug because she wanted to do a clean fix. So, I've been adding every problem that I have found to this bug instead of opening new bugs for each of them. Are you asking me to file new bug for each of my comments above?
(In reply to comment #22) > In fact, i rarely use flat review except in gnome-terminal. Phew! Thanks so much for looking into these issues and logging them, though. Definitely appreciated. I just wanted to make sure you knew there was a 'better way' to access the info in a well-behaved app. > Are you asking me to file new bug for each of my comments above? Let's just keep this one perpetually open. Thanks!
Created attachment 116692 [details] [review] revision 5 (move grid size to the method that uses it) Pylinted, regression tested, committed. I'll work on the other issues as time permits and hopefully knock a few more of them out before the 2.24 release. Dave thanks for all the testing!
Hi Dave. > - Flat review easily crosses back and forth between the headers and the message > body. I'd check with Mike and Will, but I believe that's a design decision -- i.e. flat review is supposed to go from top to bottom, left to right presenting the accessible objects and text it finds along the way. If there's an accessible separator, I'd expect Orca to announce it. If there's just a point at which you have left one place and entered another, but no physical, accessible object or text serving to delineate the boundary, I would not expect us to announce it. If I'm correct about this being a design decision, then let's not include it as part of this bug which is to address "largely" and "broken" and "Thunderbird" (as opposed to Orca-wide flat review issues). :-) > - Flat-review still does not follow focus in the message list--it always starts > in the menu bar. And I've seen that a lot in the past, but I'm not seeing it now. I've checked in a number of Gecko-related patches lately which perhaps fixed it as a happy side effect. If you are running the latest Orca trunk and are still seeing this issue, I'd ask you to try the current Thunderbird patches. Before those patches, we weren't always correctly updating the locusOfFocus and/or the caretContext; with those patches we should be. And if we know where you are, odds are greatly increased that we'll flat review to that position rather than to the top. > - Flat review does not update the page if the message is longer than one screen > in length. I'm no longer seeing this issue either. Again, there have been recent checkins, plus a few more posted, which should address this issue if it's not already addressed for you. Unless, of course, it is your NY Times Page_Up/Page_Down message. As I commented in another bug, all bets are off with the tables in your test case. That's a (mainstream) Thunderbird bug. > Thanks for the effort! And thank you for yours! Oh I see there's more. :-) > I just found another problem with flat review. In Thunderbird dialog boxes, > falt review does not speak the contents of combo boxes. Confirmed. Looks like we're grabbing the name instead. I'll look at that one next. Placing "testing required" back on this bug for now so that we can get confirmation (or denial, as the case may be) as to whether each of the above issues has been addressed through other fixes and/or patches (e.g. bug 535188 and bug 547496, if one or both of those patches is not checked in). Thanks guys for all your help with this!!
Joanie, I know you are closing on a deadline, so I'll get to this as soon as I can (hopefully on Sunday). As I said in another bug, my gnome desktop is dead (I can still use emacspeak, fortunately, so can still get work done), but I am hoping to install Intrepid on Saturday afternoon. I'll test as soon as I can get the latest Orca trunk installed and patched. Thanks for all of the effort!
Created attachment 118525 [details] [review] Fix to address the combo box issue This addresses the bulk of the Thunderbird (and Firefox) combo box issues. Verified in the Shredder Preferences dialog, Formatting page. And also in the message composition window (give focus to the body of the message and then flat review up). Note that there are still occasional issues such as with the From combo box in that same window. Without the patch we read the line as "From: From". That's bad. With the patch we read it as "From" followed by the name associated with the account, like "gmail". In this instance, we're missing some text -- the address -- and should be presenting the combo box as "joanmarie.diggs@gmail.com gmail". Given the complex nature of the beast which is combo box, that flat review was largely broken on the combo box front both in Thunderbird and in Firefox -- both for XUL and for HTML -- and that this patch seems to address nearly all of the issues, and improve those it does not fully fix, I'm leaning towards not obsessing about every last combo box. :-) Besides, as a work-around, you can always give it focus.... Will this patch has been regression tested in Firefox, OOo Writer, and Gtk-demo. I've also added a new test for HTML combo boxes. Please review. Thanks.
I think the patch looks pretty safe and I'm glad it works well. Commit if you feel good about it. Thanks!
In thunderbird flat review is working much better now. That said I am still seeing one issue: 1. Open thunderbird 2. tab to the mailbox tree 3. press the flat review key for read current line. Notice that you hear the menubar instead of the tree item with focus. 4. Down arrow to the inbox 5. tab to the message list 6. flatreview the current line again. Notice that you again hear the menubar. Also notice that once you move focus in these controls that flat review starts from the correct position.
(In reply to comment #25) > Hi Dave. > > > - Flat review easily crosses back and forth between the headers and the message > > body. > > I'd check with Mike and Will, but I believe that's a design decision -- i.e. > flat review is supposed to go from top to bottom, left to right presenting the > accessible objects and text it finds along the way. If there's an accessible > separator, I'd expect Orca to announce it. If there's just a point at which you > have left one place and entered another, but no physical, accessible object or > text serving to delineate the boundary, I would not expect us to announce it. > If I'm correct about this being a design decision, then let's not include it as > part of this bug which is to address "largely" and "broken" and "Thunderbird" > (as opposed to Orca-wide flat review issues). :-) > Joanie's understanding of the intended behavior in this case is correct.
The behavior is what I would expect. I was just hoping that the transition between the header area and the message body could be vocalized. But, if you don't have access to the needed information, nothing can be done. Thanks!
Thanks Mike! Regarding landing in the menu bar when a message is selected, I can reproduce that issue both in Thunderbird and in Evolution. This bug is about cleaning up the Thunderbird-specific issues. Therefore I've opened a new bug for the tree table issue (see bug 551891). Based on Mike's findings and what I'm seeing, I am of the opinion that Flat review in Thunderbird is no longer largely broken. :-) Phew! Will, thank you for your tolerance of my making this an uber-bug. Dave, thank you for adding all the bugs to it. I found it helpful to see the big picture. (Yeah, I know, a little ADHD goes a long way....) Let us now resume our traditional practice of one bug per report. :-) Closin' this puppy as FIXED.