Ever since we reimplemented nsIAccessibleText via nsHyperTextAccessible, we no longer support the GetText[At|Before|After]Offset() methods. The new implementation must not move the caret or selection. We're just getting text, after all. We need to look at how nsSelection::MoveCaret works and take the useful pieces from that: http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#1309 Things like: pos.SetData() frame->PeekOffset() GetFrameForNodeOffset(pos.mResultContent, pos.mContentOffset, tHint, &theFrame, &currentOffset); theFrame->GetOffsets(frameStart, frameEnd);

We have a separate patch which uses this baked into bug 312093.

Why we need to use mPreferLeft for word start/end? Isn't mDirection enough? Will it work with BiDi?

(In reply to comment #3) > Why we need to use mPreferLeft for word start/end? > Isn't mDirection enough? No it's different. It changes whether when you are going backward or forward, how far you go. Do you go to the word boundary only? Or do you keep going past the whitespace after that, to the next word boundary? > Will it work with BiDi? It probably doesn't work with Bidi at the moment. We'll need to decide whether to use visual or logical order for that. I suggest we work on that separately.

I don't really know this code, so I don't think I'd be a reasonable reviewer for this patch. Uri or roc would be better, I think.

mPreferLeft is currently an output parameter of PeekOffset, and is used (in some cases) to indicate on which of two frames the caret should be displayed when its logical position (after the move) maps to a frame boundary. I'm not happy about overloading it as an input indicating whether we're seeking the beginning or end of the word. Also, because it's an output parameter, its value might be changed by recursive calls to PeekOffset(), which will override the initial value. So, I suggest that instead of mIgnoreWhitespacePrefs, pass an argument which can have three values, indicating "stop at end of word", "stop at start of word", or "use layout.word_select.eat_space_to_next_word to decide". Even better, perhaps - do the logic (including accessing the value of the pref, if necessary) outside PeekOffset, and then you can just pass a boolean indicating whether to stop at the start or end of a word.

Comment on attachment 227143 [details] [diff] [review] Change PeekOffset() so we can control whether we get the start or end of a word. Ignore GetAccessible() that's from another patch. So, minusing.

Uri, I think nsFrameSelection.h needs to be fixed to make clear what's an in, out or in/out param, and PeekOffset shouldn't take anything that's a pure out param.

(In reply to comment #8) > Uri, I think nsFrameSelection.h needs to be fixed to make clear what's an in, > out or in/out param, and PeekOffset shouldn't take anything that's a pure out > param. > I'm not sure what you mean by "PeekOffset shouldn't take anything that's a pure out param". How is it supposed to output multiple values then? By returning a struct? I agree that nsPeekOffsetStruct is confusing and not well documented (yeah, that's an understatement). There were plans to fix that (and much more) in bug 300131. Specifically, I tried to make sense of the input/output parameters in bug 300131 comment 15, but I now realize that I made a few mistakes there - as I didn't know the code well enough at the time. I'll see if I can get back to working on that bug. Perhaps documenting the current situation in the code would be a good start.

Sorry, I meant SetData() shouldn't take pure out params, like aEatingWS.

Uri, I decided to remove the use of the binary ^ operator because it's a bit confusing. I also used if/else so that I could more easily comment and be explicit, since we don't want to make PeekOffset harder to understand.

Comment on attachment 228308 [details] [diff] [review] Change PeekOffset so caller can explicitly ask for start/end of word, and document how it works via comments >Index: layout/generic/nsBRFrame.cpp >=================================================================== >- if (nsTextTransformer::GetWordSelectEatSpaceAfter() && >- aPos->mDirection == eDirNext) >- aPos->mEatingWS = PR_TRUE; >+ >+ if (aPos->mWordMovementType != eDefaultBehavior) { >+ // aPos->mWordMovementType possible values: >+ // eEndWord: eat the space if we're moving backwards >+ // eStartWord: eat the space if we're moving forwards >+ aPos->mEatingWS = ((aPos->mWordMovementType == eEndWord) == (aPos->mDirection == eDirPrevious)); >+ } The original code was never setting mEatingWS to FALSE (supposing it was TRUE upon entry), while the new code does. Are you sure this is OK? >+ else if (aPos->mDirection == eDirNext && nsTextTransformer::GetWordSelectEatSpaceAfter()) { >+ aPos->mEatingWS = aPos->mDirection == eDirNext && nsTextTransformer::GetWordSelectEatSpaceAfter(); >+ } You have the same logic twice here. I suspect you want to either make this a simple "else", or just set mEatingWS to PR_TRUE. >Index: layout/generic/nsTextFrame.cpp >=================================================================== >- PRBool wordSelectEatSpaceAfter = aPos->mDirection == eDirNext && tx.GetWordSelectEatSpaceAfter(); >+ PRBool wordSelectEatSpaceAfter; >+ if (aPos->mWordMovementType != eDefaultBehavior) { >+ // aPos->mWordMovementType possible values: >+ // eEndWord: eat the space if we're moving backwards >+ // eStartWord: eat the space if we're moving forwards >+ wordSelectEatSpaceAfter = ((aPos->mWordMovementType == eEndWord) == (aPos->mDirection == eDirPrevious)); >+ } Nit: The variable would probably better be named "wordSelectEatSpace" (without "After"), now that we're potentially eating up whitespace before words. Wouldn't the logic be somewhat clearer if the input argument indicated whether or not we want to eat whitespace /in the direction we're moving in/ (as indicated by mDirection)? After all, when moving forward, who cares if we're "eating whitespace when moving backwards"? I'm leaving this point to your consideration - if you find the current solution clearer, let it be. Finally - This patch and my patch for bug 343763 are now obviously conflicting. I don't mind letting this go through first, and adjusting my patch accordingly. It's up to you if you want to wait for that one to be reviewed and (hopefully) landed.

Uri, thanks for the useful comments. I didn't end up changing the in param to say whether to eat the space, because I feel that makes things difficult to understand for the caller. If I need to choose I'd rather have the callers have the clarity.

Comment on attachment 228322 [details] [diff] [review] Addresses Uri's comments Looks good. r=me.

Comment on attachment 228322 [details] [diff] [review] Addresses Uri's comments Makes sense to have Roc look at this PeekOffset change since he just sr='d a different one.

Comment on attachment 228322 [details] [diff] [review] Addresses Uri's comments Sorry for the delay