Closed Bug 299842 Opened 15 years ago Closed 15 years ago

BiDi: Recursive caret movement in LTR lines which begin with an RTL word or character

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.8beta4

People

(Reporter: bugzillamozilla, Assigned: uriber)

References

Details

(Keywords: rtl)

Attachments

(2 files, 1 obsolete file)

To reproduce:

1. Open https://bugzilla.mozilla.org/attachment.cgi?id=137983
2. In the second textarea (LTR), put the caret anywhere on the Hebrew word 
   ("עברית").‎
3. Hold the LeftArrow button pressed.

Actual result:
The caret moves recursively to left. Every time it reaches the beginning of the
line (leftmost part), it comes back in the beginning of the Hebrew word (to
right of the word).

Expected result:
The caret should stop at the beginning of the line. This is the expected
behavior in Visual Caret Movement, as is the convention in Gecko.

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050705 Firefox/1.0+

First reported in Bug 207186, comment #19

Uri, if I understand correctly, your patch for Bug 299239 should also fix this
one. If so, please close this as WORKSFORME after you get that patch checked in.

Thanks,

Prog.
> Uri, if I understand correctly, your patch for Bug 299239 should also fix this
> one. If so, please close this as WORKSFORME after you get that patch checked in.

No, this is a separate issue, for which I attached a patch to bug 207186. That
patch was checked in but caused a regression, so it was backed out. I then
suggested an alternative patch. I'll post both these patches here shortly.
Attached patch patch A (obsolete) — Splinter Review
This is the original patch I proposed in bug 207186 comment 37. It got
r+:smontagu and sr+:roc, and was checked in, but it caused a regression: bug
299371, so it was backed out.
I'm still not sure if the approach here (PeekOffset() returning an error code
when there is nowhere to go) was really wrong, or whether it merely exposed a
problem elsewhere in the code.
Attached patch patch BSplinter Review
An alternative patch, originally posted on bug 207186, comment #50.
The idea here is to make sure that nsFrame::GetFrameFromDirection does not flip

mPreferLeft unless it succeeds.

This does not cause the regression caused by patch A.
Attachment #188432 - Flags: review?(smontagu)
Attached file testcase
This testcase shows that the problem can also occur in the reverse case (RTL
textarea, strating with LTR text, using right-arrow).
Status: NEW → ASSIGNED
Comment on attachment 188432 [details] [diff] [review]
patch B

Since I got no response on bug 299371, let's just go with option "B", which
fixes this bug without causing a regression.
Since Simon is on vacation, and this is a small, simple, patch - moving review
request to roc.
Attachment #188432 - Flags: superreview?(roc)
Attachment #188432 - Flags: review?(smontagu)
Attachment #188432 - Flags: review?(roc)
Attachment #188429 - Attachment is obsolete: true
Attachment #188432 - Flags: superreview?(roc)
Attachment #188432 - Flags: superreview+
Attachment #188432 - Flags: review?(roc)
Attachment #188432 - Flags: review+
Attachment #188432 - Flags: approval1.8b4?
Attachment #188432 - Flags: approval1.8b4? → approval1.8b4+
Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.573; previous revision: 3.572
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta4
Status: RESOLVED → VERIFIED
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
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.