[meta] Moving the caret in BiDi texts is buggy, especially in right-to-left forms

RESOLVED INCOMPLETE

Status

defect
--
major
RESOLVED INCOMPLETE
16 years ago
3 years ago

People

(Reporter: bugzillamozilla, Assigned: uriber)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {meta})

Trunk
mozilla1.9alpha1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: See comment #20 for an easier to follow version of this page, )

Attachments

(6 attachments, 5 obsolete attachments)

Reporter

Description

16 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030526
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030526

Using the arrows keys to move the caret in BiDi texts results in unexpected
behavior. Doing the same with the mouse usually works fine.

The linked testcase includes two textareas (rtl and ltr) and demonstrates
several problems related to this bug. Comparing caret behavior in rtl text input
widgets and ltr ones shows that the problem is mostly related to rtl forms.

This bug makes it very inconvenient to type and edit BiDi texts, as editing
often requires reaching for the mouse. Using the arrow keys (as well as the
Home/End keys) is simply too unexpted and inconsistent.

Reproducible: Always

Steps to Reproduce:
Problem #1:

1. Open the linked testcase in Mozilla
2. Place the caret to the right of the Hebrew letter Alef (first line, right corner)
3. Press the left arrow-key 3 times (or more)

Actual Results:  
The caret moves to the right of the number 1 (first line), but then remains
stuck in this position regardless of any additional key presses.

Expected Results:  
With each press, the caret should move on to the next character, pass the number
and then go on to the next line/s.

This bug effects text areas as well as single-line text fields.

Tested with Firebird 0.6 and many different Mozilla builds, including the latest
nightly (1.4b/20030526)

Prog.
Reporter

Comment 1

16 years ago
The same problem is also demonstrated in line #3, with the letter 'a' instead of
the number '1'.

At first, it doesn't seem to happen with the next example (line #5) which
replaces '1' with '12', however... using Ctrl+LeftArrow to quickly move the
caret to next words results in the same problem -> the caret moves to the right
of the number, but then remains stuck.

The weirdest behavior is demonstrated in the last example (line #7): using
Ctrl+LeftArrow results in a recursive movement. Each key-press moves the caret,
but in a sort of an endless loop, as if the text never ends.
Reporter

Comment 2

16 years ago
Another testcase (http://oren.gomen.org/mozilla/textareas2.html) demonstrates
two related caret bugs which also happen in non-BiDi/Hebrew-only texts.

1. In the first textarea, place the caret to the beginning of the first line,
then press End.
2. Press the down-arrow key twice.

Actual Results:  
The caret moves to the end of the text (line #9)

Expected Results:
The caret should move to line #3

The opposite happens if you place the caret after the last letter and press the
up-arrow twice -> the caret will move all the way up, instead of two lines up.


Last bug for today:

1. In the second textarea, place the caret in the first line
2. Press the End and Home keys alternatively.

Actual Results:  
The End key moves the caret to the *first* line and Home key moves it to the *last*.

Expected Results:
End should move the caret to the end of the text, and Home should move it to the
beginning.

If needed, I'll be happy to open up a separate bug for these two issues, but
they do seem related to this one.

Prog.
See also bug 149811. In fact it might be better to dupe this bug and attach the
testcases there.
Reporter

Comment 4

16 years ago
What would you prefer, Simon, moving the above description, comments and the
linked testcases to Bug 149811, or making it depend on this one? in fact,
shouldn't all of these caret issues be separated into several bugs?

Prog.
The relevant buggy code is probably all in much the same place
(nsTextFrame::PeekOffset) so I think it makes more sense to have one bug which
describes all the cases, but it's not really crucial.
Reporter

Comment 6

16 years ago
Here's another related bug: Pressing Enter at the end of an overflowing rtl text
(which also includes an ltr word such as an English word or a number), doesn't
result in a carriage return. Pressing Enter once more results in *two* carriage
returns.

Steps to Reproduce:
1. Open http://oren.gomen.org/mozilla/textareas3.html
2. Locate the caret at the end of the Hebrew text.
3. Press Enter

Actual Results:  
Nothing happens.

Expected Results:  
The caret should go to the next line (a carriage return)

There are currently so many bugs with caret handling in BiDi texts, that all I
can suggest as a workaround (in Windows), is to edit the text in notepad.exe and
then to paste it back into Mozilla. This is actually what I do if I have to type
more than a single Hebrew paragraph.

Simon, regarding bug 149811, I really don't know. It only covers a specific
problem, and it's almost impossible to find by anyone searching for caret issues
(I should know, I thoroughly searched Bugzilla before I opened this one). So we
should either change it's summary and all the other related fields (which I
can't do, as I don't have edit_bugs privileges), or confirm this one as it does
describe many other issues that are not part of bug 149811. Your call.

Prog.
Reporter

Comment 7

16 years ago
And yet another caret bug:

1. Open http://oren.gomen.org/mozilla/textareas4.html
2. Place the caret at the end of the first line
3. Press Enter
4. Switch the input language between English and Hebrew (Alt+Shift in Windows)

Actual Results:
In the Hebrew (rtl) textarea, the caret moves back to the end of the first line
when switching to Hebrew input.
In the English (ltr) textarea, the caret moves back to the end of the first line
when switching to English input.

Expected Results:
The caret should stay in the same position, regardless of input language.

Prog.
Reporter

Comment 8

16 years ago
Correction regarding the actual results of the last testcase:

In the Hebrew (rtl) textarea, the caret moves back to the end of the first line
when switching to *English* input.
In the English (ltr) textarea, the caret moves back to the end of the first line
when switching to *Hebrew* input.

Prog.

Updated

16 years ago
Blocks: 115711

Updated

16 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 9

16 years ago
One more,

1. Type an English word at the beginning of an RTL textarea, or a Hebrew word at
the beginning of an LTR textarea.
2. Press Delete.

Actual Results:
The caret moves to the other side of the line.

Expected Results:
The caret should stay in the same position (since there's nothing to delete)

If the user now presses Backspace, the caret returns to its original location,
instead of deleting the last typed character. It's as if the Delete key
introduces an invisible character with an opposite directionality compared the
last typed word.

Prog.
Reporter

Comment 10

16 years ago
Posted file Testcase #1
Uploading all testcases...
Reporter

Comment 11

16 years ago
Posted file Testcase #2
Reporter

Comment 12

16 years ago
Posted file Testcase #3 (obsolete) —
Reporter

Comment 13

16 years ago
Posted file Testcase #4

Comment 14

16 years ago
*** Bug 211205 has been marked as a duplicate of this bug. ***
Reporter

Comment 15

16 years ago
Posted file Testcase #5
A few more bugs,

Open testcase #5 in Mozilla, then try the following:

1. Put the caret at the beginning of the first line.
2. Press Ctrl+LeftArrow twice.

Actual Result:
The caret skips the third word and is placed at the beginning of the fourth
word.

Expected result:
The caret should move to the beginning of the third word.

The same problem happens with the LTR textarea, use Ctrl+RightArrow instead of
Ctrl+LeftArrow to test.

---

1. Put the caret at the beginning of the second line.
2. Press Ctrl+LeftArrow twice.

Actual Result:
The caret is placed is placed at the end of the third word. You can verify this
by typing a character.

Expected result:
The caret should move to the beginning of the third word.

This too happens with the LTR textarea.

---

1. Put the caret at the beginning of the last (leftmost, fourth) word in the
first line.
2. Press Ctrl+LeftArrow twice.

Actual Result:
The caret skips the next (fifth) word and is placed at the beginning of the
sixth word. In this case, it is impossible to use Ctrl+LeftArrow to move the
caret to the beginning of the second line.

Expected result:
The caret should move to the beginning of the fifth word (beginning of the
second line)

This bug doesn't occur in the LTR textarea.

Prog.
Reporter

Comment 16

16 years ago
Posted file Testcase #6
In this one, the caret disappears at the end of lines that end with spaces
(when such lines are part of an overflowing paragraph). It doesn't happen with
the LTR paragraph.

1. Put the caret anywhere on the first line (RTL textarea).
2. Press End or press the LeftArrow repeatedly until the caret is placed at the
end of the line.

Actual Result:
The caret disappears.

Expected result:
The caret should be displayed at the end of the first line. See screenshot of
IE6 handling the same situation:
http://oren.gomen.org/mozilla/textareas6IE6.png

Prog.
Reporter

Comment 17

16 years ago
Additional bug can be seen in Testcase #1 above (see Foldenyi Tamas' description
of the same issue in Bug 205679):

1. Put the caret in the second textarea (LTR), at the beginning of the third line.
2. Press the left-arrow four times.

Actual Results:  
The caret moves to the end of the line.

Expected Results:
The caret should move to next line.

This is very similar to what I described in comment 1 (RTL textarea), but in
this case the caret is only stuck for a single keypress - after the third one.
It continues to move with the fourth keypress, whereas with the RTL textarea, it
is stuck regardless of additional keypresses.

Prog.
Reporter

Comment 18

16 years ago
*** Bug 205679 has been marked as a duplicate of this bug. ***
Reporter

Updated

16 years ago
Blocks: 228290
Reporter

Comment 19

16 years ago
Posted file Testcase #7
Bug 228290 describes a selection bug. Here's a reduced testcase that show that
it is actually part of the buggy caret movement in BiDi texts:

-Steps to Reproduce-
1. Open the attached Testcase #7.
2. In the second textarea (LTR), put the caret anywhere on the Hebrew word
("עברית").‎
3. Hold the Left arrow button pressed.

-Actual Results-
The caret will recursively move to left. Every time it reaches the leftmost
part of the line, it will come back between the first two characters of the
Hebrew word.

-Expected Results-
The caret should stop at the beginning of the line.

Fixing this bug should also fix Bug 228290.

Prog.
Reporter

Comment 20

16 years ago
A version of this bug, with embedded testcases, is available at
http://oren.gomen.org/mozilla/bug_207186.html

This should be much easier to follow.

Prog.
Whiteboard: See comment #20 for an easier to follow version of this page
Reporter

Updated

15 years ago
Blocks: 240501

Comment 21

15 years ago
Still there in Mozilla 1.7.2. From my experience, this bug is a major issue for
bidi users -- perhaps the most annoying bidi bug after those (mostly) addressed
by the bidiui and hebmailpack extensions.
Reporter

Updated

15 years ago
Blocks: BidiCaret
Assignee

Comment 23

15 years ago
*** Bug 265606 has been marked as a duplicate of this bug. ***
Reporter

Comment 24

15 years ago
*** Bug 274896 has been marked as a duplicate of this bug. ***
Blocks: Persian
Assignee

Updated

14 years ago
Depends on: 288789
Assignee

Comment 25

14 years ago
Posted patch patch for comment 0 (obsolete) — Splinter Review
This fixes the problem described in comment 0 (and the first line of comment
#1).
The idea is that in case of a recursive call into PeekOffset(), let the inner
call (the one dealing with the frame we're stepping into) decide on the value
of aPos->mPreferLeft. The lines added undo the possible reversing of
mPreferLeft in the "outer" call.

I don't pretend to fully understand the code. This is basically a hack, which
fixes the most annoying problem described here (without regressing anything
else, as far as I could see). I'm only suggesting it because, as far as I could
see, no one else is working on this.
Assignee

Updated

14 years ago
Attachment #184203 - Flags: review?(smontagu)

Comment 26

14 years ago
(In reply to comment #25)

Even if you don't fully understand the code, try to include in the patch a
comment explaining what it is that the code there is doing, what it is that your
code is doing, and why this is necessary (or explain it here and add a comment
such as "fix for bug 207186 - see bugzilla for details"). I think one of the
serious obstacles to fixing BiDi bugs is the lack of documentation, or schematic
description, of what the BiDi-related code does. In fact, there isn't even a
comment explaining what ::PeekOffset does, or when is the SelectAmount
eSelectCharacter and when is it eSelectNoAmount, etc. etc.
Comment on attachment 184203 [details] [diff] [review]
patch for comment 0

>+          if (eSelectCharacter == aPos->mAmount)

This seems unnecessary, since we are inside
   switch (aPos->mAmount){
     case eSelectCharacter:

I should think you want to patch the eSelectWord case as well.

Especially in code like this, patches are much easier to read if you use more
context, e.g. cvs diff -u9p

I think the whole thing would be clearer if instead of XORing the mPreferLeft
flag back and forth, you saved the original value on entry to the method, and
then restored the saved value before the recursive call.

Apart from those nits, I think your approach is correct.
Attachment #184203 - Flags: review?(smontagu) → review-
Assignee

Comment 28

14 years ago
(In reply to comment #27)
> (From update of attachment 184203 [details] [diff] [review] [edit])
> >+          if (eSelectCharacter == aPos->mAmount)
> 
> This seems unnecessary, since we are inside
>    switch (aPos->mAmount){
>      case eSelectCharacter:

That's what I initially thought too. However, I later discovered that in the
case of moving to the next wrapped line of RTL text (when there is no hard line
break), aPosAmount at this point is actually eSelectNoAmount (changed, I think,
by GetFrameFromDirection). In this situation, flipping mPreferLeft back is
actually harmful, as it prevents you from moving forward into the next line.

> I should think you want to patch the eSelectWord case as well. 

I thought that too. Turns out that while doing that did fix the problem
described in comment #1 (line #5 of the testcase), it created a much more severe
problem: you were unable to skip over an RTL word in LTR text, or vice versa
(using ctrl+arrows). The execution path in the case of eSelectWord seems to be
much more complicated (with the possiblility of multiple levels of recursion),
so I decided to give up on that, for now.

> Especially in code like this, patches are much easier to read if you use more
> context, e.g. cvs diff -u9p

Thanks for the tip. I'll use it in the future when appropreate.

> I think the whole thing would be clearer if instead of XORing the mPreferLeft
> flag back and forth, you saved the original value on entry to the method, and
> then restored the saved value before the recursive call. 

This makes sense. I'll do it if I submit a v2 of this patch (but see below).

> Apart from those nits, I think your approach is correct.

I realize that given my notes above, my approach might not seem as correct as it
 did initially. I did warn that this was a hack, but I should probably have
provided the information above in advance.
If, in spite of this, there is still a chance for the patch to get an r+, I'll
re-submit it with the change proposed above (and with extra comments).
Otherwise, I'd rather not pollute the bug with another patch - at least until I
gain a better understanding of the code (which might take a considerable amount
of time). Let me know what you think.

Comment 29

14 years ago
(In reply to comment #28)
> That's what I initially thought too. However, I later discovered that in the
> case of moving to the next wrapped line of RTL text (when there is no hard line
> break), aPosAmount at this point is actually eSelectNoAmount (changed, I think,
> by GetFrameFromDirection). In this situation, flipping mPreferLeft back is
> actually harmful, as it prevents you from moving forward into the next line.
Agree.

Comment 30

14 years ago
I think similar hack should be done before,

 if (NS_SUCCEEDED(result = aPos->mResultFrame->PeekOffset(aPresContext, aPos)))
            return NS_OK;//else fall through

Also,
Please add #ifdef IBMBIDI.
Assignee

Comment 31

14 years ago
(In reply to comment #30)
> I think similar hack should be done before,
> 
>  if (NS_SUCCEEDED(result = aPos->mResultFrame->PeekOffset(aPresContext, aPos)))
>             return NS_OK;//else fall through

I think that's what Simon meant when he said, in comment 27:
> I should think you want to patch the eSelectWord case as well.

I explained in comment 28 that I tried it and that it did more damage than good.

> Also,
> Please add #ifdef IBMBIDI.

I'll certainly do this if I post an updated version of the patch. But I'll only
do that if I get a response from Simon indicating that the patch might be approved.
(In reply to comment #31)
> I'll certainly do this if I post an updated version of the patch. But I'll only
> do that if I get a response from Simon indicating that the patch might be
approved.

Please do. 
Assignee

Comment 33

14 years ago
Posted patch patch for comment 0, v2 (obsolete) — Splinter Review
- Added comments (hopefully not excessive) per Eyal's suggestion
- Used a variable (originalPreferLeft) to save the original value of
mPreferLeft per Simon's suggestion
- used #ifdef IBMBIDI per Ginn Chen's comment

I'll be away from my development machine for a couple of days, but I can still
manually fix the patch for issues regarding the comments or the name I chose
for the variable if necessary.
Assignee

Updated

14 years ago
Attachment #184203 - Attachment is obsolete: true
Attachment #184595 - Flags: review?(smontagu)
Assignee

Comment 34

14 years ago
The bug described in comment #2 is related to bug 277553, and is fixed by the
patch attached to that bug.
Assignee

Updated

14 years ago
Depends on: 277553
Assignee

Comment 35

14 years ago
Comment on attachment 128682 [details]
Testcase #3

The bug described in comment #6 was apparently fixed on the trunk between
2004-08-04 and 2004-08-05 (although I can't find a check-in which seems
related). Prog - can you please verify that this WFY on recent trunk builds?
Attachment #128682 - Attachment is obsolete: true

Comment 36

14 years ago
(In reply to comment #35)
> (From update of attachment 128682 [details] [edit])
> The bug described in comment #6 was apparently fixed on the trunk between
> 2004-08-04 and 2004-08-05 (although I can't find a check-in which seems
> related).

WFM on Trunk from 2005-06-21 Firefox/1.0+
Assignee

Comment 37

14 years ago
The problem was that GetFrameFromDirection() fails (there is no previous frame
to go to), but before failing, it flips aPos->mResultFrame. I don't see a
reason why  PeekOffset() shouldn't just fail in this case (as there is really
nowhere to go).

This patch is a logical extention of
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsTextFrame.cpp&rev2=1.295&rev1=1.294
 and is modelled after it.

Sadly, this does *not* fix bug 228290, which I'll investigate next.
Attachment #187512 - Flags: review?(smontagu)
Comment on attachment 187512 [details] [diff] [review]
[backed out] patch for comment 19

>         result = GetFrameFromDirection(aPresContext, aPos);
>         if (NS_SUCCEEDED(result) && aPos->mResultFrame && aPos->mResultFrame!= this)
>         {
>           result = aPos->mResultFrame->PeekOffset(aPresContext, aPos);
>           if (NS_FAILED(result))
>             return result;
>         }
>+        else if (NS_FAILED(result))
>+          return result;
>       }

I think it would be cleaner if you did

	  result = GetFrameFromDirection(aPresContext, aPos);
	  if (NS_FAILED(result))
	    return result;
	  if (aPos->mResultFrame && aPos->mResultFrame != this)

but r=smontagu either way
Attachment #187512 - Flags: review?(smontagu) → review+
Assignee

Comment 39

14 years ago
Comment on attachment 187512 [details] [diff] [review]
[backed out] patch for comment 19

I did it this way to be consistant with a similar structure above. Maybe they
should both be changed as part of a general cleanup.

Also, in comment 37, what I meant to say was "before failing, it flips
aPos->mPreferLeft".
Attachment #187512 - Flags: superreview?(roc)
Reporter

Comment 40

14 years ago
QA status report:

Fixed:
Comment #2, Testcase 2 (LTR textarea, Line 1)
Comment #6, Testcase 3 (RTL textarea, Line 2)
Comment #7, Testcase 4 (both textareas, Line 1)
Comment #8, Delete testcase (any textareas)
Comment #15, Testcase 5 (both textareas, Line 2 - "beginning of the second line")
Comment #17, Testcase 1 (LTR textarea, Line 3)

Broken:
Comment #0, Testcase 1 (RTL textarea, Line 1)
Comment #1, Testcase 1 (RTL textarea, Lines 3, 5 and 7)
Comment #2, Testcase 2 (RTL textarea, Line 1 and 7)
Comment #8, Delete then Backspace testcase (any textarea)
Comment #15, Testcase 5 (both textareas, Line 1)
Comment #15, Testcase 5 (RTL textarea, Line 2 - "leftmost, fourth"). The LTR
textarea is now also a bit buggy with this test (a regression?)
Comment #16, Testcase 6 (RTL textarea, Line 1)
Comment #19, Testcase 7 (LTR textarea, Line 1)

I've used Deer Park 1 trunk (Gecko/20050629) under WinXP Pro to test these bugs.
 I must say I'm very encouraged by the results.

Thanks for working on this Uri (btw, you might want to assign yourself to this bug).

Prog.
QA Contact: zach → prognathous
Assignee

Comment 41

14 years ago
(In reply to comment #35)

Huh - I just got a mid-air collision as I was writing a comment asking
Prognathous to go over the testcases and see what's still broken. Thanks for
doing that, Prog!

But I also wanted to suggest, given the size of this bug, the fact that many
issues were fixed, and the fact that the remaining issues (or at least some of
them) seem to be independant of each other, that we close this bug (or turn it
into a meta bug) and open separate bugs for the remaining issues.

As issues will (hopefully) get fixed, comment #35 will become outdated, and once
again it would be difficult to know what still needs fixing. Also, discussion of
patches, and marking dependencies, are difficult when so many issues are
discussed in the same place.

Simon, Prog - what do you think?
Since you're fixing bugs with small patches for individual issues, then I think
separate bugs make more sense.

When I said the opposite in comment 5 I was thinking that I would produce a big
patch that would fix everything, but we all know how much came of that in the
last two years.
Reporter

Comment 43

14 years ago
Good idea, Uri. I'll make this a meta bug and open separate bugs for the
remaining issues. It might have to wait for later tonight though, so keep those
patches coming ;-)

Prog.
Keywords: meta
Assignee

Comment 44

14 years ago
(In reply to comment #40)
> Broken:
> Comment #0, Testcase 1 (RTL textarea, Line 1)
> Comment #1, Testcase 1 (RTL textarea, Lines 3, 5 and 7)

Comment #1, TC 1 line 3 is exactly the same as Comment #0 - "1" and "a", for
this matter, are one and the same.

Comment #1, TC 1 line 7 actually WFM on a 20050628 OS X build (but not on DP
Alpha 1). This was probably fixed (or, more likely, just covered-up) by the fix
to bug 295142. Can you please re-check?
Keywords: meta
Assignee

Updated

14 years ago
Keywords: meta
Attachment #187512 - Flags: superreview?(roc) → superreview+
Assignee

Comment 45

14 years ago
Comment on attachment 187512 [details] [diff] [review]
[backed out] patch for comment 19

This is a trivial fix - asking for approval1.8b3.
Attachment #187512 - Flags: approval1.8b3?
Reporter

Updated

14 years ago
Depends on: 299239
Reporter

Updated

14 years ago
Depends on: 299240

Comment 46

14 years ago
Comment on attachment 187512 [details] [diff] [review]
[backed out] patch for comment 19

a=bsmedberg for checkin 6/30 only
Attachment #187512 - Flags: approval1.8b3? → approval1.8b3+
Comment on attachment 187512 [details] [diff] [review]
[backed out] patch for comment 19

Checking in nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--	nsTextFrame.cpp
new revision: 1.510; previous revision: 1.509
done
Attachment #187512 - Attachment description: patch for comment 19 → [checked in] patch for comment 19

Comment 48

14 years ago
This checkin may be responsible for bug 299371.
Comment on attachment 187512 [details] [diff] [review]
[backed out] patch for comment 19

backed out.
Attachment #187512 - Attachment description: [checked in] patch for comment 19 → [backed out] patch for comment 19
Assignee

Comment 50

14 years ago
Posted patch alternative patch for c. 19 (obsolete) — Splinter Review
This is an alternative approach for fixing comment #19, in case the bottom line
of the discussion in bug 299371 is that PeekOffset() is not allowed to fail.
The idea here is to make sure that nsFrame::GetFrameFromDirection does not flip
mPreferLeft unless it succeeds.
Reporter

Updated

14 years ago
Depends on: 299527
Assignee

Updated

14 years ago
No longer depends on: 277553
Assignee

Updated

14 years ago
No longer blocks: 228290
Reporter

Updated

14 years ago
Depends on: 299529
Reporter

Updated

14 years ago
Depends on: 299531
Reporter

Updated

14 years ago
Depends on: 299535
Reporter

Updated

14 years ago
Depends on: 299537
Reporter

Updated

14 years ago
Depends on: 299622
Assignee

Comment 51

14 years ago
Comment on attachment 184595 [details] [diff] [review]
patch for comment 0, v2

I moved this patch to bug 299239.
Attachment #184595 - Attachment is obsolete: true
Attachment #184595 - Flags: review?(smontagu)
Reporter

Updated

14 years ago
Depends on: 299838
Reporter

Updated

14 years ago
Depends on: 299842
Reporter

Comment 52

14 years ago
(In reply to Uri Bernstein, comment #41)
> But I also wanted to suggest, given the size of this bug, the fact that many
> issues were fixed, and the fact that the remaining issues (or at least some of
> them) seem to be independant of each other, that we close this bug (or turn it
> into a meta bug) and open separate bugs for the remaining issues.

Done. You can find a bugzilla query with those bugs here:
http://snipurl.com/Bug207186_dependents

Prog.
Assignee

Updated

14 years ago
Attachment #187512 - Attachment is obsolete: true
Assignee

Comment 53

14 years ago
Comment on attachment 188050 [details] [diff] [review]
alternative patch for c. 19

I posted the patches related to comment #19 on bug 299842.
Attachment #188050 - Attachment is obsolete: true
Assignee

Updated

14 years ago
Depends on: 277553
Assignee

Updated

14 years ago
No longer depends on: 277553
Assignee

Comment 54

14 years ago
(In reply to comment #40)
> QA status report:
> 
> Fixed:
> Comment #2, Testcase 2 (LTR textarea, Line 1)
> Comment #6, Testcase 3 (RTL textarea, Line 2)
> Comment #7, Testcase 4 (both textareas, Line 1)
> Comment #8, Delete testcase (any textareas)
> Comment #15, Testcase 5 (both textareas, Line 2 - "beginning of the second line")
> Comment #17, Testcase 1 (LTR textarea, Line 3)
> 

As it turns out, all of these problems were "fixed" by the (original) patch to
bug 254278, which was checked in on 2004-08-04 (that answers my comment #35).
What that checkin did (completely by accident) was to break the functionality of
nsSelection::GetPrevNextBidiLevels() in all but the most trivial case.

I spotted the mistake in that patch while investigating bug 299529. Biesi quckly
made a patch to fix it, and it will probably be checked in soon. This means that
all of the bugs above will be back. You can thank me later.

While some of you might be thinking "why not just leave things as they are" (not
checking in the the regression-fixing patch to 254278), I do not think it's the
right way to go. We will just need to go over the re-introduced bugs one by one
and figure out how to fix them.

Prog - I trust you to file these as separate bugs (except perhaps for comment
#8, which is arguably not a bug) as soon as the new patch to bug 254278 is in,
and they are reproducable again. Thanks.
Reporter

Comment 55

14 years ago
Uri, almost all the bugs that were "fixed" by the regression are still gone. I
tested this with Firefox/Trunk-20050712/WinXP. Only the bug in Comment #15 is
back. Any insight to offer?

Prog.
Assignee

Comment 56

14 years ago
(In reply to comment #55)
The regression fix for bug 254278 was not checked in yet (it did not get
approval1.8b3 on time) - so the bugs aren't back. I'll ask for approval1.8b4.
(I should have filed a separate bug instead of reopening 254278 - that would
have made things clearer. My apologies).
Assignee

Updated

14 years ago
Depends on: 301033

Comment 57

14 years ago
Hello,

Just my small comment, as someone who didn't look into this bug a lot:

It seems to me that it may be that those bugs are caused by trying to use visual
cursor movement, instead of logical cursor movement. Logical seems to me simpler
to implement - just go to the next or previous logical character. This is what
Windows does, and I personally prefer it, since not only the program knows where
the cursor is (what character will be deleted when I press DEL, for example) - I
can know it too.

Thanks all of you for working on this bug,
Noam Raphael
Assignee

Updated

14 years ago
Depends on: 302051
Assignee

Comment 58

14 years ago
Adding "[meta]" to the summary. Sorry for the spam.
Summary: Moving the caret in BiDi texts is buggy, especially in right-to-left forms → [meta] Moving the caret in BiDi texts is buggy, especially in right-to-left forms
Assignee

Updated

14 years ago
Depends on: 303399
Assignee

Updated

14 years ago
Depends on: 303781
Assignee

Updated

14 years ago
Depends on: 303884
Assignee

Updated

14 years ago
Depends on: 305798
Assignee

Updated

14 years ago
Depends on: 308017
Assignee

Updated

14 years ago
Blocks: 309286
Assignee

Updated

14 years ago
Depends on: 309432
Assignee

Updated

14 years ago
No longer blocks: 309286
Depends on: 309286
Assignee

Updated

14 years ago
Depends on: 313596
Assignee

Updated

14 years ago
Depends on: 313602
Assignee

Updated

14 years ago
Depends on: 320874
Assignee

Updated

14 years ago
Depends on: 320957
Assignee

Updated

14 years ago
Depends on: 327107
Assignee

Updated

13 years ago
Depends on: 328655
Assignee

Updated

13 years ago
Depends on: 328656
Assignee

Updated

13 years ago
Depends on: 330457
Assignee

Updated

13 years ago
Depends on: 330460
Assignee

Updated

13 years ago
Depends on: 330461
Assignee

Updated

13 years ago
Depends on: 330815
Assignee

Updated

13 years ago
Depends on: 331444
Assignee

Comment 59

13 years ago
All the dependencies are now fixed (on trunk). Yay!

I'd like to thank Mano for comment #22 (made two years and four days ago). Without it, it's likely that I wouldn't have started working on this bug (or even on Mozilla) at all. I did enjoy it, most of the time :)

I'd like to encourage anyone who has interest in this bug, and is comfortable working with nightly trunk builds, to download such a build and do some testing to verify that there are no regressions or additional issues. I'll keep this bug open, for any such regressions or issues to be found, now or in the future.

Comment 60

13 years ago
So is this bug fixed now, or what? Are there no more cases of erratic cursor movement?
Assignee

Comment 61

13 years ago
(In reply to comment #60)
> So is this bug fixed now, or what? Are there no more cases of erratic cursor
> movement?
> 

Sorry if I wasn't clear enough. All of the known issues were fixed (on trunk), and hopefully still are fixed. If you find any outstanding issues, please report them and make them block this bug.
(In reply to comment #59)
> I'd like to thank Mano for comment #22 (made two years and four days ago).
> Without it, it's likely that I wouldn't have started working on this bug (or
> even on Mozilla) at all.

In that case, I'd like to thank Mano too :)
Many many thanks Uri :)
Assignee: mozilla → uriber
Target Milestone: --- → mozilla1.9alpha
Reporter

Comment 64

13 years ago
I just tested the latest trunk, and I can confirm that everything related to this bug is fixed. For those who might be interested in the remaining BiDi caret bugs (not movement related), they are tracked in Bug 265070.

Uri, Fixing 32 bugs was well worth your effort. Users no longer need to use external editors for text composition. Personally, I can’t thank you enough for that!

Prog.

Comment 65

13 years ago
> Users no longer need to use external editors for text composition.

No, that's not true. There are text selection bugs, plus glazou's editor component is horribly broken in many ways, mostly behind the scenes but not just.
Assignee

Updated

12 years ago
Component: Layout: BiDi Hebrew & Arabic → Tracking
Assignee

Updated

11 years ago
Depends on: 427824

Comment 66

3 years ago
Marking all tracking bugs which haven't been updated since 2014 as INCOMPLETE.
If this bug is still relevant, please reopen it and move it into a bugzilla component related to the work
being tracked. The Core: Tracking component will no longer be used.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INCOMPLETE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.