162242 - BiDi: Text input is auto switching between Arabic/Hebrew and English

Status: RESOLVED FIXED

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

Description

[Thamer Mahmoud]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1b) Gecko/20020808
BuildID:    2002080813

This happens after viewing and selecting text on Arabic and Hebrew websites. It
requires using the language hotkey every time for switching back to English and
typing an address (which is annoying). This bug is a possible regression, as it
never happened under 1.1b.

Reproducible: Always
Steps to Reproduce:
1. Go to URL or any Arabic/Hebrew site
2. Select some text then deselect it
3. Repeat step 2
4. Type some text in the Navigation bar


Actual Results:  Navigation bar Input switched to Arabic.

Expected Results:  Input is always English unless the user uses hotkey.

Comment 1

[Thamer Mahmoud]

Gecko/20020826

Changing description since this is not just Navigation bar related.

Another way to reproduce this bug is that if you're trying to fill a form:  

1. Type some Arabic text in an empty text field.
2. Type some Arabic text in *another* empty text field. 

Step 2 requires pressing the language hotkey again.

Comment 2

[Prognathous]

Confirmed. This bug happens with both Hebrew and Arabic websites  on
Win32/1.3b/20030109.

There is no need to select and deselect the text twice. Merely selecting it once
is enough to make Mozilla switch the input language.

Prog.

Comment 3

[Thamer Mahmoud]

After more investigation, it seems to happen with every mouse click. Keyboard
related caret switching is probably another bug.

A fix for this bug is to have no switching at all, and just stay with the
expected (and IE6's) caret behavior.  Also another fix is to have some
preconditions to avoid all the useless caret switching. 

Something like this:

Caret switching should *only* happen if a user clicked inside an editable region
(textarea, input, composer...etc) AND no switching for empty (blank) editable
regions.

Comment 4

[Prognathous]

I seem to have mistakenly commented on this issue in the wrong bug, so here are
some more details about the problem:

Mozilla also switches the input language to English in the following scenarios:

1. Click on an empty textarea (use test-URLs below) and type a single Hebrew
character.
2. Press Backspace twice.

Result: the input language changes to English.

1. Using Hebrew input, click on an empty textarea and type a digit, then space.
2. Press Backspace twice.

Result: the input language changes to English (and even worse - the digit is not
deleted).

The easiest solution to all of these bugs is to completely remove the
auto-switching of input languages. Native text widgets in Windows don't do it,
so why should Mozilla invent it's own standard? it is totaly inconsistent with
what the behavior that users expect.

BTW, this mechanism is not implemented in the BeOS version of Mozilla and the
result is a better experience for end-users (which is absurd really, unlike
Windows, BeOS doesn't natively support Hebrew...)

Finally, Is there any simple way for users to disable this "feature"?

Prog.

Visual Hebrew: http://forums.ort.org.il/scripts/addform.asp?which_forum=30
Logical Hebrew: http://www.mentor.org.il/mb/mentor/post.shtml

Comment 5

[Prognathous]

Accepting. What the hell, I'm hardly a programmer, but at least I managed to
create an initial fix. Download it (for testing) from the following link:
http://oren.gomen.org/mozilla/bug_162242/gkwidget.zip

Take note that it's a very quick and dirty hack. But at least it works - here.
To install, rename gkwidget.dll under \Program Files\Common
Files\mozilla.org\GRE\1.5_2003100716\components folder, replace it with the one
in the zip, and kiss this annoying language switching goodbye :-)

Thanks for smonatgu for pointing me to the place in the code that holds this code.

I'll create a patch once I find out how to do it...

Prog.

Comment 6

[Tsahi Asher]

use PatchMaker from http://www.gerv.net/software/patch-maker/ (though i tried it
on my WinXP and id didn't work, but i understand it's the standard here.)

Comment 7

[timeless]

don't bother with patchmaker, we'll talk you through cvs diff -up on irc.

Comment 8

[Prognathous]

Attachment: Patch to disable the automatic keyboard layout switching

Thanks Tsahi and Timeless, but I already found a mozillazine thread that
explains how to create a patch using cvs: http://snipurl.com/patch_with_cvs

The attached patch seems to work very well here. I decided not to take any
risks, and to only disable the actual (automatic) keyboard switching, not the
rest of nsBidiKeyboard.cpp, as this code can also provide information about the
currently selected keyboard layout. I'm not sure if this information is used,
but if it is, then it's still available after my patch.

Prog.

Comment 9

[timeless]

Comment on attachment 138201 [details] [diff] [review]
Patch to disable the automatic keyboard layout switching

Instead of using /*, use #if 0/#ifdef SOMETHING_DESCRIPTIVE_BUT_NOT_DEFINED

#ifdef AUTOSWITCH_KEYLAYOUT
>-  if (strcmp(mCurrentLocaleName, currentLocaleName)) {
>+/*if (strcmp(mCurrentLocaleName, currentLocaleName)) {

>     if (!::LoadKeyboardLayout(currentLocaleName, KLF_ACTIVATE | KLF_SUBSTITUTE_OK)) {
>       return NS_ERROR_FAILURE;
>     }
>-  }
>+  }*/  //disable keyboard layout switching
#endif

Doing this means that you don't own blame for any of the code lines and
switching would be easier.

Although I doubt we'd ever want to use that code, but that's another story.

Comment 10

[Prognathous]

Attachment: Patch #2

This disables the same code, but uses preprocessor directives, as suggested in
comment #9.

Prog.

Comment 11

[David Baron :dbaron:]

Comment on attachment 138208 [details] [diff] [review]
Patch #2

If your intent is to disable the code, use |#if 0| rather than |#ifdef| and
some macro that somebody could decide to define at some point in the future.

Comment 12

[Prognathous]

Attachment: Patch #3

The new patch uses |#if 0| and adds the following rational: 

"This implementation of automatic keyboard layout switching is too buggy to be
useful and the feature itself is inconsistent with Windows."

Prog.

Comment 13

[Shoshannah Forbes]

Abinary version of mozilla wth this patch is avalible here:
http://www.xslf.com/mozilla_bin.exe

Comment 14

[Prognathous]

Comment on attachment 138242 [details] [diff] [review]
Patch #3

Automatic language switching has been one of the biggest nuisances for BiDi
users. Now that it's fixed, it would be a great reason for many to upgrade, but
there's no reason to keep them waiting for 1.7

I have tested the above build (1.6, 20040101) with this patch appllied, under
WinXP and Win98, and got very good results - with no regressions whatsoever. In
addition, at least two other users have tested it with Win2k and also had a
very good experience.

Among the tests that I did were to write both English and Hebrew text in
multi-line and single-line browser textareas, as well as doing the same with
Composer and Mail. Everything went well.

Now, since this is a truly a "high reward, low risk" fix, I ask for approval1.6

Prog.

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 138242 [details] [diff] [review]
Patch #3

r=smontagu (You need r= as well as approval before this can be checked in).

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsBidiKeyboard.cpp;
/cvsroot/mozilla/widget/src/windows/nsBidiKeyboard.cpp,v  <--  nsBidiKeyboard.cpp
new revision: 3.11; previous revision: 3.10
done

Comment 17

[Tsahi Asher]

now you'll get noticed too :)
see comment 14.

Comment 18

[Eyal Rozenberg]

I hate to rain, on the parade, and the patch does indeed fix the problem of
inadvertant switching from English to Hebrew/Arabic, there's still the matter of
whether or not there should be a switch from Hebrew/Arabic to English in the URL
bar: bug 204999 ... Prognathous and other CC list members: how do you feel about
constricting the language in the URL bar to English, instead of disabling
auto-switching?

Comment 19

[Prognathous]

Verified.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040105

Eyal, as for your suggestion in bug 204999, I wouldn't want it personally. I
prefer UI consistency, and that translates to _no automatic language switching_.
Ever.

Prog.

Comment 20

[Mike Kaply [:mkaply]]

Can someone quantify how many people complain about this?

Will we alienate anyone by changing the current behavior?

Comment 21

[Prognathous]

I don't think there are any chances that anyone will miss this feature, at least
not the way it was implemented. If you like, we can set up some polls to find
out. The question is, do we still have time?

Prog.

Comment 22

[Mike Kaply [:mkaply]]

Comment on attachment 138242 [details] [diff] [review]
Patch #3

I'll take your word for it.

Get it in VERY quick.

Comment 23

[Mike Kaply [:mkaply]]

checked in to 1.6

Comment 24

[Eyal Rozenberg]

No no no no. If you want to prevent automatic kb layout switches, you prevent calls to SetLangFromBidiLevel ; you do _not_ cripple the method's action so as to render it useless. 

dbaron: Please back out this patch. 
prog: Please check for gratuitous calls to SetLangFromBidiLevel and create a patch which comments them out.

You see, I need SetLangFromBidiLevel to work.

Comment 25

[Uri Bernstein (Google)]

(In reply to comment #24)
> prog: Please check for gratuitous calls to SetLangFromBidiLevel and create a
> patch which comments them out.

Actually, if we're already doing this, we should probably have a pref controlling whether to do auto switching or not, and check for that pref before calling SetLangFromBidiLevel, instead of just commenting out the calls.

Comment 26

[Uri Bernstein (Google)]

Attachment: restore with pref

Like this, perhaps.
Caveat: I don't have Windows, so I couldn't actually test this.

Also, I probably should be caching the value of the pref somewhere (but where?)

Comment 27

[Eyal Rozenberg]

> Like this, perhaps.
I like it. Although maybe a more comprehensive rewrite is in order.

Comment 28

[Uri Bernstein (Google)]

In retrospect, reopening this bug for re-enabling auto-switching was not the proper action. I'm closing it again, and I'll file a separate RFE for re-enabling auto-switching.

Comment 29

[Uri Bernstein (Google)]

Comment on attachment 218402 [details] [diff] [review]
restore with pref

obsoleting this page. I'll attach an updated version of it to bug 413267.