GNOME Bugzilla – Bug 505102
Pressing Up/Down in FF3 is moving to spaces at the end of the current line
Last modified: 2008-07-22 19:33:01 UTC
It seems that a change made to Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=405248) causes lines that end with an embedded object character to now end with a space. We're landing on these spaces when we press Up/Down Arrow when we should be moving to the next/previous line. We need to stop doing that. :-)
Created attachment 101468 [details] [review] proposed patch I tested this briefly. Seems to solve the problem and it's a pretty conservative/specific change so I think it's safe. I've pylinted it as well. I'm going to write some regression tests using it and hopefully kill two birds with one stone. :-) Please test. Thanks!
Created attachment 101472 [details] [review] stop speaking the spaces as well The previous patch prevented us from *moving* to the spaces. We were still speaking them. This patch stops that as well. Again, please test. Thanks!
Created attachment 101476 [details] [review] include the spaces in between links Given a line of links separated by spaces, we were speaking the spaces. This patch catches that case as well.
I have tried the patch with my local files which produced the bug and the actual website and it seems to have solved this. I am noticing the truncation problem still at the actual website, would another orca debug be useful for this as even when I use save page in firefox and it gets the associated files I can not get the problem locally and as before I can't point you to the actual site.
Thanks Michael. I think I see where the problem with truncation lies. I need to finish writing a bunch of regression tests to be sure subsequent changes don't break anything. I'll look at the truncation tomorrow. And that patch will wind up on bug 500016 as it's a direct consequence of those "performance enhancements" (whereas this bug is an adjustment due to a FF3 change). But you're on the CC list for both, so it shouldn't be a problem. <smile>
Created attachment 101480 [details] [review] found more spaces we were arrowing to Our new spaces are not just following embedded object characters. :-( We're seeing them after labels as well....
Created attachment 101487 [details] [review] trunk-compatible patch Some of the changes I had made to the last version of the patch for this bug were needed as part of some changes I needed to make related to bug 500016. This patch now contains just the caret navigation related fix. Mike (Pedersen), if you could hammer on this with the latest Firefox trunk that would be awesome. Landing on the extra spaces is, I think, going to get on people's nerves.
I have updated Orca to latest trunk (r3408) and added this patch (attachment 101487 [details] [review]) and I am finding some issues where up cursoring does not reveal the same as you found when down cursoring. If using the HTML and css example attached to bug 500016, up cursoring onto the line where the take course links are places the cursor on the link itself, whereas down cursoring onto that line puts the cursor in some space (two braille cells on the braille display) to the left. I personally would not say this is too much of a problem (I have the braille display to show this, but does speech alone tell this), but I have noticed that on some pages cursoring up seems to miss a line, but I suspect this may be when a table cell contains more than one line (or that is what it seems to me). I am sure I could sort out some example page to show this.
> I am finding some issues where up cursoring does not reveal the > same as you found when down cursoring. Yup. I saw that the other day before I had built the latest Firefox and discovered the change reported here. And that's been commented on on the Orca list before. Sadly, it's an existing problem -- one I plan to address when I redo find{Next,Previous}Line(). The goal w.r.t. this bug is to deal with the new spaces we're finding without breaking anything else. <smile> > noticed that on some pages cursoring up seems to miss a line, Is that only the case with this patch applied? I suppose with all the new spaces that may be hard to say.... > seems to me). I am sure I could sort out some example page to show this. That would be awesome! Your example pages have already formed the basis of one of my regression tests. It's quite helpful to have them both for squashing bugs and for preventing the introduction of new ones during the refactoring.
Setting the target milestone to 2.21.5 as we need to get this issue solved sooner rather than later.
I just got off the phone with Mike. He said to go ahead and check this in. Done. Putting Will's name on it for retro-review. :-) (I would have waited until post holiday break, but I need this and the fix to bug 504785 in place so that I can write more regression tests and then tackle the next phase of the performance enhancements).
(In reply to comment #11) > Putting Will's name on it for retro-review. :-) (I would have waited until > post holiday break, but I need this and the fix to bug 504785 in place so that > I can write more regression tests and then tackle the next phase of the > performance enhancements). Thanks Joanie! You did a lot of awesome work this winter break. :-) In the patch, I was wondering what would happen if len(unicodeText) == 0. In that case, I think the if statements would issue a "IndexError: string index out of range" error. To avoid the error, you migght change them both from: + if unicodeText[-1] == " " and len(unicodeText) > 1: to: + if len(unicodeText) > 1 and unicodeText[-1] == " ":
Heh. I think I just found a place where we can make a minor performance enhancement. ;-) But in the meantime.... The if statements are each in a block that begins with: text = self.queryNonEmptyText(obj) if text and not (self.isAriaWidget(obj) \ and obj.getRole() == pyatspi.ROLE_LIST): unicodeText = self.getUnicodeText(obj) queryNonEmptyText() only returns text if the text interface is implemented *and* there's a characterCount. And then we call getUnicodeText() which turns around and called queryNonEmptyText() again, but anyhoo.... Is it possible for the characterCount to be greater than 0 but then become 0 as a result of being encoded?
> queryNonEmptyText() only returns text if the text interface is implemented > *and* there's a characterCount. And then we call getUnicodeText() which turns > around and called queryNonEmptyText() again, but anyhoo.... Is it possible for > the characterCount to be greater than 0 but then become 0 as a result of being > encoded? It might be possible, but probably slim. But, it's probably best to be on the safer side and do the look before you leap thing (i.e., check for len(unicodeText) first).
Sounds good. Suggested change committed to trunk. Moving to pending.
This one seems good now
Thanks! Closing as FIXED.