277553 - clicking on Textarea with a RTL direction doesnt work

Status: RESOLVED FIXED

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

Description

[Oran Baruch]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Well 
the problem is with rtl textareas, I am working with hebrew,
when I click on the left side of the textarea, after the end of the line, the
text indicator should go to the end of that line, that is working 
fine on the first line,
BUT when I go down (pressing enter) and want to come back to the end of the line
of the above line.
exmpale: I want to go back from here to "on the first line," text mentioned
above, but the indicator is stuck unless I go to the last character in that
line, that is really making me insane sometimes when writing in a public forums
in Hebrew,
I hope you will find a solution

Reproducible: Always

Steps to Reproduce:
1.go to a rtl textarea (or make it rtl by right clicking and selecting switch
text direction)
2. write some words
3. press enter
4. write some more words
5. click on the left side of the words you wrote in step no 2

Actual Results:  
Now you can see that the indicator is not going up

Expected Results:  
the text indicator was supose to go up like it does when the dir="ltr"

Comment 1

[Mats Palmgren (inactive)]

Confirming bug, 2005-01-06-05 trunk Linux.

Comment 2

[Mats Palmgren (inactive)]

*** Bug 277555 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

It'd be nice to have a minimal testcase here....

Comment 4

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

Is this a problem with just textareas, or also with editor in general?

Comment 6

[Oran Baruch]

(In reply to comment #5)
> Is this a problem with just textareas, or also with editor in general?

what do you mean by editor?
like when I right click and select view source?
in that case there is no rtl option in that editor.

or forms in general?
all other forms are seems to be ok

Comment 7

[Boris Zbarsky [:bzbarsky]]

No, I mean editor.  Composer, mailcompose, contentEditable iframes, etc.

Comment 8

[Martijn Wargers (dead)]

Attachment: Testcase2

Yes, editor also suffers from the bug, as you can see with this designmode
testcase.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Over to bidi, but this may be a selection issue...

Comment 10

[Uri Bernstein]

Attachment: patch

A simpler, more efficient, and (hopefully) less buggy implementation of
nsLineIterator::FindFrameAt() for the BiDi (or suspected BiDi) case.
Notice that this implementation does not use CheckLineOrder().

In principle, the new code can be used for the no-BiDi case as well. It is only
slightly less efficient (it always scans the entire line), and this could be
probably be taken care of. However, I preferred not to touch the non-BiDi code.


This patch also fixes the bug described in bug 207186 comment 2 (regarding the
behaviour of the up/down arrows at end of line in an RTL textbox).

Comment 11

[Prognathous]

Uri, I'm almost certain that you intended to ask Simon Montagu to review your
patch, rather than Simon Fraser, am I right?

Prog.

Comment 12

[Uri Bernstein]

(In reply to comment #11)
> Uri, I'm almost certain that you intended to ask Simon Montagu to review your
> patch, rather than Simon Fraser, am I right?
> 
> Prog.

Actually, no. I saw That Simon Fraser reviewed previous patches on this code,
and that in general, this code was maintained mainly by the Editor team, not the
Layout team. However, since I wasn't sure, I also sent an e-mail to Simon Fraser
asking him if he's the right person to review this. No disrespect to Simon M, of
course.

Comment 13

[Eyal Rozenberg]

The patch's code looks kind of general-purpose... what does it have to do with
bidiness anyway?

Comment 14

[Uri Bernstein]

(In reply to comment #13)
> The patch's code looks kind of general-purpose... what does it have to do with
> bidiness anyway?

Well, for starters, the added code is wrapped in 
+#ifdef IBMBIDI
+  if (aCouldBeReordered) {
....
+  }
+#endif 
(where aCouldBeReordered is set to aPresContext->BidiEnabled() by the caller).

But if you're asking _why_ I limited my patch to the BiDi case, and not applied
it to all cases, the reason is basically that I didn't want to risk breaking
stuff that is working (the simple LTR case). I figured this will make the patch
less risky, and will give it a better chance getting in for Firefox 1.1.
If I get strong positive feedback from the reviewer (or others who know this
code), I'll be happy to change my mind.

Comment 15

[Uri Bernstein]

(In reply to comment #10)
> [The new code] is only slightly less efficient (it always scans the entire line)

Correction: it only does this if aX is not within one of the frames (which is a
rare case).

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 187293 [details] [diff] [review]
patch

I'm not really qualified to review this code. You should ask someone with more
knowledge of the frame hierarchy (dbaron, bz etc).

Comment 17

[Uri Bernstein]

Comment on attachment 187293 [details] [diff] [review]
patch

Moving reveiw request to bzbarsky, which is CCed on this bug and seems to have
less on his review plate.
Boris - please let me know if this is not OK with you.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm still sorting through everything that happened while I was away for a month.
 It's likely to be well over a week before I get a chance to even look at this.

Comment 19

[Uri Bernstein]

(In reply to comment #18)

If there's someone which is qualified to review the patch and you know to be
less busy, please let me know.
Otherwise, I'll wait till you have time.

Comment 20

[Boris Zbarsky [:bzbarsky]]

OK, so my problem is that I don't really know this code either.  dbaron might;
not sure...  That said, this patch makes the bidi and non-bidi branches return
different frames if, say, aX < line.bounds.x and mRightToLeft is true.  Is that
really intended?  That is, what does the mRightToLeft boolean actually mean here?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Also, I would much prefer a single codepath for both bidi and non-bidi, if
there's not a huge perf difference.  That would prevent issues like the one I
just raised, where the codepaths do slightly different things...

Comment 22

[Uri Bernstein]

(In reply to comment #20)
> OK, so my problem is that I don't really know this code either.  dbaron might;

Naturally, I'll be glad to hear comments from dbaron, or smontagu, or anyone
which does understand this code, on the patch in general and on the questions
you raise below specifically. I just got the impression that dbaron is a very
busy person.

> That said, this patch makes the bidi and non-bidi branches return
> different frames if, say, aX < line.bounds.x and mRightToLeft is true.  Is that
> really intended?  

It's certainly not intended, but I also can't see why it would happen, at least
in the normal case. As far as I can tell, the (logically) last non-zero-width
frame (which, in case mRightToLeft is true, is also the visually leftmost
non-zero-width frame) will be returned in both cases. Please tell me what I'm
missing.

The only case I can think of where a different frame would be returned is if the
positioning of the frames is "artificially" altered - in which case my approach
would be, IMO, better, as it would return the frame which is visually leftmost -
in accordance with the visual orientation of this method.

>That is, what does the mRightToLeft boolean actually mean here?

mRightToLeft, as far as I understand, signifies the directionality of the block
element containing the line (aka the "paragraph direction"). This is the basic
direction in which the frames are layed out. 

>Also, I would much prefer a single codepath for both bidi and non-bidi, if
>there's not a huge perf difference.  That would prevent issues like the one I
>just raised, where the codepaths do slightly different things...

As I wrote above, the new code _should_ work just fine for the non-bidi case. I
was simply being conservative, trying to make sure that if I broke anything it
would be just the bidi case. But perhaps that's not a good approach. If asked,
I'll be happy to provide a patch where the new code is used for both cases.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #22)
> As I wrote above, the new code _should_ work just fine for the non-bidi case. I
> was simply being conservative, trying to make sure that if I broke anything it
> would be just the bidi case. But perhaps that's not a good approach. If asked,
> I'll be happy to provide a patch where the new code is used for both cases.

Please do so.  We have way too many IBMBIDI ifdefs already; we don't need more
where they're not needed.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> in which case my approach would be, IMO, better

Agreed.  With your approach, the only thing mRightToLeft affects is the
aXIsBefore/After stuff, which makes sense to me...  Let's just use the new code
in both cases, ok?  A patch to that effect would be great.

Comment 25

[Uri Bernstein]

Attachment: patch v. 2

Use the new approach for both bidi and non-bidi cases. This eliminates the need
for the aCouldBeReordered parameter (which was only used here, so I removed it
from the interface and the other implementing class as well).

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190473 [details] [diff] [review]
patch v. 2

General question: if aX falls inside several different frames (eg relative
positioning), then what does this method promise to return?  Does it depend on
mRightToLeft?

>Index: layout/generic/nsLineBox.cpp
>+        if (nsnull == closestFromLeft || 

!closestFromLeft, please.  Similar in other places where we compare to nsnull.

r+sr=bzbarsky with that fixed and if the patch's behavior is correct in light
of the general question above.

Comment 27

[Uri Bernstein]

Attachment: patch v. 2.1

Changed "nsnull ==" to "!" everywhere.

>General question: if aX falls inside several different frames (eg relative
>positioning), then what does this method promise to return?  Does it depend on

>mRightToLeft?

In such a case, the first frame (logically) in which aX falls is returned. This
does not depend on mRightToLeft.

Not sure what the procedure for carrying over review flags is, so I'm just
asking for the reviews again.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190516 [details] [diff] [review]
patch v. 2.1

You can usually carry over review flags if the reviewer said you could (which I
sort of did, in this case, but I wasn't clear enough).

Comment 29

[Uri Bernstein]

Comment on attachment 190516 [details] [diff] [review]
patch v. 2.1

Requesting approval1.8b4, since this (together with bug 299527) is a pretty
major bug in bidi editing.

Comment 30

[Uri Bernstein]

I won't be around much until next Tuesday (Aug. 2) to deal with possible
regressions, so I'm not asking anybody to check this in yet. I'll do so as soon
as I'm back.

Comment 31

[Uri Bernstein]

Patch checked in by timeless at 2005-08-02 14:55.

Comment 32

[Uri Bernstein]

Note that Testcase2 is currently unusable due to bug 302885.

Comment 33

[(no longer active)]

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