Closed Bug 504450 Opened 15 years ago Closed 15 years ago

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

Categories

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

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: stephend, Assigned: smichaud)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Stack trace of focusing a hidden nsChildView
3.67 KB, text/plain
Details
Cocoa-widget-specific fix
1.14 KB, patch
jaas
: review+
Details | Diff | Splinter Review 
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
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?
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.
I'll need to dig into this to answer your question, Neil.

I should be able to get to that sometime next week.
OS: All → Mac OS X
Hardware: x86 → All
Flags: in-testsuite?
Blocks: 178324
Flags: blocking1.9.2?
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"
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
> 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.
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.
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.)
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).
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.
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!
I tested this out and couldn't find any problems with it.
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?
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.
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.
Assignee: nobody → smichaud
Attachment #390117 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #390471 - Flags: review?(joshmoz)
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.
nvm, I still see this using today's trunk build, but in other cases (for which, specifically, I haven't yet figured out).
Josh - ETA on review? This is a Firefox 3.6a1 blocker ...
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.
Enn: can you review this or is it more of a Josh-only thing?
-> P2, don't think this needs to block the alpha.
Priority: P1 → P2
Whiteboard: [needs review]
Comment on attachment 390471 [details] [diff] [review]
Cocoa-widget-specific fix

Definitely run this through try servers to look for test failures.
Attachment #390471 - Flags: review?(joshmoz) → review+
Whiteboard: [needs review]
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.2
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)...
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.
(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.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
(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.
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
> 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.
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Verified FIXED -- thanks for your work on this, Steven.
Status: RESOLVED → VERIFIED
Depends on: 511449
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.
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: