288839 - ltr caret in input field unexpected (directional caret appears when only ltr input methods are installed)

Status: VERIFIED FIXED

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

Description

[santiago.matamoros]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

In the german wikipedia the cursor in the input field looks strange.
The cursor seems to have a little dot on the left side, almost looking like a cross.
It occours at least in all versions of mozilla firefox from 0.5 to 1.0.1.
I was not able to reproduce it in IE or Opera. I never saw this on any other
site (including Wikipedia in other languages). 

The occurence of the effect seems to depend on the size of the html file.

See also
http://de.wikipedia.org/wiki/Wikipedia:Beobachtete_Fehler#Komischer_Cursor_in_Firefox
for a picture of the effect and a description in German.

Reproducible: Always

Steps to Reproduce:
1. Go to http://de.wikipedia.org/wiki/Hauptseite
2. Look at the input box on the left side.
3.

Actual Results:  
The cursor in the input box has a little dot on the left side, almost looking
like a cross.

Expected Results:  
Show the cursor as straight vertical bar.

It seems to depend on the size of the html page (not the css).

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050402
Firefox/1.0+
I'm seeing the same in the firefox URL bar & search bar.

Comment 2

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050402
Firefox/1.0+
But not with a new profile..

Comment 3

[Joona Palaste]

I'm seeing the little dot too, both in the German and the English Wikipedia. I'm
not seeing it anywhere else.

Comment 4

[Gary van der Merwe]

Moving to Core->Layout: Form Controls

Comment 5

[Gary van der Merwe]

The caret (cursor) you are seeing is to indicate the you are in left to right.
mode. https://bugzilla.mozilla.org/attachment.cgi?id=167412 shows what you would
see if you are in right to left mode.

I'm not sure when, and when not these directional carets are ment to be shown,
and why it's showing on wikipedia. (Maybe because they specify
margin-left-ltr-source, margin-left-rtl-source, margin-right-ltr-source, and
margin-right-rtl-source)

Change Component to BiDi, Block 265070, updated summary
 

Comment 6

[Hermann Schwab]

Attachment: reduced testcase, original code

Comment 7

[Hermann Schwab]

Attachment: minimized testcase

If I remove the hebrew text, the cursor remains normal.

  <div>
	  <div>
	    <form name="searchform" action="" id="searchform">
	      <input id="searchInput" name="search" accesskey="f" value=""
type="text">
	    </form>
	  </div>
	  
	  <div>
	    <ul>
	      <li><a href="http://he.wikipedia.org/wiki/">עברית</a></li>
	      <li><a href="http://hr.wikipedia.org/wiki/">Hrvatski</a></li>
	    </ul>
	  </div>
 </div>

Comment 8

[Hermann Schwab]

I didn´t check for other LTR languages, I just found that excluding the language
box brought back the cursor to normal, and then I started commenting half of the
<li>s, until I reduced it to two languages, one causing the bug, and the next
one, normal.
First testcase is original code from Wikipedia, second testcase contains no CSS.

Comment 9

[Hermann Schwab]

Attachment: minimal testcase arab language

previous testcase uses multiple divs, this one has only one:

 <div>
  <input id="searchInput" name="search" accesskey="f" value="" type="text">
  <a href="http://als.wikipedia.org/wiki/">Alemannisch</a>
  <a
href="http://ar.wikipedia.org/wiki/">&#1575;&#1604;&#1593;&#1585;&#1576;&#1610;&#1577;</a>

 </div>

Comment 10

[Simon Montagu :smontagu]

Mano, did we discuss somewhere never showing the directional caret if there is
no RTL keyboard installed?

Comment 11

[Asaf Romano (gone)]

Not in particular, but we did agree we should have a "IsBiDiCaret" or such in
nsIBidiKeyboard in order to fix bug 272096, I guess an installed rtl keyboard
could be another condition.

Comment 12

[Eyal Rozenberg]

Mano/smontagu: So, like, are you confirming this bug, marking it as a dupe or what?

Comment 13

[Prognathous]

Confirming bug. Users who don't have an RTL input method installed shouldn't
encounter the RTL caret. On any platform.

The other side of the coin is that BiDi users should never encounter a
non-directional caret. Simon, should this be a new bug?

Prog.

Comment 14

[Simon Montagu :smontagu]

(In reply to comment #13)

> The other side of the coin is that BiDi users should never encounter a
> non-directional caret. Simon, should this be a new bug?

What do you mean by "Bidi users" in this context? Users with a Bidi
localization? Users with a Bidi locale? Users with an RTL input method installed?

Comment 15

[Prognathous]

By "BiDi users" (in this context), I mean "Users with an RTL input method
installed". 

To make the interface as consistent and as expected as possible, the types of
carets should be kept to the minimum that's essential. Currently, non-BiDi users
sometimes see directional carets which they don't understand, and BiDi users
sometimes see directionless caret which is inconsistent ("why is it LTR on this
page and directionless on that page?").

Prog.

Comment 16

[Simon Montagu :smontagu]

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

Comment 17

[Hermann Schwab]

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

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Adam Guthrie]

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

Comment 20

[Richard Shewmaker]

Site does not have to be in a foreign language for the cursor artifact to appear.

Check http://www.adobe.com/ and http://en.wikipedia.org/wiki/Main_Page

Comment 21

[Simon Montagu :smontagu]

(In reply to comment #20)
> Site does not have to be in a foreign language for the cursor artifact to
> appear.
> 
> Check http://www.adobe.com/ and http://en.wikipedia.org/wiki/Main_Page

I'm not sure what "in a foreign language" means in this context. The bidirectional cursor appears in documents that contain either intrinsic right-to-left content (e.g. Arabic or Hebrew) or elements with right-to-left directionality (e.g. with direction: rtl in CSS).
http://en.wikipedia.org/wiki/Main_Page contains links to wikipedias in several dozen different languages, several of which are right-to-left. 
http://www.adobe.com has an element with direction: rtl (it's empty and I don't know why it's there, but it is).

Comment 22

[Simon Montagu :smontagu]

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

Comment 23

[Simon Montagu :smontagu]

Attachment: Windows patch

Comment 24

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

   PRBool mDefaultsSet;
+  PRBool mHaveBidiKeyboards;

Make these PRPackedBool

+  // If there is not at least one keyboard of each directionality, return
+  // without setting any values. Bidi keyboard functionality will be disabled.
+  mHaveBidiKeyboards = (isRTLKeyboardSet && isLTRKeyboardSet);
+  if (!mHaveBidiKeyboards)
+    return NS_OK;

Why are we disabling Bidi keyboard functionality if there's only an RTL keyboard? Actually I'm not sure what it means for there to be only an RTL keyboard, maybe I'm not the best reviewer for this code.

+  if (!mDefaultsSet) {
+    nsresult result = EnumerateKeyboards();
+    if (NS_SUCCEEDED(result))
+      mDefaultsSet = PR_TRUE;
+    else
+      return result;
+  }
+  if (!mHaveBidiKeyboards)
+    return NS_ERROR_FAILURE;
+

Move this code into EnumerateKeyboards instead of duplicating it.

Comment 25

[Simon Montagu :smontagu]

(In reply to comment #24)
> Why are we disabling Bidi keyboard functionality if there's only an RTL
> keyboard? Actually I'm not sure what it means for there to be only an RTL
> keyboard, maybe I'm not the best reviewer for this code.

In theory I could have e.g. only Arabic Persian and Urdu keyboards installed and I would never enter left-to-right text. In practice I doubt that this ever occurs, but I thought it made more sense to cover the case.

> +  if (!mDefaultsSet) {
> +    nsresult result = EnumerateKeyboards();
> +    if (NS_SUCCEEDED(result))
> +      mDefaultsSet = PR_TRUE;
> +    else
> +      return result;
> +  }
> +  if (!mHaveBidiKeyboards)
> +    return NS_ERROR_FAILURE;
> +
> 
> Move this code into EnumerateKeyboards instead of duplicating it.
 
Umm, apart from setting mDefaultsSet, what can I move into EnumerateKeyboards? I still have to test mHaveBidiKeyboards even when mDefaultsSet is true.

Comment 26

[Simon Montagu :smontagu]

Oops, forgot to cc roc before responding to review comments

Comment 27

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

In EnumerateKeyboards, which you might want to call something else, you could have

  if (mDefaultsSet) {
    return mHaveBidiKeyboards ? NS_OK : NS_ERROR_FAILURE;
  ...
  mDefaultsSet = PR_TRUE;
  return mHaveBidiKeyboards ? NS_OK : NS_ERROR_FAILURE;  

then at the callers you just need
  nsresult rv = EnumerateKeyboards();
  if (NS_FAILED(rv))
    return rv;

Comment 28

[Simon Montagu :smontagu]

Attachment: addressed review comments

I also added a change to IsRTLLanguage that I have been meaning to make for some time and which is possible now that 9x isn't supported.

Comment 29

[Simon Montagu :smontagu]

(In reply to comment #13)
> The other side of the coin is that BiDi users should never encounter a
> non-directional caret. Simon, should this be a new bug?

Better make it a new bug, since there doesn't seem to be consensus that it's the right thing to do. The fix would be very easy, basically just removing the marked lines in 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCaret.cpp&rev=1.160&mark=1091-1098#1078
and tidying up, but right now that would mean problems on Mac.

Comment 30

[Simon Montagu :smontagu]

Checked in.

Comment 31

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060724 Minefield/3.0a1. 

I no longer see the bidi caret in any of the testcases or on the Wikipedia page itself.

Comment 32

[Simon Montagu :smontagu]

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

Comment 33

[Hermann Schwab]

(In reply to comment #28)
> Created an attachment (id=229071) [edit]
> addressed review comments
> 
> I also added a change to IsRTLLanguage that I have been meaning to make for
> some time and which is possible now that 9x isn't supported.

This means the patch can't be used for branch, as branch still supports Win9x ? 

Comment 34

[Simon Montagu :smontagu]

Without the changes to nsBidiKeyboard::IsRTLLanguage(), it should be fine for branch.

Comment 35

[Ryan Flint [:rflint] (ping via IRC for reviews)]

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

Comment 36

[Hermann Schwab]

(In reply to comment #34)
> Without the changes to nsBidiKeyboard::IsRTLLanguage(), it should be fine for
> branch.

Could you please fix it in the branch?

Comment 37

[Simon Montagu :smontagu]

Attachment: Branch patch

This also includes a few lines from bug 272096, which are essential to make it work.

This is well-baked on trunk and low-risk, and many users complain about the Bidi caret indicator.

Comment 38

[Hermann Schwab]

(In reply to comment #34)
> Without the changes to nsBidiKeyboard::IsRTLLanguage(), it should be fine for
> branch.
> 

Isn't this breaking win98 compatibility for branch in patch Attachment 237977 [details] [diff]?

 NS_IMETHODIMP nsBidiKeyboard::IsLangRTL(PRBool *aIsRTL)
 {
   *aIsRTL = PR_FALSE;
+
+  nsresult result = SetupBidiKeyboards();
+  if (NS_FAILED(result))
+    return result;

Comment 39

[Simon Montagu :smontagu]

(In reply to comment #38)

> Isn't this breaking win98 compatibility for branch in patch Attachment 237977 [details] [diff] [edit]?

Unless I am missing something, the only non-win98 compatible part of the original patch (attachment 229071 [details] [diff] [review]) was the call to ::GetLocaleInfoW(), which is not in this patch. It has been tested on German W2K with VC6.

Comment 40

[Hermann Schwab]

(In reply to comment #39)
> (In reply to comment #38)
> 
> > Isn't this breaking win98 compatibility for branch in patch Attachment 237977 [details] [diff] [edit] [edit]?
> 
> Unless I am missing something, the only non-win98 compatible part of the
> original patch (attachment 229071 [details] [diff] [review] [edit]) was the call to ::GetLocaleInfoW(), which is
> not in this patch. It has been tested on German W2K with VC6.

Did you read my question in comment 33 and your answer in comment 34 regarding win98 compatibility of the patch attached in comment 28?

If you can give me a download address I can test on win98.

Comment 41

[Mike Schroepfer]

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

minusing the patch for drivers.   Concerned about risk and insufficient testing coverage this late in the release.

Comment 42

[Simon Montagu :smontagu]

(In reply to comment #40)
> Did you read my question in comment 33 and your answer in comment 34 regarding
> win98 compatibility of the patch attached in comment 28?

I still don't see the connection. IsLangRTL() and IsRTLLanguage() are two different things.

Comment 43

[Hermann Schwab]

(In reply to comment #42)
> (In reply to comment #40)
> > Did you read my question in comment 33 and your answer in comment 34 regarding
> > win98 compatibility of the patch attached in comment 28?
> 
> I still don't see the connection. IsLangRTL() and IsRTLLanguage() are two
> different things.

o.k., you're right, as I often have difficulties (in my own language) unconsciously picking the word 'right' or 'left', I didn't see the difference between IsLangRTL() and IsRTLLang...() comparing text in three tabs. Imho that's collateral damage from promgramming and debugging 80286 Assembler, often looking at hex numbers. You are used to swapping the nibbles automatically. Got to read slower, to not miss the end of the word, and threetimes.

Comment 44

[Maxim Radugin]

 NS_IMETHODIMP nsBidiKeyboard::IsLangRTL(PRBool *aIsRTL)
 {
   *aIsRTL = PR_FALSE; <-- is it safe? or it should be checked against NULL first?

Comment 45

[Simon Montagu :smontagu]

(In reply to comment #44)
>  NS_IMETHODIMP nsBidiKeyboard::IsLangRTL(PRBool *aIsRTL)
>  {
>    *aIsRTL = PR_FALSE; <-- is it safe? or it should be checked against NULL
> first?

Please file a new bug

Comment 46

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

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

Comment 48

[Simon Montagu :smontagu]

(In reply to comment #13)
> The other side of the coin is that BiDi users should never encounter a
> non-directional caret. Simon, should this be a new bug?

This is now happening in bug 418513.