333433 - Typing an RTL character on the third line (and onwards) of an LTR textarea, on a pure LTR page, causes text duplication

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

In a LTR textarea containing LTR text, on a page which has no RTL text at all on it, typing an RTL letter on the third line (and onwards) of the textarea, causes text duplication.

I'll attach a testcase soon.

This is a regression from bug 329878 (although I don't yet understand why).

Comment 1

[Uri Bernstein]

Attachment: testcase

Instructions included.

I should have said that this requires the line to be the third line of wrapping text, with no linebreaks.

Comment 2

[Niro]

I reoroduced it on Windows XP.
Please change OS and Hardware values to "All"

Comment 3

[Uri Bernstein]

Thanks, Nir.

Comment 4

[Uri Bernstein]

Attachment: patch

Set the NS_FRAME_IS_BIDI flag on existing fluid continuations being "left alone" for reuse by the line breaking code. Apparently that code needs to know it's dealing with bidi stuff.

On a side note, I question the usefulness of the NS_FRAME_IS_BIDI flag, as in all cases I know of, it seems to be set for all the frames on a page which contains any RTL at all. This is why adding any RTL anywhere on the testcase would cause the bug to disappear - it would make all frames have the flag set anyway.

Comment 5

[Uri Bernstein]

Attachment: patch v2

Actually, I should probably do this for RemoveBidiContinuation as well. Sorry for the spam.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217860 [details] [diff] [review]
patch v2

Looks ok, but if we can get rid of this state bit, we should!

Comment 7

[Simon Montagu :smontagu]

(In reply to comment #4)
> On a side note, I question the usefulness of the NS_FRAME_IS_BIDI flag, as in
> all cases I know of, it seems to be set for all the frames on a page which
> contains any RTL at all.

It shouldn't be being set on frames in an LTR-only paragraph even if other paragraphs contain RTL. Is it?

Comment 8

[Uri Bernstein]

(In reply to comment #7)
> 
> It shouldn't be being set on frames in an LTR-only paragraph even if other
> paragraphs contain RTL. Is it?
> 

I double-checked, and it is. Resolve() is called on the LTR-only paragraph (should it be?), and it calls AdjustOffsetsForBidi(), which sets the flag.

Comment 9

[Simon Montagu :smontagu]

> I double-checked, and it is. Resolve() is called on the LTR-only paragraph
> (should it be?)

If we don't call Resolve() we won't know whether it's an LTR-only paragraph.

> and it calls AdjustOffsetsForBidi(), which sets the flag.

That seems wrong to me. I think EnsureBidiContinuation() should be setting it.

Comment 10

[Uri Bernstein]

Checked in:

Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.76; previous revision: 1.75
done

Comment 11

[Uri Bernstein]

(In reply to comment #9)
>
> That seems wrong to me. I think EnsureBidiContinuation() should be setting it.
> 

I tracked this back to the patch on bug 74946, which added setting the flag on both branches of the |if ( (runLength > 0) && (runLength < fragmentLength) )|.
As far as I can tell, the behaviour didn't change ever since that patch was landed.

Comment 12

[Simon Montagu :smontagu]

I still think it's wrong :) The bit was originally supposed to mean "this frame either is a nextBidi or has a nextBidi"[1], though it is overloaded for TextBoxFrames to mean "this frame has some RTL content". Let's continue this discussion in a new bug.

[1]http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsIFrame.h&rev=3.316&mark=213-214#211

Comment 13

[Simon Montagu :smontagu]

The new bug is bug 333500

Comment 14

[(no longer active)]

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