Bug 27105 - Blur and focus events are not fired when window is blurred and focused
Summary: Blur and focus events are not fired when window is blurred and focused
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords: HasReduction, InRadar
: 4814 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-07-08 17:34 PDT by Erik Arvidsson
Modified: 2010-12-07 20:52 PST (History)
9 users (show)

See Also:


Attachments
Test case (374 bytes, text/html)
2009-07-08 17:34 PDT, Erik Arvidsson
no flags Details
Test case (1.38 KB, text/html)
2009-09-04 13:06 PDT, Erik Arvidsson
no flags Details
Patch v1 (7.00 KB, patch)
2009-09-04 18:36 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Fixes Eric's comments (7.00 KB, patch)
2009-09-08 16:56 PDT, Erik Arvidsson
arv: review-
Details | Formatted Diff | Diff
Fixes Eric's comments (5.62 KB, patch)
2009-09-08 16:56 PDT, Erik Arvidsson
arv: review-
Details | Formatted Diff | Diff
All comments resolved. (7.34 KB, patch)
2009-09-08 18:37 PDT, Erik Arvidsson
arv: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Fixed iframe case (9.65 KB, patch)
2009-09-09 18:34 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2009-07-08 17:34:36 PDT
Created attachment 32492 [details]
Test case

Given an element that can be focused and has focus and blur event listeners. When the user tabs to the element or clicks on it the focus event is fired. Likewise, the blur event is fired when tabbing out or clicking elsewhere. The focus rectangle correctly reflects this. However, if i focus the element and switch to another window no focus nor blur events are fired.

The test prints the events being fired. Notice that no events are fired when the user switches between windows.
Comment 1 Mark Rowe (bdash) 2009-07-08 19:59:19 PDT
<rdar://problem/7043685>
Comment 2 Erik Arvidsson 2009-07-09 09:44:51 PDT
Also, ctrl-tab to switch window tabs shows the same behavior.
Comment 3 Erik Arvidsson 2009-09-04 13:06:53 PDT
Created attachment 39087 [details]
Test case

Updated test case to include window events and targets
Comment 4 Erik Arvidsson 2009-09-04 18:36:54 PDT
Created attachment 39108 [details]
Patch v1
Comment 5 Eric Seidel (no email) 2009-09-08 14:49:59 PDT
Comment on attachment 39108 [details]
Patch v1

Arv mentioned in-person that he wanted to update this.

changes which were discussed
shouldBeEqualToString (nit)
function for shorter js test (nit)
removal of "focus me" from bottom of test (nit)
comment in dispatchEventsOnWindowAndFocusedNode to document intended behavior.

In general this looks fine to me.
Comment 6 Erik Arvidsson 2009-09-08 16:56:43 PDT
Created attachment 39228 [details]
Fixes Eric's comments


---
 6 files changed, 118 insertions(+), 4 deletions(-)
Comment 7 Erik Arvidsson 2009-09-08 16:56:47 PDT
Created attachment 39229 [details]
Fixes Eric's comments


---
 5 files changed, 38 insertions(+), 42 deletions(-)
Comment 8 Erik Arvidsson 2009-09-08 17:01:51 PDT
Sorry about the spam. My git+bugzilla-tool fu failed
Comment 9 Erik Arvidsson 2009-09-08 18:37:32 PDT
Created attachment 39240 [details]
All comments resolved.
Comment 10 Eric Seidel (no email) 2009-09-09 08:08:48 PDT
Comment on attachment 39240 [details]
All comments resolved.

OK.
Comment 11 Eric Seidel (no email) 2009-09-09 08:19:30 PDT
Comment on attachment 39240 [details]
All comments resolved.

Rejecting patch 39240 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 12 Eric Seidel (no email) 2009-09-09 09:37:48 PDT
fast/events/tabindex-focus-blur-all.html -> failed
Seems we'll need new expected results.  Meaning unless someone would like to fix this manually on landing we'll need a new patch. :)
Comment 13 Erik Arvidsson 2009-09-09 11:13:43 PDT
I'll work on a new patch.
Comment 14 Erik Arvidsson 2009-09-09 18:34:14 PDT
Created attachment 39318 [details]
Fixed iframe case

The last patch had real failures in it which was caught be the submit queue.

This patch handles iframes correctly and I extended the test case to cover that.
Comment 15 Eric Seidel (no email) 2009-09-09 18:42:40 PDT
Comment on attachment 39318 [details]
Fixed iframe case

Why does the blur count need to change?  And why is the changed value correct?
Comment 16 Erik Arvidsson 2009-09-10 08:54:51 PDT
(In reply to comment #15)
> (From update of attachment 39318 [details])
> Why does the blur count need to change?  And why is the changed value correct?

The blur count did not actually change. I just fixed the hard coded text to reflect the correct count. The expected count changed in a previous patch but the test was not updated back then.

-329 focus / 329 blur events dispatched, and should be 335 / 335 PASSED
+329 focus / 329 blur events dispatched, and should be 329 / 329 PASSED

I can take this file out of the patch if you prefer?
Comment 17 Eric Seidel (no email) 2009-09-10 09:38:19 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 39318 [details] [details])
> > Why does the blur count need to change?  And why is the changed value correct?
> 
> The blur count did not actually change. I just fixed the hard coded text to
> reflect the correct count. The expected count changed in a previous patch but
> the test was not updated back then.
> 
> -329 focus / 329 blur events dispatched, and should be 335 / 335 PASSED
> +329 focus / 329 blur events dispatched, and should be 329 / 329 PASSED
> 
> I can take this file out of the patch if you prefer?

Oh, so there is no functional change to the test, you're just correcting a typo?
Comment 18 Eric Seidel (no email) 2009-09-10 09:39:07 PDT
Comment on attachment 39318 [details]
Fixed iframe case

Makes sense.
Comment 19 WebKit Commit Bot 2009-09-10 10:03:59 PDT
Comment on attachment 39318 [details]
Fixed iframe case

Clearing flags on attachment: 39318

Committed r48257: <http://trac.webkit.org/changeset/48257>
Comment 20 WebKit Commit Bot 2009-09-10 10:04:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Simon Fraser (smfr) 2009-11-23 13:31:39 PST
This regressed clicking on autocompletion lists on windows: <rdar://problem/7391640>
Comment 22 John Sullivan 2009-12-23 10:39:17 PST
This also caused a regression in which textfield callbacks are sent to editing clients: <https://bugs.webkit.org/show_bug.cgi?id=32904>
Comment 23 Jon Honeycutt 2010-04-22 19:02:40 PDT
This led to a regression with onfocus handlers that call alert(). <rdar://problem/7880378>
Comment 24 Erik Arvidsson 2010-04-22 19:32:27 PDT
In general I think this is a bug in the web page but I agree that we need to defend against this case.

Here is what IE or Firefox do:

- IE does not dispatch the blur event
  Cons: The UI might not be correct because it did not receive the event

- Firefox dispatches the blur event but they do not restore focus so no second focus event is dispatched.
  Cons: The element does not get focus back so people using the keyboard will have to tab back to the element.

Other options include not showing the alert in a focus event which of course is also strange. 

Personally I think the best solution is to do what Chrome does and add a checkbox to the alert allowing the user to prevent more popups to be shown.
Comment 25 Adele Peterson 2010-04-22 23:54:45 PDT
Maybe there is an obvious answer here - but how was the original change an improvement?  Should we consider reverting this change?
Comment 26 Erik Arvidsson 2010-04-23 09:29:13 PDT
This fixes real world bugs [*].

Lets not revert a correct behavior due to a handful incorrect sites.

If this is causing iloops in high traffic sites a temporary revert on the release branch might be the correct answer until we solve the iloop alert problem. FWIW, Chrome has had this fix for a long time and has no reports of issues with this.  It might be due to it having a checkbox in the alerts to prevent all alert iloops.

[*] Almost all Google Apps rely on focus and blur events and one way the original bug manifests itself can be seen through the following steps:

1. Go to gmail
2. Focus the search field and type "is:"
3. This shows an auto complete popup
4. Focus another window

Expected result

The auto complete popup should be hidden.

On trunk this works but on Safari 4 the popup is not hidden when the element loses focus.
Comment 27 Alexey Proskuryakov 2010-06-11 15:01:40 PDT
*** Bug 4814 has been marked as a duplicate of this bug. ***