GNOME Bugzilla – Bug 535178
In Gecko, we should get the needed text for the speech and braille contexts while building up the line
Last modified: 2008-09-30 05:51:45 UTC
Right now in Gecko (i.e. Firefox and Thunderbird), we build up a line of [obj, startOffset, endOffset]'s. Having done so, we present those contents in speech by looking at each object and getting the text (again). We present those contents in braille by looking at each object and getting the text (again). We had the text (if any is to be had) while building up the line.... If we include the text as part of the the content associated with each item on the line we should: 1. Save at least some processing time and thus pick up at least a slight boost in performance. 2. Be able to avoid discrepancies in what we speak and in what we braille (see, for instance, bug 531806. 3. Have simpler code (getUtterancesFromContents and updateBraille could be simplified, getObjectContentsAtOffset could call a modified version of getObjectsFromEOCs, etc.) As part of this refactor, getUtterancesFromContents and updateBraille should rely more on their respective generators which would hopefully give us some more consistency in presentation.
Mike, I just ran the regression tests to see where I was at with what I had done so far (working on speech atm). I noticed two differences, both of which I can change, but both of which don't strike me as a bad thing necessarily: 1. The pause is gone between the heading role and the heading level, i.e. currently we say "Foo heading <pause> level 2". With my current changes we say "Foo heading level 2". 2. When arrowing to a line with focusable (i.e. form field) lists which do not have a label on the same line, we currently just indicate the number of items in the list (e.g. on bugzilla's advanced search page we say "multi-select List with 1248 items"). With my current changes we now include the selected item -- or the first item when nothing is selected (e.g. "abiscan multi-select List with 1248 items"). If the list had a label on the line, the before-and-after behavior would be the same. Like I said, I can certainly cause the refactored version of getUtterancesFromContents to do the old behavior, but I kinda like the new behavior, especially for lists: You don't have to give focus to the list to see what the list is all about. So, Mike, please let me know what you'd like to see done and I'll do it. Thanks!
I think both of these changes seem to be quite reasonable. I don't think you need to change the behavior.
We have several braille issues which I've been holding off addressing because updateBraille() may be gutted as part of the work for this bug (we'll be relying more upon the braille generators instead). Having just added another issue to my mental list, I fear I'm going to forget them. They are: 1. Navigating in ARIA checkboxes (and presumably radio buttons and the like). It is possible to arrow amongst the displayed text, but since you cannot do that in "default widgets" of the same ilk, we are not indicating the caret position in the braille. 2. Table cell headings are not part of the braille context because updateBraille() doesn't get the full context like we do in the default script's updateBraille(). This may have to happen after this bug is fixed, but I'm not sure yet. 3. Linked text is underlined (or will be soon), but linked images are not.
Created attachment 115976 [details] [review] part 1 (speech), revision 1 This is going to wind up being a large refactor in terms of the amount of code change. Rather than YAUP (UP == uber patch), I've broken it down into discreet pieces (speech, braille, additional performance). This is piece 1, speech: * Add the string to the various "contents" tuples * Use the string (rather than querying the text interface to get it again) in getUtterancesFromContents() * Use the speechgenerators Mike please test. Once we know this one is sound, I'd like to check it in rather than wait until all three pieces are done. By the way, when testing you will notice that the two changes we agreed upon in the earlier comments (the heading and level being treated as one utterance, and multiselect list items including the selected item as part of the speech context) have been implemented. Now's your chance to be sure that what sounded good in theory is actually good in practice. <smile> I think it is, but want to be sure you do as well. Two excellent places to get a feel for it are the main page of the Orca wiki and the bugzilla advanced search page. Will, we currently have the following: # Pylint is confused and flags these errors: # # E1101:6957:Script.getUtterancesFromContents: Instance of # 'SpeechGenerator' has no 'getSpeechForObjectRole' member # E1101:6962:Script.getUtterancesFromContents: Instance of # 'SpeechGenerator' has no 'getSpeechForObjectRole' member # # So for now, we just disable these errors in this method. # # pylint: disable-msg=E1101 In addition, with the change to relying upon speech generators in getUtterancesFromContents(), there are now cases where the default generators ultimately kick in. Some of those default generators call the private _getSpeechForObjectRole() -- which the Gecko speech generator lacks. Therefore, in this patch, I made the Gecko method private and added a public method to speechgenerator.py which merely turns around and calls the private method. It works and it makes pylint happy. Is this alright, or should I make a global/Orca-wide change so that getSpeechForObjectRole() is a public method? Thanks guys!
Created attachment 116018 [details] [review] part 1 (speech), revision 2 Will and I chatted briefly about this today. He suggested that I make _getSpeechForObjectRole() a public method -- unless I could cause it to not be called from outside the speechgenerators, which I had suggested that I might could pull off. And I actually did manage to pull it off.... But: * the change to 100% reliance upon speechgenerators necessitated a hack: all of the speechgenerators potentially need to be able to accept an alternative offset as an argument (primarily to pass along to a modified form of getTextLineAtCaret(); secondarily to decide if we really are at the last bit of a heading or child within a heading). -- the kicker -- * with that hack and with the most amazingly simple and clean getUtterancesFromContents() one could possibly imagine as a result, testing with the python profiler indicated that performance actually degraded a bit rather than improved(?!?!). So.... I would love to get to the bottom of why reliance upon speechgenerators made things worse performance-wize. And I plan to do so, as I'm genuinely surprised/curious. But in the meantime, my vote is to just make _getSpeechForObjectRole() a public method now and move on to the braille part of this bug. :-) Mike please test.
So far in my testing the output with this patch seems good.
(In reply to comment #5) > surprised/curious. But in the meantime, my vote is to just make > _getSpeechForObjectRole() a public method now and move on to the braille part > of this bug. :-) Sounds fair. Please commit. Thanks!
Oops. Some functional testing revealed a regression with sayAll by sentence with certain tables. :-( :-( We don't currently have any sayAll regression tests for Firefox. I'll write some now; then try to work out what I broke. Then maybe, just maybe, I'll get a part 1 that's right. *sigh*
Created attachment 116370 [details] [review] revision 3 This version corrects the sayAll issues I found -- with new sayAll regression tests to prove it. :-) Please test.
I've been abusing this one all morning and it seems to be working well.
Yea! Thanks Mike!! I just committed that patch to trunk. On to the braille stuff. (Not moving to pending because of the braille stuff).
Created attachment 119394 [details] [review] use the line contents in getTextLineAtCaret() The current getTextLineAtCaret() used in the Gecko script assumes a couple of things: 1. We'll get the correct return value out of text.getTextAtOffset() 2. We'll get the correct caretOffset for a text item which doesn't contain the caret at the moment and could use that offset reliably to get the current line. Problems with what we get out of FF when we attempt getTextAtOffset() constitute a healthy chunk of why our line navigation code is as complicated as it is. As for assumption #2, that will fail for any link that spans multiple lines. Given the above and the fact that we already have the line contents, we might as well use what we have. That's what this patch does. NOTE: At the moment, it causes getTextLineAtCaret() to not return a full line's worth in some cases (i.e. when the object on the current line is split by links and/or images which are children of that object). Ultimately I plan to remedy that by building up the string from the parts that make up the line. When I initially tried to do that, the regression tests went all to h*ll due to whitespace issues present in updateBraille() -- next on my hit list. ;-) In the meantime.... **This fixes some braille bugs we've had -- including the Omaha Steaks braille issue -- and passes all of the tests, and pylints.** Will, thoughts?
(In reply to comment #12) > NOTE: At the moment, it causes getTextLineAtCaret() to not return a full line's > worth in some cases ... > Will, thoughts? It looks pretty sound to me and I say check it in if you feel good about it. One thing I'd like to think about is if we should consider adding a getLineContentsAtOffset to default.py and have braille work with that. For most toolkits, it would return a list with a single (obj, startOffset, endOffset, string) tuple. In the case of Gecko, it could do the more complicated support that Gecko has to do to build up a line from several objects.
Created attachment 119552 [details] [review] slight adjustment to previous version Thanks Will. I resumed working on the underlining links and linked images bug and in doing so, wound up needing to adjust the Gecko script's getTextLineAtCaret() a bit further. This version also pylints and passes the regression tests. Committed to trunk.
There's still cleanup to be done to updateBraille(), but I'll do that as part of the underlining links bug. Moving this one to pending.