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
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.
Created attachment 390117 [details] [diff] [review] 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.)
Created attachment 390118 [details] 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).
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.
Here's a tryserver build made from my debugging patch from comment #7: https://email@example.com/bugzilla504450-debug-macosx.dmg
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.
Created attachment 390471 [details] [diff] [review] 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.
Here's a tryserver build made with my patch from comment #15: https://firstname.lastname@example.org/bugzilla504450-macosx.dmg
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+
Comment on attachment 390471 [details] [diff] [review] Cocoa-widget-specific fix Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/262500972428
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
Last Resolved: 9 years ago
Resolution: --- → FIXED
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
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  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.  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
Last Resolved: 9 years ago → 9 years ago
Resolution: --- → FIXED
Verified FIXED -- thanks for your work on this, Steven.
Status: RESOLVED → VERIFIED
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.
status1.9.2: --- → beta1-fixed
You need to log in before you can comment on or make changes to this bug.