264903 - Directional caret marker is not drawn correctly (missing pixels in RTL, extra pixels in LTR)

Status: RESOLVED FIXED

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

Description

[Prognathous]

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.

Comment 1

[Prognathous]

Attachment: Magnified view of the broken carets

Comment 2

[Prognathous]

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.

Comment 3

[Asaf Romano (gone)]

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?

Comment 4

[Prognathous]

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.

Comment 5

[Asaf Romano (gone)]

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

Comment 6

[Prognathous]

(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.
 

Comment 7

[Simon Montagu :smontagu]

(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,

Comment 8

[Simon Montagu :smontagu]

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.

Comment 9

[Simon Montagu :smontagu]

(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,

Comment 10

[Aaron Leventhal]

Thanks Simon. I'll work on the regression.

Comment 11

[Simon Montagu :smontagu]

Sigh. Let's see if I can get it right this time:

 hookRect.SetRect(caretRect.x + ((bidiLevel) ? mBidiIndicatorTwipsSize * -1 :
                                               caretRect.width),

Comment 12

[Aaron Leventhal]

Attachment: Attractive trianglular indicator scales relative to caret size but also has min and max size

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 13

[Aaron Leventhal]

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.

Comment 14

[Aaron Leventhal]

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 15

[Prognathous]

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 16

[Mike Kaply [:mkaply]]

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.

Comment 17

[Aaron Leventhal]

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

Comment 18

[Simon Montagu :smontagu]

Attachment: Minimal patch

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 19

[Simon Montagu :smontagu]

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.

Comment 20

[Aaron Leventhal]

(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.

Comment 21

[Simon Montagu :smontagu]

Attachment: Minimal patch

Thanks for picking that up, Aaron

Comment 22

[Simon Montagu :smontagu]

Attachment: Minimal patch

Thanks for catching that, Aaron.

Comment 23

[Mike Kaply [:mkaply]]

Comment on attachment 166438 [details] [diff] [review]
Minimal patch

r=mkaply

Comment 24

[Aaron Leventhal]

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 25

[Aaron Leventhal]

Comment on attachment 166438 [details] [diff] [review]
Minimal patch

r=aaronlev with the bidiLevel comment addressed.

Comment 26

[Asa Dotzler [:asa]]

This will have to wait until alpha6.

Comment 27

[Asaf Romano (gone)]

Comment on attachment 166438 [details] [diff] [review]
Minimal patch

moving sr request to rbs

Comment 28

[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 29

[Simon Montagu :smontagu]

Attachment: before/after screenshots

Comment 30

[rbs]

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...

Comment 31

[Simon Montagu :smontagu]

Fix checked in.

Comment 32

[(no longer active)]

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