The bug in question has been marked as FIXED.  I was hoping that would just magically solve the issue for us.  Apparently it has not.  I'll look at this one and see if it's an easy fix.

This one is going to require changes to basic caret navigation. Therefore re-targeting this one to 2.21.4.

Created attachment 100167 [details] [review]
take 1

1. Orca's caret navigation model should not kick in until we're at the actual end of the entry, rather than at the second-to-last character.  This will allow us to arrow to the end, BUT:

2. If findFirstCaretContext() is then allowed to return a characterOffset of -1, we'll wind up right back at the beginning of the entry because findNextCaretInOrder() sees an offset of -1 as an indication to start at the beginning of the object.  We'd thus be just as doomed as Charlie on the MTA -- arguably more so seeing as how none of the characters on the MTA were embedded objects, and none of the characters in our entries are bearing sandwiches.  But I digress.... findFirstCaretContext() should instead return the current offset, which will enable us to escape, BUT:

3. We'll erroneously repeat the last character (i.e. the one we just arrowed past) because under the circumstances sayCharacter() decrements the characterOffset before asking speakCharacterAtOffset() to do its thing. So we should prevent it from doing so,  BUT:

4. That will cause speakCharacterAtOffset() to just speak the entire component (in this case the full entry) for want of an actual character.  We don't have an actual character though, and we probably should say something, so why not say "blank"?

Happily the patch is shorter than the explanation. ;-)  Given that it's entirely limited to entries, I think it's pretty safe.  Thoughts on the approach?

I think this patch looks OK, though the first two diffs need some documentation.  The first part already has a big humongous TODO from WDW (who me?) that seems to be addressed at least partly by this patch, so maybe it should be replaced with more up-to-date comments with this patch?

For the second diff, I'm confused about the need for the change:

-            elif caretOffset >= length - 1 \
+            elif caretOffset == length \

Does this have an impact on the fix?

For the last diff, we need Mike's input. From what I can tell, it seems like it is speaking "blank" at the end of the text entry, when I think "blank" is meant to indicate you are on a blank line.

In Gedit if we arrow beyond the last space on a line we here nothing.  Perhaps we should do the same here. 

> For the second diff, I'm confused about the need for the change:
> 
> -            elif caretOffset >= length - 1 \
> +            elif caretOffset == length \
> 
> Does this have an impact on the fix?

Actually, that *is* the fix. ;-)  Or perhaps more accurately, that was the only change necessary to enable us to be able to arrow to the end of the HTML entry: We were stopping too short.  The rest of the patch is about doing the right thing in terms of being able to exit the entry and say what we should.

Given an entry with the word "foo" in it (sans quotes):

f o o
^ ^ ^    length == 3 
0 1 2

When we're at offset 2 and attempt to right arrow past it to the end of the entry, caretOffset == length - 1, thus weHandleIt becomes True.  And we don't do a very good job of handling it:  We decide we're done with the entry and move on.

My understanding of this section of code is that it's our bailing mechanism:  If we're at the end of an entry (as evidenced by caretOffset == length) and press Down or Right, we want our caret navigation mechanism to kick in so that we can proceed to the next object.  Otherwise we don't want to handleIt.  I suppose >= is safer than == however.  While I couldn't cause the offset to exceed the length, we've seen far weirder things and offsets in FF3 :-)

All of this said, this was a "wee hours" fix, and I'm still sleep deprived. :-)  So if I'm missing something, please let me know.  Otherwise, I'll change the == to a >=, add some docs, and remove the speaking of "blank."

Thanks guys!!

(In reply to comment #6)
> > For the second diff, I'm confused about the need for the change:
> > 
> > -            elif caretOffset >= length - 1 \
> > +            elif caretOffset == length \
> > 
> > Does this have an impact on the fix?
> 
> Actually, that *is* the fix. ;-)
...
> My understanding of this section of code is that it's our bailing mechanism: 
> If we're at the end of an entry (as evidenced by caretOffset == length) and
> press Down or Right, we want our caret navigation mechanism to kick in so that
> we can proceed to the next object.  Otherwise we don't want to handleIt.  I
> suppose >= is safer than == however.  While I couldn't cause the offset to
> exceed the length, we've seen far weirder things and offsets in FF3 :-)
...
>  So if I'm missing something, please let me know.  Otherwise, I'll change the
> == to a >=, add some docs, and remove the speaking of "blank."

Gotcha.  For being just a little more on the anal side, we might consider "elif characterOffset >= length" to be just a little more consistent with what the code used to me.  Getting rid of the "blank" is Mike's call, which I think he called.  :-)

Thanks for your work.

Created attachment 100334 [details] [review]
take 2 (do what Mike and Will said) :-)

Added docs/updated comments, changed the elif, took out the speaking of "blank."

Mike please test.

A problem I'm noticing is the following:
1.  reply to a comment on a bug in bugzilla.
2.  right arrow through the comment.
3.  up arrow back to the top of the comment
4.  arrow down to the bottom of the comment.
You should notice that both with down arrow and right arrow you can't move beyond the end of the comment.  On a brighter note you can now read the character at the end of an entry and we are now speaking nothing instead of blank.

Created attachment 100769 [details] [review]
adjustment for multiline entries

So *that's* what that length - 1 bit was all about.... :-) 

This version does length - int(not singleLine) instead and seems to let us bail out of both single-line and multi-line entries.

Mike please test.

(In reply to comment #10)
> Created an attachment (id=100769) [edit]
> adjustment for multiline entries
> 
> So *that's* what that length - 1 bit was all about.... :-) 
> 
> This version does length - int(not singleLine) instead and seems to let us bail
> out of both single-line and multi-line entries.
> 
> Mike please test.
> 
Works great now 

Will, okay to commit?

The mods look simple enough.  As long as it seems to work OK and it pylints to a 10.00, please commit!  :-)  Thanks for your work!

Pylinted, committed, moving to pending.

(In reply to comment #14)
> Pylinted, committed, moving to pending.
> 
seems to work great 

Thanks Mike.  Closing as FIXED.