Closed Bug 309286 Opened 14 years ago Closed 14 years ago
Caret moves incorrectly in some cases of bidi HTML contained within an inline element in LTR context
In a LTR paragtaph, when an inline element contains RTL frames which are not trivially-ordered, the caret moves incorrectly when using the arrow keys to move into the RTL text. This affects only composer and Caret Browsing, not textareas. I'll attach a testcase immediately.
In Composer or Caret Browsing mode: When using the right arrow to move the caret into the RTL text, the caret moves to the end (left) of the first (right) Hebrew word, instead of moving to the end of the second Hebrew word. This happens in the first two examples (contained in a <b>), but not in the last two lines, which are identical, except for the containing <b>.
This happens because nsLineIterator::CheckLineOrder() only checks the top level of frames within the line to see if they are reordered. In the testcase, the line only containes one top-level frame (the inline <b>), so CheckLineOrder() returns aIsReordered=false. This causes nsFrame::nsFrame::GetFrameFromDirection() to use a regular leaf iterator instead of a visual iterator. nsLineIterator::CheckLineOrder() should probably traverse the entire tree contained in the line (or at least the leaves of that tree) to see if anything is reordered.
Summary: Incorrect caret movement in some cases of bidi HTML contained within an inline element in LTR context → Caret moves incorrectly in some cases of bidi HTML contained within an inline element in LTR context
Recursively examine child frames when determining whether a line is "reordered". I think the ultimate solution to this problem (and many others) would be to replace all geometrically-based algorithms for determining the order of bidi frames with bidi-level-based algorithms. If that is done, I think CheckLineOrder() can be removed, and the codepaths for the "reordered" and "regular" cases can be unified. However, this, according to my discussion with Simon, would require fixing bug 299065 first. So for the time being, we'll just have to fix individual issues (such as tthis one) within the existing framework.
The fix to bug 299065 took care of the first line in the attached testcase, but not the second one.
Depends on: 299065
Comment on attachment 197754 [details] [diff] [review] patch This patch will become obsolete with the fix to bug 303884. Unfortunately, fixing that bug still doesn't fix the second line in the testcase for this bug, which is actually due to nsVisualIterator::Prev() using GetFirstChild() where it should be using "GetFirstVisualChild()".
(In reply to comment #5) > [...] this bug, > which is actually due to nsVisualIterator::Prev() using GetFirstChild() where > it should be using "GetFirstVisualChild()". s/nsVisualIterator::Prev()/nsVisualIterator::Next()/ nsVisualIterator::Prev() should be using "GetLastVisualChild()", instead of getting the last child. Note that "GetFirstVisualChild()" and "GetLastVisualChild()" don't currently exist, hence the quotation marks.
Basically what I said in comment #5, except the methods for getting first/last visuals work a bit differently, because they're implemented in nsFrameList. This is one of the things I was trying to do for bug 228290 (I lifted the nsFrameTraversal part of this patch from there) - so hopefully this gets us closer to fixing that bug as well.
Comment on attachment 214859 [details] [diff] [review] new patch There seems to be a lot of code duplication here. Could you use the same kind of semantic as nsBidiPresUtils::GetFrameToRight/LeftOf, so that GetNextVisualFor(nsnull) returns the first visual and GetPrevVisualFor(nsnull) returns the last?
(In reply to comment #8) > (From update of attachment 214859 [details] [diff] [review] ) > There seems to be a lot of code duplication here. Could you use the same kind > of semantic as nsBidiPresUtils::GetFrameToRight/LeftOf, so that > GetNextVisualFor(nsnull) returns the first visual and GetPrevVisualFor(nsnull) > returns the last? Initially I meant to say that I considered this, and explain why I decided not to do it, but since my explanation didn't seem convincing, I figured I'll just do it to demonstrate how unreadable the code would be. But, after doing this, I'm no longer sure. This version is less readable, but not as bad as I thought it would be. So I'm leaving the decision to you. Notice that I used this opportunity to sneak in some other subtle changes (renaming blockFrame to parent, and using it instead of aFrame/mLastFrame for determining block and embedding levels).
With this fix bug 287502 too ? Try selecting the first or the second hebrew word and you'll see that it selects the other word as in bug 287502.
(In reply to comment #10) > With this fix bug 287502 too ? No, this is not related (directly) to bug 287502. Word-selection (and by-word caret movement) is pretty broken for bidi. See e.g. bug 246482.
Comment on attachment 214884 [details] [diff] [review] new patch v2 Ugh, no. This patch sucks (I'm not sure about the first one). I'll find out exactly why and post a fix.
That's what happens when you use opportunities to sneak in subtle changes. I can't use the parent frame for getting base/embedding levels, because the base/embedding levels on a block frame are irrelevant.
Comment on attachment 214893 [details] [diff] [review] new patch v2.1 I don't find this unreadable :)
Attachment #214893 - Flags: review?(smontagu) → review+
Attachment #214893 - Flags: superreview?(roc)
Attachment #214893 - Flags: superreview?(roc) → superreview+
Checked in: Checking in layout/base/nsFrameTraversal.cpp; /cvsroot/mozilla/layout/base/nsFrameTraversal.cpp,v <-- nsFrameTraversal.cpp new revision: 3.51; previous revision: 3.50 done Checking in layout/generic/nsFrameList.h; /cvsroot/mozilla/layout/generic/nsFrameList.h,v <-- nsFrameList.h new revision: 3.21; previous revision: 3.20 done Checking in layout/generic/nsFrameList.cpp; /cvsroot/mozilla/layout/generic/nsFrameList.cpp,v <-- nsFrameList.cpp new revision: 3.36; previous revision: 3.35 done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
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.