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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

defect
--
minor
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

({testcase})

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

14 years ago
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.
Assignee

Comment 1

14 years ago
Posted file testcase (obsolete) —
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.
Assignee

Comment 2

14 years ago
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. 
Assignee: nobody → mozilla
Component: Layout: Block and Inline → Layout: BiDi Hebrew & Arabic
Keywords: testcase
QA Contact: layout.block-and-inline → zach
Assignee

Comment 3

14 years ago
Ugh - you need to place the caret to the RIGHT of "hello", then press the
right-arrow key.
Attachment #196889 - Attachment is obsolete: true
Assignee

Comment 4

14 years ago
Posted file 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).
Assignee

Comment 5

13 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Assignee: mozilla → uriber
Status: NEW → ASSIGNED
Attachment #216240 - Flags: review?(smontagu)
Assignee

Comment 6

13 years ago
Posted patch patch, unbitrotten (obsolete) — Splinter Review
Adjusted for the changes made in bug 330175.
Attachment #216240 - Attachment is obsolete: true
Attachment #218422 - Flags: review?(smontagu)
Attachment #216240 - Flags: review?(smontagu)
Assignee

Updated

13 years ago
Blocks: 344164
Assignee

Comment 7

13 years ago
Simon, could you please review this?
Comment on attachment 218422 [details] [diff] [review]
patch, unbitrotten

Looks good. r=me
Attachment #218422 - Flags: review?(smontagu) → review+
Assignee

Updated

13 years ago
Attachment #218422 - Flags: superreview?(roc)
+      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.
Assignee

Comment 10

13 years ago
Thanks for the idea!
Attachment #218422 - Attachment is obsolete: true
Attachment #228921 - Flags: superreview?(roc)
Attachment #218422 - Flags: superreview?(roc)
Attachment #228921 - Flags: superreview?(roc) → superreview+
Assignee

Comment 11

13 years ago
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Assignee

Updated

13 years ago
Depends on: 345616

Updated

11 years ago
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.