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

I can't reproduce this (on Mac, right now). Make sure you don't have caret browsing enabled?

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.

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

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

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

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

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.

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

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.

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.

> The problem is that <input> nsIContent's have their editable flag set For buttons, that's actually a little odd...

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.

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?

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

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

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

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

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 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 on attachment 308728 [details] [diff] [review] Patch v2 - with Peterv's suggested changes a1.9=beltzner

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

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

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

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.