312550 - Incorrect wrapping in RTL textarea with horizontal scrollbar

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

When an RTL textarea has a horizontal scrollbar (because one of the lines is
unbreakable and too wide to fit in the textarea), the rest of the text wraps
incrorrectly, i.e, it wraps according to the width of the longest line, instead
of the width of the textarea itself. This does not happen in LTR textareas.

Testcase coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

Notice that in the LTR textarea, the second line wraps after "33333", whereas
in the RTL textarea it only wraps after "66666".

Also, the initial position of the scrollbar on the RTL textarea is wrong - but
that might be a separate bug.

Comment 2

[Uri Bernstein]

Attachment: patch?

This patch (removing some bidi-specific code overriding the normal available width for lreflowing lines) fixes this bug, as well as bug 306349 and bug 313365.

The downside is that unbreakable lines which are too long to fit in the textarea now extend to the right, instead of to the left. However, I believe that this is not really a new problem being introduced, as I can see the same thing in fixed-width RTL DIVs. Also, the overall adverse effect of this is, IMO, much less than the combined effect of the three resolved bugs.

Simon - do you remember why this code was added in the first place? Can you think of more things that would break if it's removed?

Comment 3

[Uri Bernstein]

Following Simon's suggestion, I checked the testcases for bug 108187 and bug 131023, and neither of them is regressed (or otherwise affected) by this patch.

Comment 4

[Uri Bernstein]

Attachment: testcase w/ textarea and div

This testcase contains an RTL textarea (similar to the first testcase), and an RTL DIV, with fixed width and scroll:auto.
The patch causes the textarea to behave the way the DIV currently behaves (text wraps on correct width, but the overflowing line extends to the right instead of the left).
The patch does not affect the rendering of the DIV.

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 204126 [details] [diff] [review]
patch?

I believe that this code worked together with IBMBIDI code in nsGfxScrollFrame.cpp which was removed in bug 240276, so it should go.

Comment 6

[Uri Bernstein]

Comment on attachment 204126 [details] [diff] [review]
patch?

ROC, notice that this patch  makes your XXX comment in http://lxr.mozilla.org/seamonkey/source/layout/generic/nsGfxScrollFrame.cpp#793
("the way this works...") obsolete (which is a good thing. That behavior was annoying). Perhaps there is additional bidi code in nsGfxScrollFrame which can now be removed or simplified?

Comment 7

[Uri Bernstein]

I submitted the problem I mentioned in comment #2 as bug 318116 (which is focused on DIVs - but I think that this is the same problem we have with texareas with the proposed patch here).

Comment 8

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

Do you want me to check this in? (or are you getting CVS access real soon?)

Comment 9

[Uri Bernstein]

(In reply to comment #8)
> Do you want me to check this in? (or are you getting CVS access real soon?)
 
I was hoping for this to be my first checkin with my account when I get it, but I spoke to Marcia today and she still seems to be very busy - so I'll take you up on checking this in for me, and I'll have to come up with another patch to test-drive my account on. Thanks.

Comment 10

[Uri Bernstein]

Patch checked in:
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.749; previous revision: 3.748
done

Comment 11

[Simon Fraser [no longer active]]

Would this be suitablel for the branch (so Camino can pick it up)?

Comment 12

[Uri Bernstein]

(In reply to comment #11)
> Would this be suitablel for the branch (so Camino can pick it up)?

Since nobody else answered, I'll state my opinion: I think this can, and should, get on the 1.8 branch. I even consider nominating it for the 1.8.0.x branch (once it's on the trunk for long enough). 

Comment 13

[Mike Pinkerton (not reading bugmail)]

nominating for 1801.

Comment 14

[Uri Bernstein]

Note to 1.5.0.1 release drivers:

The patch is nominated for 1.8.0.1 no so much for fixing this bug, but mainly for fixing bug 313365, and (via bug 306349), bug 304609.
Both of these bugs are regressions in major functionality (typing into RTL textareas) from 1.0.x (more precisely, Gecko 1.7.x, since the latter only affects Camino).

The patch has been on the trunk since 2005-10-14 with no reported regressions so far.
The patch, by its nature, impacts only RTL textareas and input elements (which were tested), so the risk should be very limited.
There is no l10n impact and no API changes.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 204126 [details] [diff] [review]
patch?

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Comment 16

[Uri Bernstein]

Checked into 1.8 and 1.8.0 branches:

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.729.4.8; previous revision: 3.729.4.7

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.729.4.7.2.1; previous revision: 3.729.4.7

Comment 17

[Jay Patel [:jay]]

v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060104 Firefox/1.5.0.1, wrapping looks correct with Uri's testcase.

Comment 18

[(no longer active)]

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