To reproduce: 1. Open any page with a textarea/input (e.g. http://www.google.com) 2. Type a few spaces so that the caret is not at the edge of the textarea (this is needed due to bug 233348) 3. Switch input language to Hebrew Result: The caret switches to RTL, but a few pixels at the root of the BiDi marker are missing. 4. Switch input language back to to English Result: The caret switches to LTR, but a few pixels at the end of the BiDi marker display. By the way, the extra pixels are a very welcomed addition, it's actually an improvement over the regular BiDi caret, but obviously not one that's intended. This is a relatively recent regression, it's broken on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041017 But works fine on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a4) Gecko/20040925 Screenshot coming. Prog.

I narrowed it down. The first broken build is: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041013 http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/2004-10-13-06-trunk/mozilla-win32-installer.exe Prog.

This is _probably_ a regression from bug 261054. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-12+18%3A12&maxdate=2004-10-12+18%3A12&cvsroot=%2Fcvsroot Prognathous: can you check if changing the value of ui.caretWidth makes any difference?

This regression was most likely caused by Bug 261054. See https://bugzilla.mozilla.org/show_bug.cgi?id=261054#c12 CCing the assignee of that bug, as well as the reviewers. Prog.

CCing Aaron Leventhal, Daniel Glazman and rbs per last comment.

(In reply to comment #3) > Prognathous: can you check if changing the value of ui.caretWidth makes any > difference? Value of 1 seems to be the same as the default. Changing it to 2 solves the missing/extra pixels, but makes the caret too thick. Prog.

(from attachment 161213 [details] [diff] [review] in bug 261054) // If keyboard language is RTL, draw the hook on the left; if LTR, to the right hookRect.SetRect(caretRect.x + caretRect.width * ((bidiLevel) ? -1 : 1), - caretRect.y + caretRect.width, - caretRect.width, - caretRect.width); + caretRect.y + mBidiIndicatorTwipsSize, + mBidiIndicatorTwipsSize, mBidiIndicatorTwipsSize); This is wrong if mBidiIndicatorTwipsSize can be different from caretRect.width. In the LTR caret, the offset from caretRect.x to hookRect.x needs to be the width of the caret; in the RTL caret it needs to be the width of the hook. So the first line needs to become hookRect.SetRect(caretRect.x + (bidiLevel) ? caretRect.width : mBidiIndicatorTwipsSize,

By the way, since when has that condition been |(bidiLevel)| ? It should be (bidiLevel & 1), otherwise LTR text in an RTL field will have the hook in the wrong direction.

(In reply to comment #7) > hookRect.SetRect(caretRect.x + (bidiLevel) ? caretRect.width : > mBidiIndicatorTwipsSize, or rather, > hookRect.SetRect(caretRect.x + (bidiLevel & 1) ? caretRect.width : > mBidiIndicatorTwipsSize * -1,

Thanks Simon. I'll work on the regression.

Sigh. Let's see if I can get it right this time: hookRect.SetRect(caretRect.x + ((bidiLevel) ? mBidiIndicatorTwipsSize * -1 : caretRect.width),

I think this is a big improvement. I invite anyone who will benefit from this to help push this patch through. I'm pretty busy, so can someone else play around with the patch, improve the code, get it reviewed and checked in?

Comment on attachment 162800 [details] [diff] [review] Attractive trianglular indicator scales relative to caret size but also has min and max size Mike, ideally the patch would only have to deal with pixels, not twips. I understand that work may eventually happen. So, the code would have fewer conversions back and forth once we do that, making it much simpler. In general I'd say that this GetCaretRectAndInvert() method is spaghetti-like but I don't feel like rewriting it all and dealing with regressions.

Still looking for feedback on this patch -- does anyone feel it's important or works right? If we don't really need it we should mark it WONTFIX, since it does add some extra lines of code.

Comment on attachment 162800 [details] [diff] [review] Attractive trianglular indicator scales relative to caret size but also has min and max size Aaron, this is highly important. We're dealing with a major functionality that is very obviously broken. Definitley not something that should be wontfixed. To increase the chance of getting this patch reviewed, I'm tweaking the r and sr fields. Prog.

Comment on attachment 162800 [details] [diff] [review] Attractive trianglular indicator scales relative to caret size but also has min and max size I'm not an sr.

The patch is there for anyone who wants to drive it home.

Since this is marked as a blocker, I think we need to focus on restoring correct functionality and leave the attractive triangular indicator for after 1.8a5. This patch makes the change described in comment 11.

Comment on attachment 162800 [details] [diff] [review] Attractive trianglular indicator scales relative to caret size but also has min and max size Let's finish this of in bug 265064.

(In reply to comment #18) > Created an attachment (id=166381) > Minimal patch I think you accidentally posted the wrong patch. This one is for doing the triangle, and isn't minimal.

Thanks for picking that up, Aaron

Thanks for catching that, Aaron.

Comment on attachment 166438 [details] [diff] [review] Minimal patch r=mkaply

Comment on attachment 166438 [details] [diff] [review] Minimal patch r=, but get rid of the & 1 part of the bidiLevel condition. It's not necessary, because bidiLevel is a PRBool in this section of code. Perhaps change it to a different variable name to avoid confusion in the future, since bidiLevel is a bitfield elsewhere.

Comment on attachment 166438 [details] [diff] [review] Minimal patch r=aaronlev with the bidiLevel comment addressed.

This will have to wait until alpha6.

Comment on attachment 166438 [details] [diff] [review] Minimal patch moving sr request to rbs

Comment on attachment 166438 [details] [diff] [review] Minimal patch Care to attach a screenshot of what you see? This helps to speedup the review process of unusual code and gives something tangible to relate to.

Comment on attachment 166438 [details] [diff] [review] Minimal patch sr=rbs, with s/bidiLevel/isRTL/ as requested, to avoid the confusion that caught you too...

Fix checked in.

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