309432 - Caret moves incorrectly when moving out of a block frame into complex bidi text

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Uri Bernstein]

Moving the caret (using right-arrow) from the end of a LTR block frame (such as
a div) containing LTR text, into the beginning of non-trivially-ordered bidi
text, results in the caret being positioned incorrectly.
This is similar to, but not the same as, bug 309286.

This affects only composer and Caret Browsing, not textareas.

I'll attach a testcase immediately.

Comment 1

[Uri Bernstein]

Attachment: testcase

In Composer or Caret Browsing mode:
Place the caret to the left of the word "hello" (indise the red-borderd DIV),
and press right-arrow.
The caret moves to the right of the second (left, bolded) Hebrew word. It
should move to the left of that word.

Comment 2

[Uri Bernstein]

This happens because the line iterator used for nsLineIterator::CheckLineOrder()
is the line iterator of the block frame where the caret is currently positioned.
That line iterator cannot see the following line (which the caret is stepping
into, and is bidi). Therefore, it returns aIsReordered=false. This causes
nsFrame::nsFrame::GetFrameFromDirection() to use a regular leaf iterator instead
of a visual iterator.

I'm not sure how this could/should be fixed. 

Comment 3

[Uri Bernstein]

Attachment: testcase (corrected)

Ugh - you need to place the caret to the RIGHT of "hello", then press the
right-arrow key.

Comment 4

[Uri Bernstein]

Attachment: testcase #2

This testcase demonstrates that the original testcase won't work even after the
problem described in comment #2 is fixed.
In this testcase, the line inside the DIV is also bidi and not
trivially-ordered, so CheckLineOrder() does set aLineIsReordered=true.
However, this testcase fails due to a second, unrelated, problem:
nsFrameList::GetNextVisualFor() checkes if the current frame is a block frame,
and if it is, immediately returns the next logical frame (without invoking the
geometric algorithm).

Comment 5

[Uri Bernstein]

Attachment: patch

The change in nsFrameList.cpp takes care of the issue described in comment #4. I simply removed the special-case handling of block frames. I'm not sure why it was there in the first place.

The rest of the patch takes care of the issue described in comment #2. If bidi is enabled, I now always use the visual iterator, regardless of whether the line was actually reordered or not. This means that CheckLineOrder doesn't need to look at the previous/next lines.
Also, while I was there, I took care of the "// XXX Should be GetFirst[Last]VisualLeaf" issue. The correct leaf is now fetched (in the bidi case) by taking into account the line direction and the embedding level of the first/last frame.
Finally, I now look at the first frame only when moving backwards, and at the last frame only when moving forwards.

Comment 6

[Uri Bernstein]

Attachment: patch, unbitrotten

Adjusted for the changes made in bug 330175.

Comment 7

[Uri Bernstein]

Simon, could you please review this?

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 218422 [details] [diff] [review]
patch, unbitrotten

Looks good. r=me

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      result = it->CheckLineOrder(thisLine, &isReordered, &firstFrame, &lastFrame);
+      if (aPos->mDirection == eDirPrevious) {
+        nsBidiLevel embeddingLevel = nsBidiPresUtils::GetFrameEmbeddingLevel(firstFrame);
+        if ((embeddingLevel & 1) && lineIsRTL || !(embeddingLevel & 1) && !lineIsRTL) {
+          GetFirstLeaf(aPresContext, &firstFrame);
+        } else {
+          GetLastLeaf(aPresContext, &firstFrame);
+        }
+        atLineEdge = firstFrame == traversedFrame;
+      } else { // eDirNext
+        nsBidiLevel embeddingLevel = nsBidiPresUtils::GetFrameEmbeddingLevel(lastFrame);
+        if ((embeddingLevel & 1) && lineIsRTL || !(embeddingLevel & 1) && !lineIsRTL) {
+          GetLastLeaf(aPresContext, &lastFrame);
+        } else {
+          GetFirstLeaf(aPresContext, &lastFrame);
+        }
+        atLineEdge = lastFrame == traversedFrame;
+      }

Couldn't this be simplified to reduce the code duplication? Set "nsIFrame** framePtr = aPos->mDirection == eDirPrevious ? &firstFrame : &lastFrame". The inner "if ((embeddingLevel & 1) ..." can have a "== (aPos->mDirection == eDirPrevious))" tacked onto it.

Comment 10

[Uri Bernstein]

Attachment: patch w/ roc's suggestion

Thanks for the idea!

Comment 11

[Uri Bernstein]

Checked in:
Checking in layout/generic/nsFrameList.cpp;
/cvsroot/mozilla/layout/generic/nsFrameList.cpp,v  <--  nsFrameList.cpp
new revision: 3.42; previous revision: 3.41
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.658; previous revision: 3.657
done
Checking in layout/generic/nsLineBox.cpp;
/cvsroot/mozilla/layout/generic/nsLineBox.cpp,v  <--  nsLineBox.cpp
new revision: 1.108; previous revision: 1.107
done