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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: uriber, Assigned: uriber)

References

Details

(Keywords: testcase)

Attachments

(2 files, 3 obsolete files)

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.
Attached file testcase
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.
Keywords: testcase
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
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #197754 - Flags: review?(smontagu)
Blocks: 207186
Status: NEW → ASSIGNED
No longer depends on: 207186
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()".
Attachment #197754 - Attachment is obsolete: true
Attachment #197754 - Flags: review?(smontagu)
(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.
Attached patch new patch (obsolete) — Splinter Review
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.
Attachment #214859 - Flags: review?(smontagu)
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?
Attached patch new patch v2 (obsolete) — Splinter Review
(In reply to comment #8)
> (From update of attachment 214859 [details] [diff] [review] [edit])
> 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).
Attachment #214884 - Flags: review?(smontagu)
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.
Attachment #214884 - Flags: review?(smontagu)
Attached patch new patch v2.1Splinter Review
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.
Attachment #214884 - Attachment is obsolete: true
Attachment #214893 - Flags: review?(smontagu)
Comment on attachment 214893 [details] [diff] [review]
new patch v2.1

I don't find this unreadable :)
Attachment #214893 - Flags: review?(smontagu) → review+
Attachment #214859 - Attachment is obsolete: true
Attachment #214859 - Flags: review?(smontagu)
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.