421169 - Flashing cursor appears next to programmatically focused buttons

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Duncan Loveday]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030506 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030506 Minefield/3.0b5pre

When focus is set on a button programmatically, for example using "document.getElementById('theButton').focus();" a flashing cursor appears next to the button.

Reproducible: Always

Steps to Reproduce:
1. Open the attached test case
2.
3.
Actual Results:  
A flashing cursor appears

Expected Results:  
No cursor should be visible

Regression since beta 3

Comment 1

[Duncan Loveday]

Attachment: test case

Comment 2

[Boris Zbarsky [:bzbarsky]]

I can't reproduce this (on Mac, right now).

Make sure you don't have caret browsing enabled?

Comment 3

[Duncan Loveday]

Caret browsing....what's that and how do I check if it's enabled ? I haven't changed any settings between beta 3 and the current trunk.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Apparently crowder can reproduce on his Mac...  This needs a regression range.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Duncan, if you don't get a blinking caret just clicking on text in a web page, you don't have caret browsing enabled.

Comment 6

[Duncan Loveday]

My relevant settings from about:config are

accessibility.browsewithcaret;false
accessibility.warn_on_browsewithcaret;true
bidi.edit.caret_movement_style;2
layout.selection.caret_style;0

I'll have a go at the regression range now, watch this space...

Comment 7

[Duncan Loveday]

Regression is between

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008-02-26-04-trunk/firefox-3.0b4pre.en-US.win32.installer.exe

and

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008-02-27-04-trunk/firefox-3.0b4pre.en-US.win32.installer.exe

Comment 8

[Boris Zbarsky [:bzbarsky]]

Bonsai range:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-26+02&maxdate=2008-02-27+06&cvsroot=%2Fcvsroot

Gut feeling is that this is one of roc's changes (the overflow rect one?) or _maybe_ Mats.  confirming since crowder saw this.

Comment 9

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

I can reproduce this. I suspect 394473 or possibly 406596. Chris, can you check by backing out your patch for 394473 locally?

Comment 10

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

Damn, this is caused by bug 394473. That's two regressions that I know of. We should make some reftests for the caret. What we could do is add a function on nsICaret, or a user pref or something, that disables caret blinking, making the caret a solid unblinking line. We can then make reftests that test the unblinking caret.

Comment 12

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

Attachment: Patch v1

The problem is that <input> nsIContent's have their editable flag set, so the test in nsEventStateManager::ResetBrowseWithCaret() (which is called on prehandle focus event) would assume that the <input> represents a contentEditable, and so display the caret.

This patch makes the following changes to ResetBrowseWithCaret():
* We now set mBrowseWithCaret before bailing out if we're in !contentEditableDoc rather than after. This means that if we turn off caret browsing in designMode, it actually is turned off. The caret stays visible, since it's designMode, but if you browser to another page, caret browsing won't be on.
* Previously we'd bail out if we're in an editable non-contentEditable document, so as to prevent the caret being made invisibile in designMode. Now we also bail out if we're in a contentEditableDoc and we've focused a contentEditable node. This means we won't turn off the caret if we disable caret-browsing while we've focused a CE element.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> The problem is that <input> nsIContent's have their editable flag set

For buttons, that's actually a little odd...

Comment 14

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

Yeah, I tried with checkbox and textfield input types, and they're editable (so the bug repoduces with them too), so I assume that something's making all input's editable.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah, sounds like it.  We should fix that too (possibly as a separate bug).  Should be easy to test via matching of the :-moz-read-write selectors etc, right?

Comment 16

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

Attachment: Test: which input types are -moz-read-write

(In reply to comment #15)
> Yeah, sounds like it.  We should fix that too (possibly as a separate bug). 
> Should be easy to test via matching of the :-moz-read-write selectors etc,
> right?
> 

You mean like this?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Somewhat like that, yes, but since we disallow the styling of backgrounds on some inputs and don't draw them on others, I'd use something like ":-moz-read-write + span" as the selector and "<input><span>Text</span>" as the markup.  Presumably the read-write inputs should be the ones whose state can change....

Comment 18

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

Attachment: Test v2: which input types are -moz-read-write

(In reply to comment #17)
> Somewhat like that, yes, but since we disallow the styling of backgrounds on
> some inputs and don't draw them on others, I'd use something like
> ":-moz-read-write + span" as the selector and "<input><span>Text</span>" as the
> markup.  Presumably the read-write inputs should be the ones whose state can
> change....
> 

Yeah, that works pretty well. On a related note, when the inputs are actually editable, it would be nice if we could allow editing on the inputs' values, like IE7 does, e.g. allow you to edit the text displayed on a button inline. Not a critical feature at this stage though.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 307864 [details] [diff] [review]
Patch v1

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================

>+      PRBool isContentEditableDoc =
>+        doc ? (doc->GetEditingState() == nsIHTMLDocument::eContentEditable) 
>+            : PR_FALSE;

I'd set isContentEditableDoc to |doc && doc->GetEditingState() == nsIHTMLDocument::eContentEditable|

>+
>+      PRBool isFocusContentEditable =
>+        isContentEditableDoc && 
>+        ((mCurrentFocus) ? mCurrentFocus->HasFlag(NODE_IS_EDITABLE) : PR_FALSE);

Same here, mCurrentFocus && mCurrentFocus->HasFlag(NODE_IS_EDITABLE)
You don't need to check isContentEditableDoc here, you already check it in the if below.

>+
>+      if (!isContentEditableDoc || isFocusContentEditable)

Comment 21

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

Attachment: Patch v2 - with Peterv's suggested changes

As patch v1 with PeterV's suggested changes. r=peterv.

Comment 22

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

Comment on attachment 308728 [details] [diff] [review]
Patch v2 - with Peterv's suggested changes

You can get r+sr from peterv in one shot, normally.

This code would really benefit from moving to a model where "should I show the caret?" is a test in one place and we just have an Invalidate-style method to show or hide the caret whenever that might be needed.

Comment 23

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

Comment on attachment 308728 [details] [diff] [review]
Patch v2 - with Peterv's suggested changes

Requesting approval 1.9, since this is an embarrassing and regression which causes the caret to appear in odd places.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 308728 [details] [diff] [review]
Patch v2 - with Peterv's suggested changes

a1.9=beltzner

Comment 25

[Reed Loden [:reed]]

Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.733; previous revision: 1.732
done

Comment 26

[Duncan Loveday]

We spell focussed with a double s here in Old Blighty you know

Comment 27

[Reed Loden [:reed]]

(In reply to comment #26)
> We spell focussed with a double s here in Old Blighty you know

;)

Comment 28

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040704 Minefield/3.0pre as well as the Win XP nightly. I used the testcase attached to the bug to verify.