GNOME Bugzilla – Bug 541094
Back out 'silent focus' code
Last modified: 2009-03-10 00:05:07 UTC
As the result of several things (see bug #350854, for example), we put code in default.py to silently set the locusOfFocus if we detected an event for a text area that had the FOCUSED state set, but which wasn't the locus of focus. Since the time we added that code, however, something has changed somewhere to prevent the need for us to do the silent focus action. In particular, it appears as though we now get the appropriate focus events for when a text area gets focus. As a result, we can remove this code. In searching for and analyzing calls to setLocusOfFocus with "False" for the third parameter, we seem to be doing the silent focus setting in two modules: default.py and Gecko/script.py.
Created attachment 113785 [details] [review] Patch to back out the silent focus action for default.py This patch backs out the silent focus changes for default.py. It passes the gtk-demo regression tests well and also pylints fine (modulo some other pylint issues from something else that slipped in by accident).
Joanie will take a look at Gecko/script.py and see about ripping the stuff out from there. She also suspects that may make one of our other bugs just 'disappear', which would be nice.
Created attachment 114224 [details] [review] back out silent focus action for default.py + some things in Gecko.script.py As discussed in team meeting today, Will's backing-out patch doesn't seem to introduce any regressions for Firefox. It also means that we should be able to remove some work-around code that was added to prevent the default script from being called and setting the locusOfFocus incorrectly. This patch has (what I believe to be) the "safe" changes in it. I still want to go through the Gecko onCaretMoved and see if some of the "possibility #x" code can come out. But that change will require more careful examination and testing... Therefore, I suggest that this patch be tested and, if found safe, checked in. Then slowly but surely we can evaluate the onCaretMoved stuff.
Given.... Everything.... I decided to focus on the onCaretMoved Gecko stuff tonight. I have pretty much gutted the existing onCaretMoved. In doing so, it would appear that I have also solved the "repetition" bug (see all my comments in the regression tests where we double-speak and double-braille things), a label guess bug for entries, and at least partially (if not completely) addressed the ARIA checkbox navigation bug. It would also seem that this helps that ARIA menu bug that only manifests itself in the regression tests. The code is also much simpler now. I don't yet know if this will make us more performant. It might. There's basically a whole bunch of checks that we're simply not doing any more because it would seem that they are unnecessary. Therefore, I'm assigning this bug to me and for the moment removing Mike's name and the request for testing. With any luck, if the regression tests I'm running at the moment pan out, I'll add Mike's name back for really thorough testing and Will's back for a similarly thorough review. :-)
Created attachment 114367 [details] [review] revision 3: includes the onCaretMoved changes for Gecko This regression tests quite nicely with the "unexpected failures" that result being the removal of bugs. :-) But who knows what conditions we're not testing for that this patch might have broken. So.... Mike: Remember before when you asked me what in particular you should test for this bug and I said "pretty much everything"? That was before I made a bunch of additional changes which impact even more. <smile> Therefore, please test the bajeesus out of this, with a focus both on Firefox and Thunderbird. Will: Given that this is a major change, I'd appreciate a code review when you have the time to really examine the changes. Thanks guys!
Sorry for the spam. Mike while testing this please also see if this has any impact on the remaining issues where structural navigation kicks in where it shouldn't when you're composing a message. Without a reliable test case, I can't test it myself. However, I have to wonder if a caret-moved event or a text-changed event in a background window is causing us to silently update the locusOfFocus. If so, hopefully this patch will take care of it. Thanks!
I just ran a couple of performance tests. Not all that surprisingly, this patch doesn't make line navigation any speedier (because when we're controlling the caret, we don't get very many caret-moved events). But it does seem to buy us a bit when tabbing (because then we do get caret-moved events). These numbers were taken when starting at the top of the main page of the Orca wiki and tabbing through to the last link: Gecko onCaretMoved: ncalls tottime percall cumtime percall 100 0.037 0.000 7.483 0.075 before 100 0.005 0.000 0.465 0.005 after Then I took a look at the number of calls to the default script's onCaretMoved: Default onCaretMoved: ncalls tottime percall cumtime percall 95 0.003 0.000 6.666 0.070 before 5 0.000 0.000 0.016 0.003 after So it would seem that the savings isn't so much my excellent coding skills in shortening/generalizing what was in Gecko. :-) The savings is that we're ignoring a lot of events that we were passing along to the default script. And it's not a huge savings. But if we can pick up a little here and a little there.... The question is are those events save to ignore. Hence the need for testing.
So far this patch seems to be behaving well. Perhaps we should get it checked in for wider community testing.
Go for it and thanks for doing the Gecko portion!
Thanks guys. And thanks for doing the default portion. :-) Patch (and a whole lot of updated regression tests with "BUG? - Foo" removed) committed to trunk. Moving to pending and crossing my fingers. :-)
I haven't noticed any problems with this one and the community hasn't commented so I think it's OK to close.