This bug spawned off bug 388980, which fixed a similar problem. When you try to highlight text in gmail, you can't on a Mac builds, it works fine on other platforms. Steps to reproduce: 1. Open Minefield trunk on mac. 2. Go to gmail.com. 3. Click on the "Compose Mail" link. 4. Type text into the message body box. 5. Select the text you entered into the message box. 6. Click the high light text button on the formatting toolbar and select a color. 7. Observe the background color of your selected text does not change. This regressed between 2006-09-28-08 and 2006-09-29-06.

In that range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-28+08&maxdate=2006-09-29+06&cvsroot=%2Fcvsroot Bug 326469 was fixed. So my guess would be that this was caused by turning on Cocoa widgets. I guess a focus issue of some sort?

(In reply to comment #1) > In that range: > http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-28+08&maxdate=2006-09-29+06&cvsroot=%2Fcvsroot > Bug 326469 was fixed. So my guess would be that this was caused by turning on > Cocoa widgets. I guess a focus issue of some sort? > Yeah, I can't get head to build with the old mac widgets, which Roc says isn't surprising, and I've given up trying to get an old checkout to build. I will have to look at the cocoa changes preceding that I guess...

Oh, I tried rolling back a few of the other mac-ish things in the regression range, and it wasn't them, so the most likely avenue for investigation is the Cocoa widgets one.

This sounds similar to bug 357535 (and in both cases, it seems like perhaps some buttons are functional and some are not).

Oh, and if you're looking into changes in Widget:Cocoa before that became the default toolkit for Fx/Tb/etc, you should be able to check Camino trunk builds for a regression range (at least for this bug). I did a little poking before I found bug 388980, but I don't remember what I found....

Good tip, thanks Smokey! This regressed between 2006-09-20 22:00:00 and 2006-09-22 00:00:00 (the nightly builds on Camino on 2006-09-21 are only for 1.0-M1.8.0 and 1.1-M1.8, which I guess is not the main trunk, so I can't tell if its on that day. Regression range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-20+22%3A00%3A00&maxdate=2006-09-22+00%3A00%3A00&cvsroot=%2Fcvsroot Looking deeper...

After some binary-search and checking out lots of old builds, I've identified the *exact* checkin that caused this bug: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-21+09%3A21%3A00&maxdate=2006-09-21+09%3A25%3A00&cvsroot=%2Fcvsroot Which is bug 350018, attachment 239493 [details] [diff] [review]. This patches the translation of Cocoa coordinates to Gecko coordinates. So I guess there's something that still expects Cocoa coordinates which is now getting Gecko coordinates? Adding CC Håkan Waara, cos he checked in the patch that busted this. Håkan, any ideas? Component -> Widget:Cocoa.

Sounds like you could find out more by setting a breakpoint in nsChildView::WidgetToScreen() and looking at the callers when you select some text?

Chris, how do you know that patch is what broke this? I tried backing it out and the problem remained.

(In reply to comment #9) > Chris, how do you know that patch is what broke this? I tried backing it out > and the problem remained. > I've spent some time looking at this, and now I'm not convinced that patch did not cause this. I noticed that the hilight works if you select a color from the color-choser which does not lie over top of iframe/message body. So if you select a color whose box lies over the parent document, it works. The patch I thought broke this was causing the color chooser to be drawn mostly over the iframe, whereas before the patch, it was mostly drawn above the iframe, meaning the bug was less likely to occur with a casual click. So my mistake, sorry. What's happening is that when the mouse goes over the iframe, I think the mouse takes its coords relative to the origin of the iframe. I think Google's color-chooser is determining what color to hilight with by the mouse coords of the click - which is off by the x-offset of the iframe, so it's interpreting the click in the color-chooser as being outside of the color-chooser.

If I'm understanding Chris's last comment correctly, this reminds me a good bit of bug 365987 (although most of the manifestations of that bug we've seen have been branch-only)....

Oddly, this problem turns out to be a focus bug. It's fixed by my patch for bug 403232 (attachment 290742 [details] [diff] [review]). I've tested the patch (on the trunk) in both Minefield and Camino (both of which have the problem reported here). Here's a tryserver build of Minefield, made using that patch. (Unfortunately, the try servers can't (yet) build Camino.) https://build.mozilla.org/tryserver-builds/2007-11-29_13:05-smichaud@pobox.com-focus/

If Steven's patch is relatively simple and self-contained (and applies on or can reasonably be ported to the branch), we're probably interested in trying to take it on the MOZILLA_1_8_BRANCH, since Camino suffers from this bug there, since it's also a regression for us ("from" bug 350018 exposing this bug, or bug 403232, or whatever) on the branch, and since Gmail is a pretty high-profile site.

(In reply to comment #12) > Oddly, this problem turns out to be a focus bug. > > It's fixed by my patch for bug 403232 (attachment 290742 [details] [diff] [review]). I've > tested the patch (on the trunk) in both Minefield and Camino (both of > which have the problem reported here). > > Here's a tryserver build of Minefield, made using that patch. > (Unfortunately, the try servers can't (yet) build Camino.) > Steven, yay! This patch also fixes bug 357535.. Nice work on the patch. > https://build.mozilla.org/tryserver-builds/2007-11-29_13:05-smichaud@pobox.com-focus/ >

(In reply to comment #14) The context of this patch (in nsChildView.mm) is exactly the same on the 1.8 branch as it is on the trunk. So yes, I'd expect it also to work (and to fix bugs) on the branch (though I haven't yet tested it there). (In reply to comment #15) Very glad to hear it. And thanks!

Steven: Your tryserver build looks good running on Leopard - I can confirm the Gmail fix using that build. (In reply to comment #12) > Oddly, this problem turns out to be a focus bug. > > It's fixed by my patch for bug 403232 (attachment 290742 [details] [diff] [review]). I've > tested the patch (on the trunk) in both Minefield and Camino (both of > which have the problem reported here). > > Here's a tryserver build of Minefield, made using that patch. > (Unfortunately, the try servers can't (yet) build Camino.) > > https://build.mozilla.org/tryserver-builds/2007-11-29_13:05-smichaud@pobox.com-focus/ >

Fixed by patch for bug 403232, which just landed on trunk.

(In reply to comment #18) > Fixed by patch for bug 403232, which just landed on trunk. Reopening since that patch was backed out.

(Following up comment #18) The first time I landed a patch for bug 403232, I had to back it out because of Mochitest failures. I've just landed another version of the patch, which had no problems.

Bad news: It turns out this bug _isn't_ a focus bug, after all. And my patch for bug 413882 (which restores the correct focus-handling behavior for left-mouse-down events, attachment 301566 [details] [diff] [review]) will regress this bug. Instead, this bug is caused by some kind of Gecko design flaw, or perhaps by a bad interaction between how Gecko handles its nsChildView hierarchy and Cocoa widgets. In the test case for this bug (comment #0), notice that, when selecting the background color for your text, the operation works if the mouse comes down above the line that marks the top of the composition window. It only fails if the mouse comes down below this line. The problem is that the color-select box, the button you click on to open it, and everything under the color-select box above the top of the composition window are in nsChildView A. But the nsChildView object that gets focused (or should get focused) when you click anywhere in the composition window (including in that part of the color-select box above the composition window) is a "grandchild" of nsChildView A (call it nsChildView C). So when you click on the part of the color-select window above the composition window, the mouse-down doesn't go to nsChildView A (where the color-select box was drawn), but to nsChildView C. And, since (as of my patch for bug 413882) the focus now changes to nsChildView C on left-mouse-down, the results of (apparently) clicking on the color-select box get lost. Or to put it more simply: When you left-mouse-click on the part of the color-select box above the composition window (in nsChildView A), you're actually clicking on a different object (nsChildView C, underneath the color-select box in nsChildView A). The reason my patch for bug 403232 appeared to fix this bug is that it stopped any NS_GOTFOCUS or NS_LOSTFOCUS events being sent on left-mouse-down events. I haven't tested on Windows or Linux to see if they have the same nsChildView hierarchy -- though I presume they do. And I don't know why clicking on the part of the color-select box above the composition window works on Windows and Linux -- though this may have something to do with the fact that nsChildView objects are "windows" on Windows and Linux, while they're "views" (components of windows) on OS X. I can't think of an easy Cocoa-widgets-specific fix for this. Can anyone think of an easy tweak to Gecko that might help?

> Or to put it more simply: When you left-mouse-click on the part of > the color-select box ->above<- the composition window (in > nsChildView A), you're actually clicking on a different object > (nsChildView C, underneath the color-select box in nsChildView A). This should have been: When you left-mouse-click on the part of the color-select box ->below<- the ->top of the<- composition window (in nsChildView A), you're actually clicking on a different object (nsChildView C, underneath the color-select box in nsChildView A). Sorry.

The way this is supposed to work is that mouse events get routed to the correct Gecko nsIView and nsIFrame regardless of which native widget they come in on, so it doesn't really matter which native widget you click on, the only thing that matters is which Gecko content you click on. I guess the problem is that your patch is looking at which nsChildView the mouse events are happening on, and you're doing something when the nsChildView changes even though the nsIView/nsIFrame is the same (or at least closely related). You need to find a way to avoid that. (As you know, I never liked your fix for 403232, which caused 413882.)

Alright, which nsIView and/or nsIFrame methods should I hook (add printf() calls to) to see which nsIFrame/nsIView objects these events are getting sent to?

nsViewManager::HandleEvent should be useful.

(In continued reply to comment #24) > I guess the problem is that your patch is looking at which > nsChildView the mouse events are happening on, and you're doing > something when the nsChildView changes even though the > nsIView/nsIFrame is the same (or at least closely related). You need > to find a way to avoid that. (As you know, I never liked your fix > for 403232, which caused 413882.) My patch for bug 403232 can't, of course, cause this bug -- this bug existed before that patch was landed.

> and you're doing something when the nsChildView changes even though > the nsIView/nsIFrame is the same (or at least closely related) The only thing that changes with my patch for bug 413882 is that I'm now sending NS_LOSTFOCUS and NS_GOTFOCUS events, as appropriate, on a left-mouse-down event. I'm pretty sure that shouldn't change. I'll be digging through event handlers for focus and left-mouse-down events, to see what I can find. If I find bugs and they're easy to fix, I'll try to fix them.

> I'll be digging through event handlers for focus and left-mouse-down > events, to see what I can find. If I find bugs and they're easy to > fix, I'll try to fix them. Thanks to your comments, I know I should concentrate on nsIView, nsIFrame and nsIContent handlers. This should help narrow my search, and make it easier.

Roc, I actually followed your advice (sort of). In my tests I found that mouse-down events were (apparently) always going to the "right" nsIFrame, and that the "right" nsIContent always ended up getting focused (by Gecko). So I altered my patch for bug 413882 in the following two ways: 1) Now I only send NS_LOSTFOCUS and NS_GOTFOCUS events after a mouse-down if both the (OS-specific) "first reponder" and the Gecko focused content have changed. 2) Furthermore, I change the "first responder" back to what it used to be if oldFirstResponder is a ChildView and the Gecko focused content _hasn't_ changed. I find that doing this fixes this bug (bug 404433), bug 403232 and bug 357535 (all of which were regressed by my patch for bug 413882), without also regressing bug 413882.

Here's a Firefox tryserver build made with this patch (attachment 303930 [review]). Please test it and let us know your results. https://build.mozilla.org/tryserver-builds/2008-02-17_15:35-smichaud@pobox.com-bugzilla404433/smichaud@pobox.com-bugzilla404433-firefox-try-mac.dmg

We should only restore oldFirstResponder as first responder if it still has a widget and a Gecko element is still focused.

Here's a tryserver build made with my rev1 patch (attachment 304052 [details] [diff] [review]): https://build.mozilla.org/tryserver-builds/2008-02-18_08:56-smichaud@pobox.com-bugzilla404433-rev1/smichaud@pobox.com-bugzilla404433-rev1-firefox-try-mac.dmg

Comment on attachment 304052 [details] [diff] [review] Fix rev1 (tighten up) I want to understand why we have to muck around with focused content elements while other platforms (gtk2 and Windows) don't. I'm going to be really uncomfortable with this until I get such an explanation.

The short answer is that it works -- it fixes all outstanding focus bugs. I already gave a slightly longer answer in comment #30 above. As I've said all along, I don't fully know why my patches for bug 403232 and this bug work ... but it appears that they _do_ work, very well. As you know, focus issues are very complex. The debugging patch I use to look at focus stuff (which just adds printf statements to every focus-related method in the tree) is 500K, and touches around 100 files. I hope, post Firefox 3, to understand focus issues well enough to replace my patches for bug 403232 and this bug (bug 404433) with ones that address the fundamental issues (whatever they are) more directly. But for now these patches are the best we can do -- and they do very well.

Any update on those tests I asked for?

I'm about to start working on them. The one for bug 403232 should be easy. Those for bug 413882 and this bug (404433) _won't_ be easy (for obvious reasons), and will probably take me the rest of the week (or into next week).

OK, I'll take a mochitest for 403232 as down payment for the others and then I'll review this patch.

"Fix rev1 (tighten up)" no longer applies to trunk

Updated to current trunk, I wanted to use the patch so I updated it myself.

Thanks. I'll do another tryserver build in an hour or two. By the way, Martijn has opened bug 418745 and I've opened bug 418859 on problems I've had writing a mochitest for bug 403232. Martijn decided that a browser chrome test was more appropriate (since I need to emulate mouse-clicks outside of DOM), and is writing one himself. But what he has doesn't yet work right.

Here's a tryserver build made with the latest version of my patch (attachment 304828 [details] [diff] [review]): https://build.mozilla.org/tryserver-builds/2008-02-21_14:54-smichaud@pobox.com-bugzilla404433-rev1.1/smichaud@pobox.com-bugzilla404433-rev1.1-firefox-try-mac.dmg

(Following up comment #41) I've spent the last 4 or 5 days trying to write an automated test for bug 403232. I've discovered problems that will either make this impossible or very difficult. See bug 419466.

The tree has changed yet again, so here'a another updated version of rev1 of my patch. And here's a tryserver build made using it: https://build.mozilla.org/tryserver-builds/2008-02-25_07:59-smichaud@pobox.com-bugzilla404433-rev1.2/smichaud@pobox.com-bugzilla404433-rev1.2-firefox-try-mac.dmg

Steven, will attachment 304828 [details] [diff] [review] land without automated tests? The patch completely fixes my Select Text/Apply Formatting Buttons problems (Bug 418031 and Bug 404433) with Thunderbird which seems pretty major to me at least. Well, looks like it is now attachment 305539 [details] [diff] [review] while I've been typing, so will that one land without automated testing. :)

> will [my patch rev1.X] land without automated tests? I think that's the right thing to do. But keep testing! Automated tests, while useful, are like closing the barn door after the horse has bolted. The best way to find problems is to put my patch (via the tryserver builds) in the hands of a bunch of users, and have them bang away at it.

+ [newFirstResponder release]; Is this safe when newFirstResponder is nil? +nsresult GetFocusedElement(nsIDOMElement **aFocusedElement) Just return already_AddRefed<nsIDOMElement>. This is pretty gross. I think a better approach would be to expose nsViewManager::GetCurrentlyFocusedView somehow and just call that and watch for it to change. Although since views aren't refcounted, better to watch for the nsIViewManager to change, since you can hold a strong reference to it, if that's enough of a fix.

> + [newFirstResponder release]; > > Is this safe when newFirstResponder is nil? Yes. > Just return already_AddRefed<nsIDOMElement> So change nsresult GetFocusedElement(nsIDOMElement **aFocusedElement); to already_AddRefed<nsIDOMElement> GetFocusedElement(); ? > nsViewManager::GetCurrentlyFocusedView This wasn't in the tree when I wrote my patch, or I would probably have noticed it and tried to use it. But do we really gain anything by using it? In any case, if I do switch to using it, I'll have to redo all my tests ... and so will those who've been testing my tryserver builds.

already_AddRefed<nsIDOMElement> GetFocusedElement(); Right. What we gain by using the view manager stuff is that we don't have to go all the way to the window watcher. It's less code and more reliable because it's lower level. If you redo your tests, that will be enough.

> If you redo your tests, that will be enough. I'll write a new patch and start testing it tomorrow. This will include changes that make nsViewManager::GetCurrentlyFocusedView() public (presumably by adding it to nsIViewManager and changing that interface's UUID). If nsViewManager::GetCurrentlyFocusedView() is (more or less) functionally equivalent to what I'm doing now, we should be fine. If not, we'll be in trouble :-)

(In reply to comment #46) > Automated tests, while useful, are like closing the barn door > after the horse has bolted. A better analogy would be that automated testes are like closing the barn door after you've chased down the horse and put in back in the barn, so that you don't come out the next day and find that it's gone again. The recent history of the focus and event changes in Cocoa widgets makes it pretty clear that hand-testing is not sufficient to prevent regressions.

> If not, we'll be in trouble :-) If not, we'll just land your current approach. And what Stuart said.

(In reply to comment #51) Stuart, let me ask you a question? How many times have you landed patches that regressed existing bugs? And how many times did you write automated tests as a result of that?

(In reply to comment #53) > (In reply to comment #51) > > Stuart, let me ask you a question? > > How many times have you landed patches that regressed existing bugs? > And how many times did you write automated tests as a result of that? > What's the point of that question? Do you disagree that this has been a difficult area of code to get right (nothing personal there - just looking at the facts) so we should do everything in our power to make sure we monotonically increase in quality not just flap back and forth between different error states. Automated testing is not magic - it doesn't solve the problem but it helps you know you never have the same problem twice.

Roc, I've discovered that there are differences in behavior between my current patch's GetFocusedElement() and nsIViewManager::GetCurrentlyFocusedView(). Most importantly, I've never seen nsIViewManager::GetCurrentlyFocusedView() return NULL, while GetFocusedElement() often returns NULL. NULL testing of newFocusedElement is an important part of my patch (adding it in rev1 fixed a number of problems, including the one reported at bug 413882 comment #77). Now that we're going to have a beta5, there's a pretty obvious solution: We should land my current patch (rev 1.2, attachment 305549 [review]) more-or-less as is. Then I can have more time to test your suggestion (use nsIViewManager::GetCurrentlyFocusedView()) for possible inclusion in beta5.

Comment on attachment 305539 [details] [diff] [review] Fix rev1 (updated again) Okay.

Landed on trunk.

What I landed followed Roc's suggestion to change the return value and parameters of GetFocusedElement().

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4 ID:2008030317

I fail to see how Bug 466704 is a duplicate of this bug. Bug 466704 regarded the "word counter" and "download as" features of Googles word processor, not text hilighting in gmail.