In bug 197253 comment 77: ------- Comment #77 From sairuh 2005-01-20 13:53 PDT [reply] ------- tested with 2005011808-trunk camino bits on 10.3.7. overall, this works, but found some issues --are they already filed? b. after trying the commands in the 2nd formatting toolbar (same sample url above), the caret (and focus) within the midas editing area is lost. need to hit Tab or reclick in the editing area again. No one ever replied to sairuh and so this bug was never filed. In Fx, all the toolbar buttons and selects preserve focus of the Midas area/selection. (Whee, another focus bug!)

This isn't the same as our "lose focus when tabbing through plugin" bug, is it?

(In reply to comment #1) > This isn't the same as our "lose focus when tabbing through plugin" bug, is it? I don't think so (but what bug is that?); this is about pushing buttons in Midas (which is not a plugin). And using selects, but they have focus issues already in Camino.

Remind me to look at this with Steven's focus patch.

And the answer is no, at least for everything except the colorpicker buttons.

Trunk FTW! This works correctly in 20080331

Really? In a build from last night, pressing a button doesn't eat the caret anymore, but it still sends focus somewhere else such that I have to click back into the midas area to start typing again.... Actually, I can move the caret with the cursor; I just can't type. Text/highlight color do work properly.

Wow, that's crazy. I was just looking at the highlighting and cursor; I didn't realize the current behavior could be possible.

Tweaking summary, and sending to core since today's Minefield has the same bug.

This isn't really a focus problem, as far as I can tell. But it does seem to be a Cocoa widgets problem (I can't reproduce it in today's Windows or Linux nightlies). Here's a clear description of the problems I saw with today's Minefield nightly: 1) Load the Midas demo URL (http://www.mozilla.org/editor/midasdemo/), click in the edit box (below the two menu bars), and type some text. 2) Click on the Bold button (or the Italic button or the Underline button). The text-input cursor still blinks in the correct location, and the arrow keys move it. But no new text gets entered as you type. And even more strangely, pressing the Backspace key triggers the browser's Back button! This isn't a recent regression. Before my focus patches (for bug 403232 and bug 404433) landed, the text-input cursor would stop blinking when you pressed any of these buttons. But the Backspace key still triggers the Back button in these earlier nightlies.

(Following up comment #9) > Backspace key I should have said "Delete key". (The keyboard I first tested with really does have a Backspace key -- it's an old Dell keyboard. But I've now retested with an Apple keyboard and got the same results.)

The Delete key wierdness no longer happens with today's nightly. Could have been fixed by the patch for bug 398514.

So for mail-client type folks (and probably HTML editor-type folks), this is a regression from the switch to Cocoa. For them, it's also not minor, either.

(In reply to comment #13) > So for mail-client type folks (and probably HTML editor-type folks), this is a > regression from the switch to Cocoa. For them, it's also not minor, either. For Thunderbird/SeaMonkey, anyone attempting to compose an HTML email on Mac will likely run across this. Steps to repeat: - Type some characters - Click the bold/italic/underline button - Try to type some more characters - nothing happens, and it is very hard to get the text bold or something without first typing your text and then going back and selecting it. This will be found easily - I expect it hasn't been in our releases so far because I expect that most of our nightly testers probably use plain text, or just haven't reported it. Although this bug is old, TB/SM worked fine on the 1.8 branch. I think it is only that we didn't ship off 1.9 that we haven't found it until now - we'd have most likely had some reports if we'd been shipping betas. Therefore requesting blocking as David, Dan and myself think this will be reasonably high profile for TB.

Here's a patch that fixes this bug without regressing bug 403232, bug 404433, bug 357535, bug 413882 or bug 418031. It also doesn't regress the (separate) problem reported in bug 413882 comment #77. I tested in Minefield, Shredder and Seamonkey, on OS X 10.5.4 and 10.4.11. I also tested a cvs variant of this patch in Camino (again on OS X 10.5.4 and 10.4.11). I surprised myself by finding that I could accomplish all this while substantially simplifying the previous code. I've done a lot of testing. But I'd really like to see others test, too. Marcia? :-) And whoever else you can persuade to participate from the QA team. The STRs of all the bugs I mentioned above should be tested. I'd also like to see someone who knows Google docs well (and particularly Google spreadsheets) bang away hard on this patch. A tryserver build (of Minefield) will follow in an hour or two.

Mark: Needless to say, I'll expect you to test this patch, too.

(In reply to comment #16) > Mark: Needless to say, I'll expect you to test this patch, too. I've done a short test on Thunderbird with this patch and it appears to be working correctly. I'll do some more testing tomorrow and maybe check out those other bugs you referenced (i.e. check they haven't regressed) if I get time.

Here's the tryserver build (of Minefield) made with this patch: https://build.mozilla.org/tryserver-builds/2008-09-25_13:40-smichaud@pobox.com-bugzilla314160/smichaud@pobox.com-bugzilla314160-firefox-try-mac.dmg

ere's a more precise list of things to test (STRs) to vet my patch for this bug (including some that I didn't mention before): A) Tests in Minefield/Firefox: 1) Bug 314160 comment #9. 2) Bug 399471 comment #0 and comment #2. 3) Bug 403232 comment #0. 4) Bug 404433 comment #0 (need a Gmail account). 5) Bug 408266 comment #0. This STR is already in litmus (bug 408266 comment #7). 6) Bug 413882 comment #18 (need a Gmail account). Note that the cell in step 2 needs to be above the "bar", and the cell in step 3 needs to be below the "bar". 7) Bug 413882 comment #25 (need a Gmail account). 8) Bug 413882 comment #77 and bug 413882 comment #80 (need a free vBulletin account). Here's a more precise STR: a) Visit http://www.vbulletin.com/forum/ and log in. b) Click on "vBulletin Suggestions and Feedback" (under "vBulletin 3.7"). c) Click on the "New Thread" button. d) Click in the message body and try typing. (For extra credit also fiddle with the formatting buttons and keep typing.) e) Log out without submitting your post :-) B) Tests in Thunderbird/Shredder 1) Bug 357535 comment #0. 2) Bug 418031 comment #0.

> 2) Bug 399471 comment #0 and comment #2. 2) Bug 399471 comment #0 and bug 399471 comment #2.

For what it's worth, here's a Shredder build (universal) that I did myself and uploaded to my people.mozilla.com account. It was built using code pulled the morning of 2008-09-22 -- plus my patch, of course. http://people.mozilla.com/~stmichaud/bmo/thunderbird-3.0b1pre.en-US.mac.bugzilla314160-smichaud.dmg

I've just tested all the bugs that don't require logins on my local built FF and TB (pulled today) with the patch applied. I couldn't see any regressions.

Thanks, Mark!

Comment on attachment 340418 [details] [diff] [review] Fix + ChildView *mShouldFocusView; // Strong Why did you choose to make this a strong reference instead of just clearing the value when a view gets deleted via the view's destructor or destroy call, something like that?

I can't make mShouldFocusView a weak pointer: This variable needs to be associated with a NSWindow object. But by the time [ChildView widgetDestroyed] is called the ChildView ("self") may already have been detached from its NSWindow. In this case it won't be possible to null-out this variable (still associated with the NSWindow to which "self" once belonged), and there's a real possibility that mShouldFocusView will become an invalid pointer (that it will point to a deleted object). But I've made other changes to neaten up my patch. In particular, I've added code to [TopLevelWindowData dealloc] to make sure mShouldFocusView is never leaked. And I've changed [TopLevelWindowData getShouldFocusView] to make sure it never returns an object that's been detached from its window (and that such an object is never made a first reponder in nsCocoaWindow_NSWindow_sendEvent:). A new tryserver build and Shredder build will follow in a while. I've tested my new patch (in my own builds) with all the STRs from comment #19, on both OS X 10.5.5 and 10.4.11.

Here's a tryserver build (of Firefox/Minefield) made with my rev1 patch: https://build.mozilla.org/tryserver-builds/2008-10-01_13:33-smichaud@pobox.com-bugzilla314160-rev1/smichaud@pobox.com-bugzilla314160-rev1-firefox-try-mac.dmg And here's a custom build of Thunderbird/Shredder made with my rev1 patch. As before, the base code was pulled on 2008-09-22. http://people.mozilla.com/~stmichaud/bmo/thunderbird-3.0b1pre.en-US.mac.bugzilla314160-rev1-smichaud.dmg

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.

Comment on attachment 341339 [details] [diff] [review] Fix rev1 (clean up) Vlad, do you want to sr this? Or should I ask someone else?

Comment on attachment 341339 [details] [diff] [review] Fix rev1 (clean up) This looks fine; sorry I missed this review request earlier.

Landed on mozilla-central (changeset 83cb01024590).

Landed before 1.9.1 branching: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/83cb01024590 Verified fixed on recent Shiretoko build: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090220 Shiretoko/3.1b3pre (Also confirmed present on Firefox 3.0 on Intel Mac.)

It looks like that it's worth having an automated test for this issue.

Automated testing of widget-level focus problems won't really be feasible until bug 470845 is resolved.

Ok, so it should depend on bug 470845. Means we don't miss it. Thanks Steven.