458373 - Caret not visible in textfields

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Fabrice Desré]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080909174204 Minefield/3.1b1pre
Build Identifier: 

There is no blinking caret in textfields.

Reproducible: Always

Steps to Reproduce:
1. Open any page with a text input field (eg www.google.com)
2. Focus the textfield (its background become yellow)
3. 
Actual Results:  
You can enter text, but no caret appears. 

Expected Results:  
The caret should be visible. Without the caret, it's very difficult to position the insertion point again.

Comment 3

[Stuart Parmenter]

we need to hook up the show carret flag in drawWindow to pass through to the drawing code.

roc: can you take a look at this -- you seemed to know what thing needed to be passed through where.

Comment 4

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

Attachment: patch

Try this.

Comment 7

[Martijn Wargers (dead)]

Why would one not the caret to be drawn with drawWindow? If the caret is visible, it should be drawn, no?

Comment 8

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

For many uses you don't want the caret to draw. reftests are a big one. Other people taking snapshots of Web pages probably don't want the caret either. The caret's a bit like the mouse cursor; it's more part of the UI than part of the page.

Comment 9

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

But I agree there are cases where you need the caret. So providing a flag is the way to go.

Comment 10

[Martijn Wargers (dead)]

For reftests, you do want the caret to draw. Currently, there is no way to reftests whether a caret is there.

Comment 11

[Martijn Wargers (dead)]

And fwiw, I don't agree with the people taking snapshots of Web pages argument, either. If you don't want the caret to be there, just make it invisible by setting ui.caretWidth to 0 or something.

Comment 12

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

(In reply to comment #10)
> For reftests, you do want the caret to draw. Currently, there is no way to
> reftests whether a caret is there.

No you don't, because you don't want a test to fail just because it blinked in the test but not in the reference.

(In reply to comment #11)
> And fwiw, I don't agree with the people taking snapshots of Web pages argument,
> either. If you don't want the caret to be there, just make it invisible by
> setting ui.caretWidth to 0 or something.

That's an option but it's too obscure IMHO. And currently reftests, for example, don't depend on setting specific non-default pref settings.

Comment 13

[Martijn Wargers (dead)]

(In reply to comment #12)
> (In reply to comment #10)
> > For reftests, you do want the caret to draw. Currently, there is no way to
> > reftests whether a caret is there.
> 
> No you don't, because you don't want a test to fail just because it blinked in
> the test but not in the reference.

The caret can be made non-blinking by using setCaretReadOnly of nsISelectionController.

> (In reply to comment #11)
> > And fwiw, I don't agree with the people taking snapshots of Web pages argument,
> > either. If you don't want the caret to be there, just make it invisible by
> > setting ui.caretWidth to 0 or something.
> 
> That's an option but it's too obscure IMHO. And currently reftests, for
> example, don't depend on setting specific non-default pref settings.

Ok, setCaretEnabled(false) on nsISelectionController would be the way to go then, I think.

Comment 14

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

Making sure it's always set to false in all selection controllers would be a real pain --- there's one per document, and also one per textbox.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #13)
> > That's an option but it's too obscure IMHO. And currently reftests, for
> > example, don't depend on setting specific non-default pref settings.
> 
> Ok, setCaretEnabled(false) on nsISelectionController would be the way to go
> then, I think.

I wouldn't want an extension, trying to thumbnail a page, to modify the actual
cursor setting. This patch allows the image to be captured without changing any
of the actual behavior of the browser.

Comment 16

[Martijn Wargers (dead)]

Well, I just disagree with every point made in comment 13. A caret is just like a scrollbar, as I see it.

(In reply to comment #15)
> I wouldn't want an extension, trying to thumbnail a page, to modify the actual
> cursor setting. This patch allows the image to be captured without changing any
> of the actual behavior of the browser.

The patch also allows the image to be captured with changing the actual behavior of the browser.

Comment 17

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

(In reply to comment #16)
> Well, I just disagree with every point made in comment 13.

Which comment? You made comment 13 :-).

> A caret is just like a scrollbar, as I see it.

Like I said, I think it's much more like a mouse cursor. When the window isn't focused, the caret isn't there. It never affects the layout of the page. Its position is almost entirely under the user's control.

> The patch also allows the image to be captured with changing the actual
> behavior of the browser.

I'm not sure what you mean. The patch allows the caller of drawWindow to decide whether they want the caret, without affecting any other users of drawWindow or any other parts of the browser.

Comment 18

[Martijn Wargers (dead)]

(In reply to comment #17)
> (In reply to comment #16)
> > Well, I just disagree with every point made in comment 13.
> 
> Which comment? You made comment 13 :-).

Oops! Probably that means it's time to shut up.

> I'm not sure what you mean. The patch allows the caller of drawWindow to decide
> whether they want the caret, without affecting any other users of drawWindow or
> any other parts of the browser.

Sorry, I wasn't aware of that. That basically addresses my concern.
Could those flags perhaps be documented at:
http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/canvas/nsIDOMCanvasRenderingContext2D.idl#165
?

Comment 19

[Martijn Wargers (dead)]

ARgh, never mind, they are already documented.

Comment 20

[Stuart Parmenter]

Comment on attachment 351110 [details] [diff] [review]
patch

this works for me.

Comment 21

[Stuart Parmenter]

Attachment: pass in drawcaret flag in fennec

Comment 22

[Stuart Parmenter]

pushed the fennec changes, just need roc to land the layout ones

Comment 23

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

Feel free to land the layout patch

Comment 24

[Stuart Parmenter]

checked in

Comment 25

[Aakash Desai [:aakashd]]

verified FIXED On builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2a2pre) Gecko/20090921 Fennec/1.0a3

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2a2pre) Gecko/20090921
Fennec/1.0b4pre