483564 - caret is invisible in rtl text input boxes

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Karl Tomlinson (:karlt)]

Attachment: testcase

In the attached testcase the top text input is with <div dir="rtl">, while the bottom text input is ltr.  The caret is visible only in the ltr input.  In the rtl input, the caret is invisible, but editing seems to function correctly.

Comment 1

[Simon Montagu :smontagu]

Do you mean that the caret is not visible at all, even when it is located in the middle of the input field, or that it disappears off the edge in certain situations?

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
Not visible at all. i.e. not visible at any time after clicking in the rtl input, while moving the caret from one end of the text to the other.

Comment 3

[Simon Montagu :smontagu]

I can't reproduce that in a Linux trunk build from a couple of hours ago, although I do notice that the caret blinks slowly and irregularly in any text field, and often disappears for a few seconds at a time.

Comment 4

[Karl Tomlinson (:karlt)]

I can see selections but haven't seen a cursor in the rtl input box under any situation, with a x86_64/Linux build from m-c fbbc3d6c9f31 (a few days ago) or an official i686 build of Firefox 3.1b3.  Firefox 3.0.7 behaves as expected.  I may be able to get a regression range tomorrow.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

On 32bit Linux m-c 92a6bdc7da98 (from March 10), I don't see a caret in the rtl div. Karl: that's on mystery BTW.

With yesterday's m-c 7208e221c109 [Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090316 Minefield/3.2a1pre (.NET CLR 3.5.30729)] I don't see a caret in the rtl div when the caret is at the far left position. Otherwise I see the caret as expected on Windows. 

I don't think the windows and linux bugs are the same. The windows bug regressed between 2006-11-01-04 and 2006-11-02-04, I'm not sure about the Linux bug. It could be related to bug 394473, about a year ago that caused a number of regressions similar to this.

Comment 6

[Karl Tomlinson (:karlt)]

Regression from ba1a2dc688af, bug 118308.
The caret becomes visible in the input box at http://www.google.co.il/ when Latin letters are typed.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Spun off bug 483733 for the hidden caret bug on Windows.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: adjust gtk_draw_insertion_cursor call

gtk_draw_insertion_cursor gets the location from its argument but ignores the width.

Mozilla draws cursors to either side of the location in the text depending on the text direction.  The rects (including clip rect) passed from nsCaret::PaintCaret to nsITheme::DrawWidgetBackground try to tell the theme precisely where to draw, but the theme (like Mozilla) actually decides where to draw based on the location and direction.  For the default GTK theme and Clearlooks atleast, (at least part of) the cursor seems to fall on either side of the location (according to the direction).

Comment 9

[Michael Ventnor]

Why can't you just change rect->x directly?

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> Why can't you just change rect->x directly?

That would happen to work fine because ThemeRenderer::NativeDraw creates its own copies for rect and cliprect.

But the main reason is that I don't like functions having unexpected side effects.  We don't really seem to use "const" enough so that we can interpret the lack of "const" as an indicator that something can be modified.  If moz_gtk_widget_paint were to modify the data referenced by its arguments (which look like input arguments), then its API docs should clearly state so, and state that rect and cliprect must not point at the same rectangle data.

The cost of a rectangle copy seems smaller than the cost of the caller needing to be careful how the function is used.

Comment 11

[Michael Ventnor]

(In reply to comment #10)
> The cost of a rectangle copy seems smaller than the cost of the caller needing
> to be careful how the function is used.

Yes, but then there's the issue of code size and consistency. I think we modify the rect in other parts of the file, and I don't foresee a reason for this GdkRectangle to be used elsewhere in the code. We don't have to make this more complicated than it needs to be.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: alternative patch to overwrite the cursor bounds with the insertion location

(In reply to comment #11)
> I think we modify the rect in other parts of the file

We already modify cliprect.  With this patch we now modify rect too.
Some const qualifiers attempt to clarify what might get modified where.

Comment 13

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

Comment on attachment 367724 [details] [diff] [review]
adjust gtk_draw_insertion_cursor call

Adjusting the rect locally is the right thing to do here. Seriously, functions, especially functions that are supposed to just paint stuff, should not modify their arguments.

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/34643058b5bb

Comment 15

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cf82f8168501

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED using testcase on builds:


Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre ID:20090722031710

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2pre) Gecko/20090722 Shiretoko/3.5.2pre ID:20090722032036