This is a follow-up to bug 330815. Sometimes (depending on whether there is or isn't whitespace betwen the paragraphs), when moving the caret into a pargraph starting [ending] with text of reverse direction (relative to that pargraph), the caret moves to the wrong side of the reverse-direction text (that is, not to the visual start/end of the paragraph). In bug 330815, comment #6, I wrote about this: > As far as I can tell, there are two hint-related problems: > > 1. The hint is flipped for each line-jump, so in the case of unrendered > whitespace between two paragraphs, a single step involves two line-jumps, which > results in two hint-flips, which is wrong. The solution to this is fairly > simple: the hint should be flipped (in GetFrameForDirection) if the amount is > eSelectCharacter, but not if it is eSelectNoAmount (indicating that we already > jumped a line). > This problem is not bidi-related, but in practice I couldn't come up with a > non-bidi case which shows a visible bug due to this. > > 2. For bidi, it seems that the correct rule is to flip the hint if the final > destination frame is a reverse-direction frame (relative to its own paragraph). > The current code flips the hint for a reverse-direction frame when the amount > is eSelectCharacter, (which is not necessarily the last step), and also flips > it back during a line jump if we moved to a frame with an opposite direction. I'll attach a testcase soon.
Created attachment 215990 [details] testcase Use in caret browsing mode or composer. Notice that in some cases whitespace between the paragraphs is what's causing the problem, and in other cases it's the lack of such whitespace.
Created attachment 216232 [details] [diff] [review] patch Enough with the hint-flipping madness! It took me a while, but finally I realized that if we can count on PeekOffset to return the correct frame in mResultFrame, we can use that to easily infer the correct hint. It turned out that usually this was true, except for one case where PeekOffset didn't return immediately after calling itself recursively, but continued to override the correct frame. I couldn't think of any reason why that was correct, so I changed it. The hint is now determined solely by nsSelection::MoveCaret(). I'd really like to altogether remove mPreferLeft from nsPeekOffsetStruct, but there seem to be other unrelated methods using it, so I'm leaving it there for now.
The patch also fixes bug 328656, as expected.
Status: NEW → ASSIGNED
Comment on attachment 216232 [details] [diff] [review] patch Sweet! As you will have seen in the comments you removed, I wanted to do exactly this in the original Bidi check-in, but I didn't have the guts.
Attachment #216232 - Flags: review?(smontagu) → review+
Created attachment 216306 [details] [diff] [review] patch v.1.1 After some more testing, I found out that in the eSelectLine (up/down) case, mResultFrame might be an inline containing the target frame, not the actual text frame. So I need to use nsBidiPresUtils::GetFrameEmbeddingLevel instead of just NS_GET_EMBEDDING_LEVEL. Also, removed preferLeft completely from nsFrame::GetFrameFromDirection().
(In reply to comment #5) > Also, removed preferLeft completely from nsFrame::GetFrameFromDirection(). Or at least I meant to. I'll do this before checkin, if this patch gets r/sr.
Comment on attachment 216306 [details] [diff] [review] patch v.1.1 No, sorry. This was not a perfect solution to the up/down-into-inline problem, because the hint calculation is now wrong. I guess I'll have to revert to the previous way of doing things for everything but left/right, where I know I can rely on mResultFrame.
Created attachment 216311 [details] [diff] [review] patch v2 Like I said above: - For left/right, use frame and offset to determine hint - For up/down and home/end, use offset and hint to determine frame (as before).
Attachment #216311 - Flags: superreview?(roc) → superreview+
Attachment #216311 - Flags: review?(smontagu) → review+
Checked in: Checking in layout/generic/nsSelection.cpp; /cvsroot/mozilla/layout/generic/nsSelection.cpp,v <-- nsSelection.cpp new revision: 3.222; previous revision: 3.221 done Checking in layout/generic/nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.560; previous revision: 1.559 done Checking in layout/generic/nsFrame.cpp; /cvsroot/mozilla/layout/generic/nsFrame.cpp,v <-- nsFrame.cpp new revision: 3.635; previous revision: 3.634 done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.