Closed Bug 162242 Opened 19 years ago Closed 14 years ago

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

Categories

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

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neokuwait, Assigned: bugzillamozilla)

References

(Depends on 1 open bug, )

Details

(Keywords: fixed1.6)

Attachments

(1 file, 3 obsolete files)

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.
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.
Summary: Bidi: Navigation bar input text is auto switching to Arabic → BiDi: Text input is auto switching between Arabic/Hebrew and English
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Blocks: 179377
Blocks: 222932
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.
Status: NEW → ASSIGNED
Assignee: mkaply → prognathous
Status: ASSIGNED → NEW
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.)
don't bother with patchmaker, we'll talk you through cvs diff -up on irc.
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 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.
Attached patch Patch #2 (obsolete) — Splinter Review
This disables the same code, but uses preprocessor directives, as suggested in
comment #9.

Prog.
Attachment #138201 - Attachment is obsolete: true
Attachment #138208 - Flags: superreview?(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.
Attachment #138208 - Flags: superreview?(dbaron) → superreview-
Attached patch Patch #3Splinter Review
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.
Attachment #138208 - Attachment is obsolete: true
Attachment #138242 - Flags: superreview?(dbaron)
Attachment #138242 - Flags: superreview?(dbaron) → superreview+
Abinary version of mozilla wth this patch is avalible here:
http://www.xslf.com/mozilla_bin.exe
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.
Attachment #138242 - Flags: approval1.6?
Comment on attachment 138242 [details] [diff] [review]
Patch #3

r=smontagu (You need r= as well as approval before this can be checked in).
Attachment #138242 - Flags: review+
Checking in nsBidiKeyboard.cpp;
/cvsroot/mozilla/widget/src/windows/nsBidiKeyboard.cpp,v  <--  nsBidiKeyboard.cpp
new revision: 3.11; previous revision: 3.10
done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
now you'll get noticed too :)
see comment 14.
Flags: blocking1.6?
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?
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.
Status: RESOLVED → VERIFIED
Can someone quantify how many people complain about this?

Will we alienate anyone by changing the current behavior?
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 on attachment 138242 [details] [diff] [review]
Patch #3

I'll take your word for it.

Get it in VERY quick.
Attachment #138242 - Flags: approval1.6? → approval1.6+
checked in to 1.6
Flags: blocking1.6? → blocking1.6+
Keywords: fixed1.6
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(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.
Attached patch restore with pref (obsolete) — Splinter Review
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?)
Attachment #218402 - Flags: review?(smontagu)
> Like this, perhaps.
I like it. Although maybe a more comprehensive rewrite is in order.
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.
Status: REOPENED → RESOLVED
Closed: 18 years ago14 years ago
Resolution: --- → FIXED
Depends on: 413267
Comment on attachment 218402 [details] [diff] [review]
restore with pref

obsoleting this page. I'll attach an updated version of it to bug 413267.
Attachment #218402 - Attachment is obsolete: true
Attachment #218402 - Flags: review?(smontagu)
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.