142233 - English text inverted in RTL text-box and Hebrew text Inverted in LTR text box

Status: RESOLVED FIXED

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

Description

[Ronen Abrabanel]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1) Gecko/20020417
BuildID:    2002041711

while I'm writing in english in the "name" text box in the attached site and
others, Ther text is inverted:
while I write "untitled" it come out "deltitnu"...
The same thing happan when I'm tring to write hebrew in English Text boxses
(like the one in the Bugzilla Form...)

Reproducible: Always
Steps to Reproduce:
writing english in hebrew text box


Actual Results:  The text is inverted

Expected Results:  the text shuld be normal, not inverted.

Comment 1

[Boris Zbarsky [:bzbarsky]]

to BiDi....   This looks very odd, but I can't shake the feeling that this is
what the spec says for textboxes with dir specified..

Comment 2

[Simon Montagu :smontagu]

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

Comment 3

[Simon Montagu :smontagu]

If the charset of a page is visual (ISO-8859-8), as it is on the fisheye.co.il
URL listed here and the nana.co.il URL in bug 142455, this is indeed what the
spec says for <dir=rtl>. However, Hebrew text should not be inverted in a
Bugzilla form, unless the charset has been reset to ISO-8859-8. Have you set
that as your default charset?

On the other hand, because this behaviour is annoying and unexpected, we have a
pref to always treat form fields as logical, whatever the charset, and this
should be the default. In other words, this looks like a regression of bug 82849.

*** This bug has been marked as a duplicate of 82849 ***

Comment 4

[Simon Montagu :smontagu]

Further testing shows that the regression is only in text input fields (either
<input type="text"> or <textarea>, so I am un-duping this bug and will work on
it here.

Comment 5

[Simon Montagu :smontagu]

accepting

Comment 6

[Shoshannah Forbes]

If it helps any, I have seen this bug re-appear on Mac OS nightly builds around 
2002-04-21 
(see my comment #23 from bug #82849 )

Comment 7

[Simon Montagu :smontagu]

Thanks for the hint, Shoshannah. This regression is caused by the checkin from
bug 135146: if I unset the NS_FRAME_REFLOW_ROOT frame state bit in
nsGfxTextControlFrame2.cpp as a quick and dirty way of backing that out, the
text fields are correctly ordered.

Why? Because the truncation of the reflow path means that we no longer pass
through the IBMBIDI code in nsLineLayout::ReflowFrame that unsets visual mode in
the presContext if we are in a form control.

So... I guess we need either to clone or move that code somewhere else which
will catch all cases.

Comment 8

[Frank Tang]

looks like important issue. add nsbeta1+ for the regression .

Comment 9

[Shoshannah Forbes]

Any progress here? This is THE most common complaint I am hearing about moz 1.0
here in Israel (closely followed by bug 119881 ).
Please please do something here...

Comment 10

[Jaime Rodriguez, Jr.]

If this is an important issue, and the top issue for 1.0 in Israel, than it
needs to be resolved asap. pls provide an eta, or mark this one as nsbeta1-. thanks!

Comment 11

[Simon Montagu :smontagu]

I am working on a solution for this. eta this week.

Comment 12

[Simon Montagu :smontagu]

Attachment: Patch v.1

This moves the test for being in a form control from nsLineLayout.cpp to
nsBidiPresUtils.cpp, and tests for specific controls by atom rather than QIing
the frame to nsIFormControlFrame.

This looks like a more stable approach than the original, i.e. it doesn't just
fix this regression but should be less likely to regress again in future. First
of all, we are now testing the thing that we really care about, not something
else related to it, and secondly we are testing in the part of the code which
wants to know, rather than setting a flag in the prescontext in one place and
using it in another.

Comment 13

[Simon Montagu :smontagu]

I tested the patch today on Windows and Linux, and it works well.

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 87240 [details] [diff] [review]
Patch v.1

You can test whether the content is a form control much easier using
content->IsContentOfType(nsIContent::eHTML_FORM_CONTROL).

Also, it seems like there ought to be some way to avoid recursing to the root
of the tree every time you check for this ... I'd ask kin, but I'd bet form
control children never go more than 3 deep (and it may just be 2 deep).

Other than that, it seems good to me, though I shudder at the necessity for it.

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 87240 [details] [diff] [review]
Patch v.1

Yes, this patch sucks performance-wise. The worst part is that standards
compliant pages are going to take the same hit.

Comment 16

[Simon Montagu :smontagu]

Attachment: Patch v.2

Comment 17

[John Keiser (jkeiser)]

Comment on attachment 87441 [details] [diff] [review]
Patch v.2

Patch looks good.  Any chance you could add the notation [OUT] to that @param
aIsInFormControl comment?  Just a personal request, I'm trying to homogenize
our API documentation a little.  You *rock* for adding comments without anyone
asking! :)

Could you please get sr= from kin or at least get him to tell you for sure how
deep the anonymous content under the textarea can go?  Also, please note that
in the future we may actually make the textarea an HTML editor, in which case
all bets are unfortunately off.

r=jkeiser

Comment 18

[Simon Montagu :smontagu]

I talked to kin on IRC last night, and he isn't happy with *any* assumption
about the depth between the form control and the text node.

Comment 19

[John Keiser (jkeiser)]

Okey dokey then.  I guess we have no choice but to have absolutely godawful
performance.  I can't figure out any way to determine if nodes are under a form
control--unless there is some quick way to tell if a node is either anonymous
itself or under some anonymous content?

We really ought to try and solve this problem one way or another.  Is there a
way we can sidestep it by having some sort of special direction style applied to
elements under a form control, perhaps?

Comment 20

[John Keiser (jkeiser)]

The more I think about it, the more I think a -moz-bidi-reverse-ltr style is a
good idea.  However, don't let this stop you from checking this in unless you
already know how to make styles and can do it quickly; we can open a new bug.

Comment 21

[Simon Montagu :smontagu]

Indeed I don't know how to make styles, so I'll go back to the version without
SEARCHDEPTH, and twiddle the logic a bit so that we only take the perfomance hit
on visual pages.

Comment 22

[Simon Montagu :smontagu]

Attachment: Patch v.3

I have made the search through the tree only happen on visual pages and
simplified the logic that toggles visual mode when we discover a form control.

This makes the pref setting to have visual form controls on logical pages
unsupported, but I can't imagine anyone ever needing it anyway, and I have
added a note to that effect to all.js

Comment 23

[John Keiser (jkeiser)]

Comment on attachment 87744 [details] [diff] [review]
Patch v.3

r=jkeiser

Comment 24

[Frank Tang]

simon- did you follow up the sr=? please send mail, aim, phone call to follow up
asap .thanks

Comment 25

[Simon Montagu :smontagu]

I spoke to kin today and am following up in parallel two ideas which he
suggested: a) get some figures on the performance impact
b) try going back to another approach (which I couldn't get to work earlier) of
doing this by setting a bit in the nsHTMLReflowState.

Comment 26

[Simon Montagu :smontagu]

Attachment: Patch v.4

This is a totally new approach which kin suggested to me: leaving the current
code more or less in place and, on reflow roots in visual pages only, looking
up the frame tree for a form control.

Comment 27

[kinmoz]

Comment on attachment 88574 [details] [diff] [review]
Patch v.4

Yeah this is exactly the approach I had in mind when we were talking! :-)
Thanks for looking into it and doing the perf measurements.

sr=[e-mail]

I'm just wondering if this should be encapsulated in some sort of bidi utility
function ... it's probably not worth it if this is the only place that will
ever use it though.

Comment 28

[John Keiser (jkeiser)]

Comment on attachment 88574 [details] [diff] [review]
Patch v.4

Need to check content rather than frame to see if it is a form
control--otherwise XBL Form Controls won't work.  Additionally, I am concerned
about the nsLineLayout change being able to catch all reflows of form
controls--but we're working that out with dbaron in IRC right now.

Comment 29

[Simon Montagu :smontagu]

Attachment: Patch v.5

Changed tests of frame type to test of content type to catch XBL form controls.

Also moved the original test from nsLineLayout to nsBlockFrame to catch
controls with {display: block} and added a few conditions there to catch cases
that were getting missed, as explained in comments in source.

Comment 30

[Simon Montagu :smontagu]

Attachment: Patch v.5

Responding to comments from jkeiser in IRC, I have moved all the tests to a
method called from the constructor of nsHTMLReflowState and set a bit in the
reflow state's flags which then gets inherited by the child reflow states, and
eventually passed to nsBidiPresUtils::Resolve, rather than resetting the state
in the presContext.

Comment 31

[John Keiser (jkeiser)]

Comment on attachment 89607 [details] [diff] [review]
Patch v.5

r=jkeiser, but could you change the comment for IsBidiFormControl() to a
JavaDoc comment?  It would look like this:

/**
 * Test whether the frame is a form control in a visual Bidi page.
 * This is necessary for backwards-compatibility, because most visual
 * pages use logical order for form controls so that they will
 * display correctly on native widgets in OSs with Bidi support
 * @param aPresContext the pres context
 * @return whether the frame is a BIDI form control
 */

Comment 32

[kinmoz]

Comment on attachment 89607 [details] [diff] [review]
Patch v.5

sr=[e-mail]

Comment 33

[Simon Montagu :smontagu]

checked in to trunk. Setting adt1.0.1 per comment 9 and 10

Comment 34

[Paul Wyskoczka]

Adding adt1.0.1- per the adt.  Let's try to get this fix into the next release.

Comment 35

[Uri Bernstein]

Attachment: testcase

Testcase, per BZ's request (via e-mail).

I verified that this testcase is broken in Mozilla 1.0.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Checked in the test to the reftests test suite.  Thanks for the test, Uri!

Comment 37

[(no longer active)]

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

Comment 38

[Javier Pintos]

Actually the problem persist. I have a Galaxy S Advance with Android 2.3.6 and Firefox V18.0 and when i write in Spanish websites the text is writed inverted... for example eht txet si detrevni :(

Comment 39

[Jonathan Kew [:jfkthame]]

(In reply to Javier Pintos from comment #38)
> Actually the problem persist. I have a Galaxy S Advance with Android 2.3.6
> and Firefox V18.0 and when i write in Spanish websites the text is writed
> inverted... for example eht txet si detrevni :(

The original issue here was fixed; I think you must be seeing a new issue, perhaps related to a particular configuration somehow. Please file a separate bug report about this, with details of your system and a specific example of a site or sites where you experience this problem.