309286 - Caret moves incorrectly in some cases of bidi HTML contained within an inline element in LTR context

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

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.

Comment 1

[Uri Bernstein]

Attachment: 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>.

Comment 2

[Uri Bernstein]

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.

Comment 3

[Uri Bernstein]

Attachment: patch

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.

Comment 4

[Uri Bernstein]

The fix to bug 299065 took care of the first line in the attached testcase, but not the second one.

Comment 5

[Uri Bernstein]

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()".

Comment 6

[Uri Bernstein]

(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.

Comment 7

[Uri Bernstein]

Attachment: new patch

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 8

[Simon Montagu :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?

Comment 9

[Uri Bernstein]

Attachment: new patch v2

(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).

Comment 10

[Jean-Marc Desperrier]

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.

Comment 11

[Uri Bernstein]

(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 12

[Uri Bernstein]

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.

Comment 13

[Uri Bernstein]

Attachment: new patch v2.1

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 14

[Simon Montagu :smontagu]

Comment on attachment 214893 [details] [diff] [review]
new patch v2.1

I don't find this unreadable :)

Comment 15

[Uri Bernstein]

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