303884 - Bidi: Pressing the down-arrow on the last line of an RTL textarea has unexpected effect on further caret movement

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

In any RTL textarea with more than one line (e.g.
https://bugzilla.mozilla.org/attachment.cgi?id=128682):

1. Place the caret anywhere on the last line.
2. Press the down arrow. The caret will move to the end (left) of the line, as
expected.
3a. Now press the right arrow. The caret will move to the end (left) of the
previous line. (expected: move to the right of the leftmost character in the
last line)
3b. Alternatively, type a character. The character will appear on the beginning
(right) of the last line (expected: the character should appear on the left of
the line). Typing an additional character will move the first character (and the
new one) to the correct position.

Comment 1

[Uri Bernstein]

Two updates:

A. The behaviour described in 3b does not happen for all RTL textareas, but only
if the line contains LTR text (such as in the current testcase). 

B. Instead of step 2 (pressing down arrow), you can go to the end of the last
line, press enter to create a line break (the caret won't visually move due to
bug 303399), and then delete that line break by pressing Backspace *twice* (due
to bug  303781). You should then be able to reproduce the problems described in
3a or 3b.

The issue here is that after following step 2 in comment 0, or the steps
described in (B), the caret is considered to be not in the last text frame, but
rather in a BRFrame which follows that text frame (although there is no actual
line break there!). This happens for LTR textboxes as well, but is apparently
benign there. RTL textboxes, however, use visually-oriented algorithms for
determining the order of the frames, and since the BRFrame and the text frame it
is adjacant to have the same X position, the algorithms get confused and don't
find the correct frame to step into when then using the right arrow. This
explains 3a.

I don't yet have an explanation for 3b.

I'm not sure what's the right way to fix this. I have a fix for the 3a case (by
tweaking the visual-frame-order code), but I wonder if perhaps the root problem
here is that the caret ever gets to that BRFrame at the end (or perhaps that it
even exists). Fixing that (assuming it's really a problem, and not intentional),
should fix both 3a and 3b, but the fix has to be general enough to cover both
the scenario from comment #0, and the one decribed in (B), above.

Any input on this from someone who understands this code would be much appreciated.

Comment 2

[Uri Bernstein]

Attachment: patch for the "3a" case, alternative A

This is the first of two alternatives to fix the "3a" case (i.e., they deal
with what happens when right-arrow is pressed, assuming we already got somehow
into the BRFrame).

The idea here is that when considering the visual order of frames, frames with
the same "x" value are ordered according to their logical order. (i.e. if
frames A and B have the same x origin, but A comes before B logically, than A
will be treated as if it were to the left of B in LTR context, and as if it
were to the right of B in RTL context).

Comment 3

[Uri Bernstein]

Attachment: patch for the "3a" case, alternative B

Second alternative fox fixing the "3a" part of the bug:

In RTL context, look at the right edges of the frames instead of the left edges
when determining visual order.

This is simpler than A (and more "symmetrical", one can argue), but I think it
is less theroetically correct (It just happens to work in the case of a BRFrame
at the end of the line. Then again, that's the only case I know that needs
fixing).

Comment 4

[Uri Bernstein]

Attachment: patch for the down-arrow case

This patch handles the issue of getting into the BRFrame in the first place,
when pressing down-arrow on the last line. It does not cover the newline-then
delete case from comment #1 (B), and, surprisingly (for me), it also only fixes
the 3a case from comment #0, and not the 3b case (which might be a separate bug
altogether).

What I'm doing here is simply to limit the fix for bug 106855 (setting the hint
to HINTRIGHT), to the case where it really matters for the purpose of that bug
(last line is blank).

I can't really say why this is correct (or why the original patch for bug
106855 was correct, for that matter) - but it's simple, and it fixes the
current problem (or at least half of it) at its source.

Comment 5

[Uri Bernstein]

Attachment: patch for delete-blank-line case

This makes sure that we don't get into that final BR frame in the case
described in comment #1 (B). This time the fix I needed to limit the scope of
was for bug 92124.

To summarize, we have the following options:

1. Decide that having the caret in the final BRFrame is OK, so either:
1a. Go with 'patch for the "3a" case, alternative A', or
1b. Go with 'patch for the "3a" case, alternative B'
2. Decide that we never want the caret in that BR frame, and then apply *both*
'patch for the down-arrow case' and 'patch for delete-blank-line case' (or
variants thereof).

mrbkap, roc - would one of you volunteer to take a look and make a decision?
Then I can ask for an actual review depending on the path chosen.

Thanks, and sorry for the bugspam I'm generating here.

Comment 6

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

Uri, assuming this bug is not a regression, I think we might stop taking these
caret fixes for 1.8. It seems none of us really understand the code very well
and I'm afraid of causing regressions. mrbkap is landing new caret drawing code
in 1.9 that may allow us to simplify the way carets work; maybe you or mrbkap
should do a more comprehensive rewrite of how caret works then.

Comment 7

[Uri Bernstein]

Yeah, I didn't really expect this to get into 1.8. I don't think anything in
these patches interacts directly with mrbkap's caret overhaul, but nevertheless
I'll accept your advice to wait for it to land and see if the issues remain and
these patches still relevant (I suspect they will be).

Comment 8

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

I'm sure mrbkap's work won't fix these bugs. He might make it possible to fix
these bugs in a simpler way.

Comment 9

[Uri Bernstein]

(In reply to comment #5)
> To summarize, we have the following options:
> 
> 1. Decide that having the caret in the final BRFrame is OK, so either:
> 1a. Go with 'patch for the "3a" case, alternative A', or
> 1b. Go with 'patch for the "3a" case, alternative B'
> 2. Decide that we never want the caret in that BR frame, and then apply *both*
> 'patch for the down-arrow case' and 'patch for delete-blank-line case' (or
> variants thereof).

Given bug 320957 (another case where the caret ends up in a BRFrame), I now tend to go with (1) - which would fix that bug as well. We still have to choose between 1a and 1b.

Comment 10

[Uri Bernstein]

Comment on attachment 192088 [details] [diff] [review]
patch for the "3a" case, alternative A

Simon, can you please take a look at these two alternatives, and say which one (if any) you prefer?

Comment 11

[Uri Bernstein]

A better solution would be to replace geometric algorithms for determining frame order with ones based on bidi levels. This should be possible once bug 299065 is landed.

Comment 12

[Uri Bernstein]

Attachment: alternative C

Do away with geometry in nsFrameList::GetPrev[Next]VisualFor() and nsLineIterator::CheckLineOrder(). Instead use new helper methods in nsBidiPresUtils for determining visual order of the top level of frames in a line. For lower levels, rely on the fact that all the descendants of a top-level frame are now of the same direction.

I left nsFrame::GetFrameFromDirection() untouched for now, although it is crying for a rewrite.

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 214179 [details] [diff] [review]
alternative C

>Index: layout/base/nsBidiPresUtils.h
>+   * Check if a line is reordered, i.e., if the child frames are not
>+   * all layed out left-to-right.

"laid out"

>+  nsIFrame* GetFrameToRightOf(nsIFrame*        aFirstFrameOnLine,
>+                              PRInt32          aNumFramesOnLine,
>+                              const nsIFrame*  aFrame);

I think it makes for more readable code like this:

 nsIFrame* GetFrameToRightOf(const nsIFrame*  aFrame,
                             nsIFrame*        aFirstFrameOnLine,
                             PRInt32          aNumFramesOnLine);

i.e. that the (grammatical) object of the function name is first in the argument list. Ditto with GetFrameToLeftOf(), CheckLineOrder() GetPrevVisualFor() and GetNextVisualFor()

>+   * @param aFrame : We're looking for the frame to the leftt of this frame.

"left"

>+PRBool
>+nsBidiPresUtils::CheckLineOrder(nsIFrame*  aFirstFrameOnLine,
>+                                PRInt32    aNumFramesOnLine,
>+                                nsIFrame** aFirstVisual,
>+                                nsIFrame** aLastVisual)
>+{
>+  InitLogicalArrayFromLine(aFirstFrameOnLine, aNumFramesOnLine);
>+  
>+  PRBool isReordered;
>+  Reorder(isReordered);
>+  PRInt32 count = mLogicalFrames.Count();
>+  
>+  // If there's an odd-leveled frame, assume the line is reordered
>+  if (!isReordered) {
>+    for (PRInt32 i = 0; i < count; i++) {
>+      if (mLevels[i] & 1) {
>+        isReordered = PR_TRUE;
>+        break;
>+      }
>+    }
>+  }

We discussed on IRC whether no other callers needed to make this assumption. Although you say they don't, I think it's still more general and efficient to push this logic down into Reorder() and make it return two booleans, say isReordered and hasRTLFrames.


>+  if (NS_FAILED(result) || !iter) { 
>+    // If the parent is not a block frame, just get the next or prev sibling, depending on block and frame direction.
>+    nsBidiLevel frameEmbeddingLevel = nsBidiPresUtils::GetFrameEmbeddingLevel(aFrame);
>+    if (frameEmbeddingLevel & 1 == baseLevel & 1) {
>+      return GetPrevSiblingFor(aFrame);
>+    } else {
>+      return aFrame->GetNextSibling();
>+    }    

I don't understand the logic of this. If the level of this frame is the same as the base level, the previous visual frame is necessarily the previous logical frame? What about numbers following rtl text in a ltr paragraph?

Comment 14

[Uri Bernstein]

(In reply to comment #13)

> >+  if (NS_FAILED(result) || !iter) { 
> >+    // If the parent is not a block frame, just get the next or prev sibling, depending on block and frame direction.
> >+    nsBidiLevel frameEmbeddingLevel = nsBidiPresUtils::GetFrameEmbeddingLevel(aFrame);
> >+    if (frameEmbeddingLevel & 1 == baseLevel & 1) {
> >+      return GetPrevSiblingFor(aFrame);
> >+    } else {
> >+      return aFrame->GetNextSibling();
> >+    }    
> 
> I don't understand the logic of this. If the level of this frame is the same as
> the base level, the previous visual frame is necessarily the previous logical
> frame? What about numbers following rtl text in a ltr paragraph?
> 

This is the case where the parent is not a block frame, and therefore all siblings are guaranteed to have the same embedding level (because of the inline-splitting we are now doing). Since they all have the same level, they are either all laid(!) out RTL or all LTR, depending on whether the embedding level is odd or even (which may mean "backwards" or "fowards" depending on the base level).

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 214179 [details] [diff] [review]
alternative C

OK, r=me with the other nits fixed

Comment 16

[Uri Bernstein]

Attachment: alternative C, v2

Fixed all remaining issues from comment #13.

Comment 17

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

Please do not add aPresContext parameters. Call aFrame->GetPresContext() (or line->mFirstChild->GetPresContext()) instead.

I would prefer to just use nsBlockFrame::line_iterator and avoid nsILineIterator, but since the existing code uses nsILineIterator, I'll let it slide.

Other than that it looks good. I assume that GetPrev/NextVisualFor and related methods are only used for event-related activities like caret movement so we don't need to worry that GetFrameToLeft/RightOf is slower than it could be.

Comment 18

[Uri Bernstein]

Attachment: alternative C, v2.1

(In reply to comment #17)
> Please do not add aPresContext parameters. Call aFrame->GetPresContext() (or
> line->mFirstChild->GetPresContext()) instead.

I wasn't aware this was the policy. Fixed with this patch.

> I would prefer to just use nsBlockFrame::line_iterator and avoid
> nsILineIterator, but since the existing code uses nsILineIterator, I'll let it
> slide.

Thanks ;)

> Other than that it looks good. I assume that GetPrev/NextVisualFor and related
> methods are only used for event-related activities like caret movement so we
> don't need to worry that GetFrameToLeft/RightOf is slower than it could be.

Right, this is used only for caret movement, not for reflow.

Comment 19

[Uri Bernstein]

Checked in:
Checking in layout/base/nsBidiPresUtils.h;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.h,v  <--  nsBidiPresUtils.h
new revision: 1.19; previous revision: 1.18
done
Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.67; previous revision: 1.66
done
Checking in layout/generic/nsLineBox.cpp;
/cvsroot/mozilla/layout/generic/nsLineBox.cpp,v  <--  nsLineBox.cpp
new revision: 1.92; previous revision: 1.91
done
Checking in layout/generic/nsFrameList.cpp;
/cvsroot/mozilla/layout/generic/nsFrameList.cpp,v  <--  nsFrameList.cpp
new revision: 3.34; previous revision: 3.33
done
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.758; previous revision: 3.757
done

Comment 20

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).