504450 - Have to set explicit focus to a page by clicking on it, after clicking Back

Status: VERIFIED FIXED

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

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Not sure if this is a Cocoa widget bug or just a general focus bug, sorry.

Build ID: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090715 Minefield/3.6a1pre

Steps to Reproduce:

1. Load http://hg.mozilla.org/mozilla-central/
2. Click on any bug link
3. Click the Back button
4. Now, hover over any link

Expected Results:

On hover, you should see the cursor-pointer turn into a hand

Actual Results:

You have to make an explicit click _before_ being able to click on page elements

Comment 1

[Neil Deakin]

It seems that just removing the implementation of nsChildView::SetFocus makes this work, but am worried that this might cause regressions.

With the new focus code, key events can be redirected to the right place as long as the active window receives them. Maybe we don't need to change the firstResponder at all on Mac? Is there ever a case where we need to change it?

Comment 2

[Neil Deakin]

Just to clarify the problem here, after going back whether by the back button or pressing the shortcut key, after doing everything, a call to resignFirstResponder is being made. I don't know what it calling this, or what the firstResponder is being changed to.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I'll need to dig into this to answer your question, Neil.

I should be able to get to that sometime next week.

Comment 4

[Benjamin Smedberg]

How involved is the fix for this going to be? Marking P1 for now to make sure we don't get caught taking a large landing "late"

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> How involved is the fix for this going to be?

I don't think anybody yet knows.

I'll get to this as soon as I can -- later today or tomorrow.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Not surprisingly, the regression range for this bug is:

firefox-2009-06-10-03-mozilla-central
firefox-2009-06-11-03-mozilla-central

Which implicates the patch for bug 178324.

But I've made some headway on this -- I'll post a debugging patch in
my next comment.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch and potential fix

If you run Minefield in gdb and break on '-[NSResponder
becomeFirstResponder]' and '-[NSResponder resignFirstResponder]', you
find that both can be called from nsChildView::Show() (via [NSView
setHidden:]), from nsChildView::SetFocus() (via [NSWindow
makeFirstResponder:]) and from [NSWindow sendEvent:] (where the event
is a mouse-down event).

With this debugging patch, you can also see that
nsChildView::SetFocus() can be called on an nsChildView object that's
"hidden" (on which nsChildView::Show(PR_FALSE) has been called, or on
which nsChildView::Show(PR_TRUE) hasn't yet been called).

This doesn't seem kosher to me -- maybe this is a Gecko bug.

Furthermore, this bug (bug 504450) goes away if you stop
nsChildView::SetFocus() from having any effect if nsChildView is
currently hidden.  My debugging patch also demonstrates this.

In a pinch, we might be able to get away with using a non-debugging
version of my patch to "fix" this problem.  It's less drastic than
simply getting rid of nsChildView::SetFocus().  But I think we should
first try to find deeper causes for this bug.

Neil? :-)

(A tryserver build will follow in a few hours.)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack trace of focusing a hidden nsChildView

The call to nsChildView::SetFocus() on a hidden object happens at step
3 of the STR from comment #0.  You can get a stack trace of this by
doing a debug build with my patch (or an opt build with debug
symbols), running it in gdb, and breaking on 'focus_hidden_view_break'.

Here's an example (made using an opt build with debug symbols).

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Something I forgot to mention above:

My debugging patch not only "fixes" this bug -- it also greatly
reduces the number of calls to '-[NSResponder becomeFirstResponder]'
and '-[NSResponder resignFirstResponder]' that happen at step 3 in
comment #0's STR.  I take this as another sign that I'm moving in the
right direction.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made from my debugging patch from comment #7:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla504450-debug/bugzilla504450-debug-macosx.dmg

Comment 11

[Neil Deakin]

We had a similar problem in bug 497565 where nsCocoaWindow::SetFocus was doing work when the window wasn't visible, so I feel confident that this could be the either the right fix or will lead to the right fix as well. Thanks for looking into this, I'll try it out tomorrow!

Comment 12

[Neil Deakin]

I tested this out and couldn't find any problems with it.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

So Neil, do you think it's worthwhile trying to fix this in
cross-platform code (as I suggested above)?  In other words, does it
make sense to try to stop nsChildView::SetFocus() (or
nsCocoaWindow::SetFocus()) from ever being called on an invisible
widget?

Or should we deal with this in Cocoa widgets code (as you've already
done for bug 497565)?  That is, should we just land a non-debug
version of my patch from comment #7?

Comment 14

[Neil Deakin]

I think a version of this latest patch should be ok. I'll look at other platforms though and see if this would be a problem on them, and we can re-evaluate then.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Attachment: Cocoa-widget-specific fix

Here's a non-debug version of my patch from comment #7.

I went through all the focus tests at bug 314160 comment #19 and had
no problems.

If/when Neil comes up with a more fundamental fix, it will obsolete my
patch.  But in the meantime let's get the ball rolling on my patch.

Another tryserver build will follow in a few hours.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #15:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla504450/bugzilla504450-macosx.dmg

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Did anything land which might have fixed this on trunk?  I'm using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090726 Minefield/3.6a1pre and having a hard time reproducing this bug so far.

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

nvm, I still see this using today's trunk build, but in other cases (for which, specifically, I haven't yet figured out).

Comment 19

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

Josh - ETA on review? This is a Firefox 3.6a1 blocker ...

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

New testcase that reproduces the bug for me 100% on trunk:

1. Load http://www.google.com/search?q=form+focus+html&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:official&client=firefox-a
2. Click on "Focus and TabIndex in HTML Forms" (http://www.coteindustries.com/articles/html_articles/tabindex.html).

Now try hovering over links.

Comment 21

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

Enn: can you review this or is it more of a Josh-only thing?

Comment 22

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

-> P2, don't think this needs to block the alpha.

Comment 23

[Josh Aas]

Comment on attachment 390471 [details] [diff] [review]
Cocoa-widget-specific fix

Definitely run this through try servers to look for test failures.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 390471 [details] [diff] [review]
Cocoa-widget-specific fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/262500972428

Comment 25

[Steven Michaud [:smichaud] (Retired)]

I ran tests locally and on the tryservers, and had no problems.

I'll land the patch on the 1.9.2 branch once I've pulled a local copy.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 390471 [details] [diff] [review]
Cocoa-widget-specific fix

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/94ac2bb6335a

Comment 27

[Daniel Holbert [:dholbert]]

Looks like this bug's patch may be responsible for this failure:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1250195035.1250200642.31009.gz#err0
OS X 10.5.2 mozilla-central unit test on 2009/08/13 13:23:55

> 29337 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_seek.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred: [ [object ProgressEvent] ]

I don't think that's a known intermittent failure, and it seems like it might be focus-related (i.e. related to this bug's patch)...

Comment 28

[Steven Michaud [:smichaud] (Retired)]

It's very unlikely my patch caused this failure.

Just before I landed it, I ran all tests locally and on the
tryservers, and had no problems.  And the mochitests are all still
passing on the 1.9.2 branch, where my patch also landed.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to comment #27)
> I don't think that's a known intermittent failure

Ah, turns out it's a known intermittent failure after all -- bug 503623.  I've now updated that bug's summary to make it more discoverable.

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

Sadly, I still can reproduce using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090814 Namoroka/3.6a2pre and trunk, on the STR I listed in comment 20 -- don't know what happened; guess I didn't test comment 20 with the tryserver build, or else something else landed in the interim?

Comment 31

[Steven Michaud [:smichaud] (Retired)]

I strongly suspect the STR in comment #20 is a different bug from what
you first reported.  And no, my patch doesn't seem to fix it.

Unfortunately the original STR is no longer reproducible in nightlies
from just before my patch was landed.

I think the lesson here is that simple patches shouldn't take so very
long to get landed.

Next week I'll look more carefully at the STR from comment #20, and if
need be open a new bug on it (and reclose this one).

Comment 32

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #20)

> 2. Click on "Focus and TabIndex in HTML Forms"
>    (http://www.coteindustries.com/articles/html_articles/tabindex.html).

This step is by itself enough to reproduce the new bug.	 The bug is
that the mouse-cursor remains a	hand no	matter where you hover.

But note that you can't move the mouse while the link is loading -- if
you move it enough (while the original page is still loaded) to	change
the mouse-cursor from a hand to a pointer, the bug doesn't happen.

Which must be some kind of clue.  I'll get on it next week.

Comment 33

[Markus Stange [:mstange]]

Steven, may I suggest that updating and reviewing my patch in bug 443178 might be a better use of your time?

The issue in this bug seems to be that mouse move events are not sent to the right view. The Cocoa event documentation [1] says "Mouse-moved events (type NSMouseMoved) are always sent to the first responder, not to the view under the mouse.", which explains why your patch might work in some cases.

I think the right approach is to just retarget every mouse move event to the view under the mouse. That's what ensureCorrectMouseEventTarget does when a popup is open, and I think we should do it for all mouse events, even when no popup is open. And that's exactly what my patch in bug 443178 does.

[1] http://developer.apple.com/documentation/Cocoa/Conceptual/EventOverview/EventArchitecture/EventArchitecture.html

Comment 34

[Steven Michaud [:smichaud] (Retired)]

> The issue in this bug seems to be that mouse move events are not
> sent to the right view

That wasn't the issue in (or rather behind) the original STR (from
comment #0) -- which is shown by the fact that my patch fixed it.

I also doubt it's the issue in the "new bug" (as represented by the
STR from comment #20) -- there I suspect a problem with the mouse
cursor not changing when it should.  And this STR is partially
reproducible on Windows (I'll talk more about that next week).

> Steven, may I suggest that updating and reviewing my patch in bug
> 443178 might be a better use of your time?

But I *will* take another look at your patch for bug 443178.  It's
certainly not a "simple patch".  But I've been meaning to deal with it
for a long time, and (frankly) Josh has kept bumping it off my list.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

I've spun off the STR from comment #32 and comment #20 into a new bug
-- bug 510919.  For various reasons (not least the difference in
regression ranges) it's clear bug 510919 is different from (and
probably unrelated to) this bug, so I'm closing this bug again.

Comment 36

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED -- thanks for your work on this, Steven.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

I've just discovered that my patch for this bug caused bug 511449.

As mentioned above, this bug's STR (from comment #0) stopped being
reproducible sometime before the patch was landed.  I've discovered
this happened during the following range:

firefox-2009-07-23-03-mozilla-central
firefox-2009-07-24-03-mozilla-central

And with hg bisect I've found that Neil's patch for bug 504367
(http://hg.mozilla.org/mozilla-central/rev/c8a66d96c1a1) was what
triggered this change.

Neil, do you think that patch could have fixed this bug?  Or do you
think the fact that it stopped comment #0's STR from being
reproducible was just an accidental side effect?

Of course, the simplest way to fix bug 511449 is to backout my patch
for bug 504450.  But if your patch for bug 504367 didn't fix this bug,
we might need to come up with an alternative patch.

Comment 38

[Neil Deakin]

(In reply to comment #37)
> Neil, do you think that patch could have fixed this bug?  Or do you
> think the fact that it stopped comment #0's STR from being
> reproducible was just an accidental side effect?

It could have fixed it yes, but only because some redundancy was removed by that patch. Other code could be added or another listener added which recreates this bug again. I think the patch is ok, but there is just something more that is needed to make it correct.