404433 - Text highlight in gmail broken on Mac

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

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

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.

Comment 1

[Martijn Wargers (dead)]

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?

Comment 2

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

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

Comment 3

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

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.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 6

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

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

Comment 7

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

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.

Comment 8

[Håkan Waara]

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

Comment 9

[Josh Aas]

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

Comment 10

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

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

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 12

[Steven Michaud [:smichaud] (Retired)]

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:[e-mail]-focus/

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 15

[Nick Kreeger]

(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:[e-mail]-focus/
> 

Comment 16

[Steven Michaud [:smichaud] (Retired)]

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

Comment 17

[Marcia Knous [:marcia]]

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:[e-mail]-focus/
> 

Comment 18

[Steven Michaud [:smichaud] (Retired)]

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

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #18)
> Fixed by patch for bug 403232, which just landed on trunk.

Reopening since that patch was backed out.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

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

Comment 22

[Steven Michaud [:smichaud] (Retired)]

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?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

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

Comment 24

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

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

Comment 25

[Steven Michaud [:smichaud] (Retired)]

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?

Comment 26

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

nsViewManager::HandleEvent should be useful.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

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

Comment 28

[Steven Michaud [:smichaud] (Retired)]

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

Comment 29

[Steven Michaud [:smichaud] (Retired)]

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

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Preliminary patch

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.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

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:[e-mail]-bugzilla404433/[e-mail]

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (tighten up)

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

Comment 33

[Steven Michaud [:smichaud] (Retired)]

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:[e-mail]-bugzilla404433-rev1/[e-mail]

Comment 34

[Josh Aas]

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.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 36

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

Any update on those tests I asked for?

Comment 37

[Steven Michaud [:smichaud] (Retired)]

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

Comment 38

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

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

Comment 39

[Josh Aas]

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

Comment 40

[Josh Aas]

Attachment: Fix rev1 (updated)

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

Comment 41

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 42

[Steven Michaud [:smichaud] (Retired)]

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:[e-mail]-bugzilla404433-rev1.1/[e-mail]

Comment 43

[Steven Michaud [:smichaud] (Retired)]

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

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (updated again)

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:[e-mail]-bugzilla404433-rev1.2/[e-mail]

Comment 45

[Jay Moran (jaym/lazvon)]

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

Comment 46

[Steven Michaud [:smichaud] (Retired)]

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

Comment 47

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

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

Comment 48

[Steven Michaud [:smichaud] (Retired)]

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

Comment 49

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

  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.

Comment 50

[Steven Michaud [:smichaud] (Retired)]

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

Comment 51

[Stuart Morgan]

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

Comment 52

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

> If not, we'll be in trouble :-)

If not, we'll just land your current approach.

And what Stuart said.

Comment 53

[Steven Michaud [:smichaud] (Retired)]

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

Comment 54

[Mike Schroepfer]

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

Comment 55

[Steven Michaud [:smichaud] (Retired)]

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 56

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

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

Okay.

Comment 57

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 58

[Steven Michaud [:smichaud] (Retired)]

Attachment: What I landed

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

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 62

[jhanks1981]

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.