418513 - Notch on caret when RTL languages are present on a page is misinterpreted as a polish problem

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

When giving the focus to a text field on a page that contains right to left languages, the cursor in the field picks up a small notch that extends towards the right.

Most users do not understand the correlation between a small area of a foreign language on the Web page, and this new appearance of the cursor in the text field, so this behavior is very commonly misinterpreted as a graphical glitch, or a polish problem.

Is there any way we could only display directional text field cursors for RTL localizations?  Also, can text fields actually take RTL input if you are using a LTR localization?  If not, the visual affordance would be superfluous.

Comment 1

[Behnam Esfahbod [:zwnj]]

We only display a small notch (the bidi marker) if and only if you have keyboard layouts with both ltr and rtl directions.

The Win backend was around for a while, and should be fine.

The GTK backend has a little problem in the trunk, which is bug 348724.  It's been almost fixed in the branch too.

And IIRC there's no implementation for Mac.  So you shouldn't see the marker in Mac at all.

So, in Win and Linux, You should not see the marker if you have keyboard layouts in only one direction.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Image of the bidi marker on OS X

I took this screenshot in the comments field of bug 407861 (which contains the phrase جريدة القاهرة).  The comments field of this bug should now get the bidi marker as well.

using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008021904 Minefield/3.0b4pre

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> We only display a small notch (the bidi marker) if and only if you have
> keyboard layouts with both ltr and rtl directions.

Is this only true on the trunk? I've seen the bidi caret many times, and the only keyboard layouts I have "set up" are French and English. I can certainly reproduce with current 1.8 branch builds, but it's possible that I'm remembering a problem that has since been fixed on the trunk.

(Note that bug 277701 is somewhat related - it's about the bidi caret getting "stuck" once it appears.)

Comment 4

[Dotan Cohen]

On Tbird 2.0.0.12 in KDE I get the notch pointing to the right on LTR languages, and to the left on RTL languages.

@Alex: If you have the bidi implementation, then you can switch textbox direction with Ctrl-Shift-X.

Comment 5

[(no longer active)]

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

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #1)
> And IIRC there's no implementation for Mac.  So you shouldn't see the marker in
> Mac at all.

I'm pretty sure Mano implemented this a year or so ago.

(In reply to comment #3)
> Is this only true on the trunk? I've seen the bidi caret many times, and the
> only keyboard layouts I have "set up" are French and English. I can certainly
> reproduce with current 1.8 branch builds, but it's possible that I'm
> remembering a problem that has since been fixed on the trunk.

Yeah, bug 288839 never landed on branch.

Comment 8

[Simon Montagu :smontagu]

See the summary in bug 434304:

* if your system supports RTL, then this is the expected behavior
* if your system does not support RTL and you see this in Firefox 2, then it's
a known problem
* if your system does not support RTL and you see this in Firefox 3, then it's
a new problem since this was supposed to have been fixed (and is indeed fixed
on Windows)

Uri, IIRC you told me once that there is no way to determine whether an RTL keyboard is installed on OSX (i.e. available in the input menu), so there may be no way to fix this there. Is my memory correct?

(In reply to comment #0)
> Is there any way we could only display directional text field cursors for RTL
> localizations?  Also, can text fields actually take RTL input if you are using
> a LTR localization?  If not, the visual affordance would be superfluous.

Text fields can certainly take RTL input in an LTR localization and vice versa. Localization language isn't the right criterion for enabling this.

Comment 9

[Uri Bernstein]

(In reply to comment #8)

> Uri, IIRC you told me once that there is no way to determine whether an RTL
> keyboard is installed on OSX (i.e. available in the input menu), so there may
> be no way to fix this there. Is my memory correct?

I don't know if it's correct, but it matches mine :-)

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > Uri, IIRC you told me once that there is no way to determine whether an RTL
> > keyboard is installed on OSX (i.e. available in the input menu), so there may
> > be no way to fix this there. Is my memory correct?
> 
> I don't know if it's correct, but it matches mine :-)

roc, karlt, Matthew Gregan and others have written a bunch of code recently to support testing keyboard stuff with different layouts and to work around problems with specific layouts in Cocoa widgets; I wonder if any of that code would be helpful here (I'm not sure what it's doing when it's dealing with specific layouts)?

Comment 11

[Simon Montagu :smontagu]

What do people think of only showing the directional caret when the pref bidi.browser.ui is set to true?

Comment 12

[Dotan Cohen]

(In reply to comment #11)
> What do people think of only showing the directional caret when the pref
> bidi.browser.ui is set to true?
> 

I'm fine with that suggestion, so long as it will not be used to sweep the issue under the carpet. If the issue is addressed, then I have no problem with single-directional users not seeing the directional caret.

Comment 13

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

On Windows I'm pretty sure we can detect whether you have an RTL layout installed.

Not sure about Mac or Linux, but I bet there is a way even if it involves grovelling through undocumented system internals.

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to comment #13)
> Not sure about Mac or Linux, but I bet there is a way even if it involves
> grovelling through undocumented system internals.

Linux is implemented through bug 348724.

Mac OS X 10.5 has:

http://developer.apple.com/documentation/TextFonts/Reference/TextInputSourcesReference/Reference/reference.html#//apple_ref/doc/c_ref/kTISPropertyInputSourceLanguages

http://developer.apple.com/documentation/TextFonts/Reference/TextInputSourcesReference/Reference/reference.html#//apple_ref/c/func/TISCreateInputSourceList

"includeAllInstalled
 Typically, set to false so that only enabled input sources are included"

With Keyboard Layout Services apis (before 10.5) I haven't found a way to find out which layouts are enabled (in the input menu).
GetScriptManagerVariable(smKeyScript) would provide the script of the currently selected layout only.  (I'm not sure whether that's useful.)

Comment 15

[Matthew Gregan [:kinetik]]

The smBidirect script manager variable sounds like it does what we want, but it always returns 0 (false) on my 10.5 system, even when a RTL layout is installed and in use.

The smScriptRight script variable also sounds useful (but would require querying each enabled script manually by first querying smScriptEnabled for each possible script--also, note that this defintion of enabled is not that it appears in the input menu), but also always returns 0 (false) on my system.  It's interesting to note that GetScriptManagerVariable(smKeyScript) returns 126 (smUnicode) when using a Hebrew layout, so it wouldn't be possible for the script manager to give a sensible result for an smScriptRight with this script ID anyway.  Additionally, querying smScriptRight for the smHebrew ID returns 0 anyway.

Comment 16

[Simon Montagu :smontagu]

Attachment: Patch

This does what I proposed in comment 11 and makes the presence of the bidi caret marker dependent on the bidi.browser.ui pref rather than the existence of RTL content on the current page.

The more I think about it the more I'm convinced that this is the Right Thing To Do: in localizations to RTL languages the pref is turned on by default, and I'm fairly sure that anyone not using such a localization who is doing enough bidi text entry that they want the caret marker also wants control of text input base direction, so will have turned on the pref anyway.

It will also mean that the marker doesn't come and go anymore: any given user will see it either in all contexts or none.

Comment 17

[Dotan Cohen]

Thank you Simon. I am currently unable to patch and test but I will get on that as soon as I can. Have a great weekend!

Comment 18

[Uri Bernstein]

Comment on attachment 345700 [details] [diff] [review]
Patch

bidi.browser.ui is defined in firefox.js, and this is core code. Isn't this a problem?
Also, the documentation on the preference should be updated to reflect this change.

Comment 19

[Simon Montagu :smontagu]

(In reply to comment #18)
> (From update of attachment 345700 [details] [diff] [review])
> bidi.browser.ui is defined in firefox.js, and this is core code. Isn't this a
> problem?

Hmmm. I can of course move it from firefox.js (and suite/browser/browser-prefs.js) into all.js, but I'm not sure what effect that will have in Thunderbird, where Bidi UI is currently implemented in an extension.

Comment 20

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

Comment on attachment 345700 [details] [diff] [review]
Patch

Please post an additional patch to move the pref to all.js

Comment 21

[Simon Montagu :smontagu]

Attachment: Move the pref - mozilla-central part

Comment 22

[Simon Montagu :smontagu]

Attachment: Move the pref - comm-central part

Comment 23

[Simon Montagu :smontagu]

Attachment: Set the pref in RTL system locales (browser)

Comment 24

[Simon Montagu :smontagu]

Attachment: Set the pref in RTL system locales (suite)

Comment 25

[Asaf Romano (gone)]

Comment on attachment 348824 [details] [diff] [review]
Set the pref in RTL system locales (browser)

I would rather do that in utilityOverlay (inside isBidiEnabled, you may want to rename that, I don't mind either way)

Comment 26

[[e-mail]]

Comment on attachment 348825 [details] [diff] [review]
Set the pref in RTL system locales (suite)

>   // BiDi UI
>   gShowBiDi = isBidiEnabled();
>   if (gShowBiDi) {
>+    pref.setBoolPref("browser.bidi.ui", true);
isBidiEnabled reads this pref, so you've got circular logic.

Depending on what you're trying to achieve, you may need to move some code into nsSuiteGlue.js to ensure that it gets run on startup.

Comment 27

[Simon Montagu :smontagu]

Attachment: Set the pref in RTL system locales (browser, v.2)

Comment 28

[Simon Montagu :smontagu]

Attachment: Set the pref in RTL system locales (suite v.2)

Comment 29

[[e-mail]]

Comment on attachment 349195 [details] [diff] [review]
Set the pref in RTL system locales (suite v.2)

>+  if (pref.getBoolPref("bidi.browser.ui", false))
No "default" value needed (are you thinking of browser's window.getBoolPref?)
r=me with that fixed.

Comment 30

[Asaf Romano (gone)]

Comment on attachment 348904 [details] [diff] [review]
Set the pref in RTL system locales (browser, v.2)

Please fix  setBoolPref to match browser/ code-style.

r=mano otherwise.

Comment 31

[Simon Montagu :smontagu]

Attachment: Set the pref in RTL system locales (browser, v.3)

Comment 32

[Simon Montagu :smontagu]

Attachment 345700 [details] [diff], attachment 348800 [details] [diff] [review] and attachment 352890 [details] [diff] [review] pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/68e3d56ef84c

Comment 33

[Simon Montagu :smontagu]

Attachment: Oops

Fixes a typo in the last attachment.

Comment 34

[Simon Montagu :smontagu]

Attachment 353385 [details] [diff] pushed as http://hg.mozilla.org/mozilla-central/rev/c1df222657c1

Comment 35

[Benjamin Smedberg]

This introduced a new warning:

layout/base/nsCaret.h:311: 'nsCaret::mLastHint' will be initialized after
layout/base/nsCaret.h:295:    'PRPackedBool nsCaret::mIgnoreUserModify'
layout/base/nsCaret.cpp:89:   when initialized here

Comment 36

[Shawn Wilsher :sdwilsh]

Simon - could you generate a branch patch for this that has your test fix and fixes the warning so we can get this in on 1.9.1?  We'd like to get this polish bug fixed there.

Comment 37

[Simon Montagu :smontagu]

Attachment: Branch patch

Combined diff of all the checkins here and bug 470574

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this new pref not imply a late-l10n change for locales that are bidi that will need to set the pref to true?

Comment 40

[Axel Hecht [:Pike]]

We could white-card adding the pref to firefox-l10n.js for RTL locales, but I'd prefer to know what it really does before doing that.

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

From what I can tell, if the pref is true (for RTL locales), then the text-insertion indicator will always show the extra pixel/notch on the top-right or top-left to indicate the direction of text input.

If the pref is false (for LTR locales), that notch will not appear.

Is that right?

Comment 42

[Simon Montagu :smontagu]

That is right, but incomplete. If the pref is true, we also add the "Switch Page Direction" and "Switch Text Direction" menu items. Because of that I had thought that all RTL locales would want those menu items and already set the pref, but I just searched in mxr and saw that they don't. Apparently they rely on the code in navigator.js that turns the pref on when the system locale is RTL.

Comment 43

[Axel Hecht [:Pike]]

To clarify, as that pref is set automagically, this is just going to work without a change to our localizations?

Comment 44

[(no longer active)]

What about the case of an RTL localized build being run on a system with an LTR locale?

Comment 45

[Simon Montagu :smontagu]

I think that this can work without a change to localizations. That is making the assumption that users who regularly edit bidirectional text will need the "Switch Text Direction" feature even more than they need the directional indicator on the caret, so they are likely to have turned the pref on manually if it hasn't already been set according to the system locale. I think it would be good if l10ns did set the pref anyway to cover the scenario Ehsan mentions in comment 44, but that doesn't need to block taking the code change.

However, since there will be a change of behaviour on LTR locales, we should mention it in a release note.

Comment 46

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368066 [details] [diff] [review]
Branch patch

a191=beltzner

Comment 47

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/96e4e7d6e5f1

Comment 48

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

This bug was only occasionally encountered, but was commonly reported (I even had people privately email me to ask what was causing the problem).