Last Comment Bug 687787 - Add support for DOM3 focusin/focusout
: Add support for DOM3 focusin/focusout
Status: NEW
btpp-active
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
: -- normal with 54 votes (vote)
: ---
Assigned To: Kevin Wern
:
Mentors:
: 95022 689869 (view as bug list)
Depends on: 855741
Blocks: 371728 968240 826916 934525
  Show dependency treegraph
 
Reported: 2011-09-19 23:58 PDT by Olli Pettay [:smaug]
Modified: 2016-06-23 02:11 PDT (History)
53 users (show)
kevin.m.wern: needinfo? (bugs)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

MozReview Requests
Submitter Diff Reviews Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
testcase (1.16 KB, text/html)
2011-09-28 01:32 PDT, bugzilla33
no flags Details
add DOM3 focusin/focusout support (8.48 KB, patch)
2013-05-18 01:34 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
focus, focusin, focusout event order test (2.84 KB, text/html)
2013-06-04 01:47 PDT, Alfredo Yang (:alfredo)
no flags Details
iframe focusin, focusout, focus test case (2.91 KB, text/html)
2013-06-04 23:50 PDT, Alfredo Yang (:alfredo)
no flags Details
add focusin and focusout event (17.84 KB, patch)
2013-06-09 21:41 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
focusin, focusout mochitest (4.96 KB, patch)
2013-06-12 23:03 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
Bug 687787 - Support focusin/focusout events (58 bytes, text/x-review-board-request)
2016-03-20 04:59 PDT, Kevin Wern
no flags Details | Review
Mochitest cases (logs to console) (17.36 KB, text/html)
2016-05-14 13:00 PDT, Kevin Wern
no flags Details
Mochitest cases (logs to console), fixed incorrect label (17.36 KB, text/html)
2016-05-16 00:48 PDT, Kevin Wern
no flags Details

Description Olli Pettay [:smaug] 2011-09-19 23:58:40 PDT
.
Comment 1 Boris Zbarsky [:bz] 2011-09-28 01:03:11 PDT
*** Bug 689869 has been marked as a duplicate of this bug. ***
Comment 2 bugzilla33 2011-09-28 01:32:59 PDT
Created attachment 563001 [details]
testcase
Comment 3 bugzilla33 2012-01-03 14:46:56 PST Comment hidden (spam)
Comment 4 Olli Pettay [:smaug] 2012-01-03 21:25:51 PST
(In reply to bugzilla33 from comment #3)
> Even IE.
Eh, focusin/focusout are originally from IE.

> When it will be in FF?
When I or someone else has time to implement these events.
Patches welcome ;)
Comment 5 phoang 2012-01-28 12:56:30 PST
Anybody have the ideas how to implement it. Our team is going to take this work.
Comment 6 Olli Pettay [:smaug] 2012-01-28 14:06:50 PST
Look at nsFocusManager. It is the one which dispatches focus and blur events.
When implementing this, be careful to not fire events at times when it is not safe
(event listeners may do all sorts of unexpected things).
Comment 7 Marshall Roch 2012-04-10 15:23:51 PDT
phoang: any progress here? this would be pretty useful to us at Facebook.
Comment 8 phoang 2012-04-10 16:20:07 PDT
I'm still work on it but I'm kinda busy now. I can only work a few hours on weekend.
Comment 9 bugzilla33 2012-06-27 15:00:57 PDT Comment hidden (spam)
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-07-24 10:44:44 PDT
Phaong, I assume you're not actively working on this, so I hope you don't mind me looping in somebody else.

Neil, is this something that, with your focus expertise, you may be willing to do or help with? I took a quick look but don't have the time I used to to dive deeply in this code.
Comment 11 Louis-Rémi BABE 2012-07-30 06:47:52 PDT
I've already documented those two events, specifying that they're not implemented (w/ link to this bug). I've also indicated that focus/blur can be used alternatively, even for event delegation.
https://developer-new.mozilla.org/en-US/docs/Mozilla_event_reference/focusin
https://developer-new.mozilla.org/en-US/docs/Mozilla_event_reference/focusout

We'll have to update these pages once this bug is resolved.
Comment 12 4esn0k 2012-07-30 08:12:12 PDT
some info about support in other browsers:

http://stackoverflow.com/questions/7337670/how-to-detect-focusin-support/11722713#11722713
Comment 13 Olli Pettay [:smaug] 2012-07-31 05:17:34 PDT
Interesting. Looks like some browser engines are doing it all wrong.
The whole point of focusin/out is that they happen before focus/blur.

(and DOMFocusIn/Out have very little to do with focusin/focusout)
Comment 14 Neil Deakin 2012-07-31 05:44:37 PDT
To implement this, it probably requires calling nsFocusManager::SendFocusOrBlurEvent either once for 'focusout' or twice for both 'focusout' and 'focusin' in each case just before nsFocusManager::Blur is called, or near the beginning of Blur. Similarly, for 'focusin' when nsFocusManager::Focus is called.
Comment 15 Neil Deakin 2012-08-28 08:09:19 PDT
*** Bug 95022 has been marked as a duplicate of this bug. ***
Comment 16 bugzilla33 2012-09-27 02:49:52 PDT Comment hidden (me-too)
Comment 17 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-12-17 16:05:19 PST Comment hidden (me-too)
Comment 18 bugzilla77 2013-01-20 02:27:37 PST Comment hidden (me-too)
Comment 19 bugzilla77 2013-01-20 02:29:26 PST Comment hidden (me-too)
Comment 20 Olli Pettay [:smaug] 2013-01-20 02:38:25 PST
When I or someone else finds the time to write the patch.
Trying to look at this soonish.
Comment 21 Mayhem~ 2013-03-12 18:29:35 PDT Comment hidden (me-too)
Comment 22 Olli Pettay [:smaug] 2013-03-12 18:33:32 PDT
Apparently not so soon. Maybe next month.
Comment 23 Olli Pettay [:smaug] 2013-03-12 18:34:10 PDT
If anyone has time to work on this, feel free to take the bug.
Comment 24 Mayhem~ 2013-05-08 07:28:30 PDT Comment hidden (me-too)
Comment 25 Alfredo Yang (:alfredo) 2013-05-17 09:20:46 PDT
Not fmiliar with event compoment but I would like to give it a try.
Comment 26 Josh Matthews [:jdm] 2013-05-17 09:54:49 PDT
Alfredo, take a look at comment 14 and see if it makes sense.
Comment 27 Alfredo Yang (:alfredo) 2013-05-18 01:34:00 PDT
Created attachment 751319 [details] [diff] [review]
add DOM3 focusin/focusout support
Comment 28 Alfredo Yang (:alfredo) 2013-05-18 01:46:55 PDT
Comment on attachment 751319 [details] [diff] [review]
add DOM3 focusin/focusout support

This patch is based on comment 14 and it's a rough version. But it works on above samples at least.

1. Is there any other sample available? Especially for event order in [1]
2. It doesn't implement relatedTarget because of nsFocusEvent does't have it. Is relatedTarget necessary for this case?

Comments are welcome.

[1] http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-focusevent-event-order
Comment 29 Olli Pettay [:smaug] 2013-05-18 02:08:15 PDT
Yes, we want relatedTarget. That is one of the key points of focusin/out.
Need to do testing how other browsers implement this, especially when focus is moving from
other iframes or from other program to FF.

You should add the properties to http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIInlineEventHandlers.idl and to http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl and use something else but NON_IDL_EVENT

Thanks for taking this bug!
Comment 30 Olli Pettay [:smaug] 2013-05-18 02:09:54 PDT
Comment on attachment 751319 [details] [diff] [review]
add DOM3 focusin/focusout support

I know nsFocusManager.cpp doesn't use any coding style consistently, but
could you always use {} with if
Comment 31 Alfredo Yang (:alfredo) 2013-05-18 03:46:57 PDT
Thanks for comment.

(In reply to Olli Pettay [:smaug] from comment #29)
> Yes, we want relatedTarget. That is one of the key points of focusin/out.

From [1]: "For security reasons with nested browsing contexts, when tabbing into or out of a nested context, the relevant EventTarget should be null."
I don't quite understand "nested context" here, does that mean nested iframe?

> I know nsFocusManager.cpp doesn't use any coding style consistently, but
> could you always use {} with if

Sure, no problem.



[1] http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-FocusEvent-relatedTarget
Comment 32 Olli Pettay [:smaug] 2013-05-18 08:12:16 PDT
Yes, iframes have their own browsing contexts.
Comment 33 Alfredo Yang (:alfredo) 2013-05-21 03:18:25 PDT
(In reply to Olli Pettay [:smaug] from comment #29)
> Yes, we want relatedTarget. That is one of the key points of focusin/out.
> Need to do testing how other browsers implement this, especially when focus
> is moving from
> other iframes or from other program to FF.

My original thought is to reuse the focus webidl but it doesn't exist under dom/webidl.
Does current focus event implementation including these context info in [1]?
If no, I'd like to fix that part first before fixing this bug.

[1] http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-focus
Comment 34 Olli Pettay [:smaug] 2013-05-21 06:41:42 PDT
Current focus event uses Event interface, it should be updated to use FocusEvent.
Comment 35 Alfredo Yang (:alfredo) 2013-05-21 19:46:44 PDT
Ok, it looks like Erik post a bug 855741 for FocusEvent interface.
I'll add FocusEvent interface first.
Please let me know if there is any better way.
Thank you.
Comment 36 Alfredo Yang (:alfredo) 2013-06-04 01:47:40 PDT
Created attachment 757848 [details]
focus, focusin, focusout event order test
Comment 37 Alfredo Yang (:alfredo) 2013-06-04 23:50:15 PDT
Created attachment 758426 [details]
iframe focusin, focusout, focus test case
Comment 39 Alfredo Yang (:alfredo) 2013-06-09 21:41:38 PDT
Created attachment 760277 [details] [diff] [review]
add focusin and focusout event

It fires focusin and focusout with the blur event. focusin or focusout will be always with blur except for moving focus from non-focusable element.

test case will be added later.
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-06-10 00:34:58 PDT
You should add automated tests for the patch (It's okay to be separated patch).
Comment 41 Alfredo Yang (:alfredo) 2013-06-12 23:03:27 PDT
Created attachment 761891 [details] [diff] [review]
focusin, focusout mochitest

1. test focusin, focusout, blur, and focus event order amongs 3 text elements.
2. test relatedTarget when moving focus from nested iframe.
Comment 42 Neil Deakin 2013-06-14 06:49:53 PDT
Two things stood out from skimming this patch.


     // otherwise, for inactive windows and when the caller cannot steal the
     // focus, update the node in the window, and  raise the window if desired.
     if (allowFrameSwitch)
       AdjustWindowFocus(newWindow, true);
 
+    if (contentToFocus) {
+      SendFocusInOrOut(NS_FOCUSIN, contentToFocus, mFocusedContent);
+    }
+

This code path changes the focus in an inactive window, so no events should be firing here.


+  if (sendBlurEvent && content) {
+    // relatedTarget should be at the same document as target.
+    nsCOMPtr<nsIContent> relatedTarget;
+    if (IsSameBrowsingContext(content, aContentToFocus)) {
+      relatedTarget = aContentToFocus;
+    }
+    SendFocusInOrOut(NS_FOCUSOUT, content, relatedTarget);
+  }

Sending an event can cause the focus to change, which means that mFocusedContent would be different in the code below and you'd be firing the later blur event on the wrong element since 'content' isn't focused any more.
Comment 43 Olli Pettay [:smaug] 2013-06-14 13:33:33 PDT
(In reply to Neil Deakin from comment #42)

> Sending an event can cause the focus to change, which means that
> mFocusedContent would be different in the code below and you'd be firing the
> later blur event on the wrong element since 'content' isn't focused any more.
Yeah, this is the kind of stuff where one needs to test how other browsers work.
Comment 44 Alfredo Yang (:alfredo) 2013-06-16 01:28:20 PDT
(In reply to Olli Pettay [:smaug] from comment #43)
> (In reply to Neil Deakin from comment #42)
> 
> > Sending an event can cause the focus to change, which means that
> > mFocusedContent would be different in the code below and you'd be firing the
> > later blur event on the wrong element since 'content' isn't focused any more.
> Yeah, this is the kind of stuff where one needs to test how other browsers
> work.

I may not quite understand it.
Do you mean a focusout script changes the focus to another element?
Comment 45 Olli Pettay [:smaug] 2013-06-16 07:52:29 PDT
Yes. focusin/out listeners may move focus to anywhere. Or remove elements from DOM or other
somewhat unexpected stuff. Need to test what other browsers do,
Comment 46 Alfredo Yang (:alfredo) 2013-06-16 18:01:36 PDT
Ok, I'll write another sample to test it.
Comment 47 Alfredo Yang (:alfredo) 2013-06-19 00:08:17 PDT
http://people.mozilla.org/~ayang/687787/change_focus_in_focusin_out.html
A simple test which moves focus to another element in blur, focusin, and focusout handler.
Comment 48 Alfredo Yang (:alfredo) 2013-06-19 00:11:17 PDT
Well, from above test, the result is interesting.
I need more time to think how to handle the focus() command in the script.
Comment 49 andreas.jonsson 2013-11-15 08:38:48 PST
Any luck yet?

And just a quick note. The link posted in comment #47 is missing an 'a' (e.relatedTrget should be e.relatedTarget) on row 61. Without the 'a' the relatedTarget will, of course, give null in any browser.
Comment 50 Alfredo Yang (:alfredo) 2013-11-17 00:29:21 PST
Sorry, I'm busy for others recently. I'll pick it up 2 weeks later.
Comment 51 Mayhem~ 2014-01-20 01:21:24 PST Comment hidden (me-too)
Comment 52 bugzilla77 2014-02-05 14:40:17 PST Comment hidden (me-too)
Comment 53 Hixie (not reading bugmail) 2014-05-27 22:45:17 PDT
Is this really needed? focus/blur have relatedTarget in some browsers (and the spec, now). Do we have a compat need for this?
Comment 54 Masayuki Nakano [:masayuki] (Mozilla Japan) 2014-05-27 23:30:56 PDT
(In reply to Hixie (not reading bugmail) from comment #53)
> Is this really needed? focus/blur have relatedTarget in some browsers (and
> the spec, now). Do we have a compat need for this?

I'm not sure. It seems that the events are suggested by IE. So, asking it to Travis makes you get the reason why they are necessary (or unnecessary).
Comment 55 Tobias Buschor 2014-05-28 01:11:31 PDT
Every other Browser does support it.

I use the following code to support it in firefox to:

https://gist.github.com/nuxodin/9250e56a3ce6c0446efa
Comment 56 wawyed 2014-09-18 02:16:31 PDT
(In reply to Hixie (not reading bugmail) from comment #53)
> Is this really needed? focus/blur have relatedTarget in some browsers (and
> the spec, now). Do we have a compat need for this?

This event is necesary in order to get relatedTarget to be defined as in focus/blur it's always null.

This bug relates to the issue I'm mentioning https://bugzilla.mozilla.org/show_bug.cgi?id=962251
Comment 57 (inactive after 6/18) Alive Kuo [:alive] 2015-08-24 20:10:11 PDT
(In reply to Tobias Buschor from comment #55)
> Every other Browser does support it.
> 
> I use the following code to support it in firefox to:
> 
> https://gist.github.com/nuxodin/9250e56a3ce6c0446efa

This polyfill is incomplete; it does not make every DOM element support focusin/focusout event but only window.document. We still need the platform support or by settings capture in every focus/blur event registration.
Comment 58 alexander 2015-09-10 08:51:49 PDT
It would be awesome if this could be supported (works in all other browsers). Bubbling plus `relatedTarget` are both useful. As is the normalization in itself.

***

An aside, it would be neat if Mozilla could start using Github, I think you'd get more contributions if it was easier to write batches, review the code, etc.
Comment 59 Andrew Overholt [:overholt] 2015-10-16 10:57:23 PDT
Someone said that Edge dropped onfocusin and onfocusout (I think).
Comment 60 Michał Gołębiowski [:m_gol] 2015-10-16 11:02:18 PDT
(In reply to Andrew Overholt [:overholt] from comment #59)
> Someone said that Edge dropped onfocusin and onfocusout (I think).

No, just the 'onfocusin' property on window that jQuery uses to detect if a UA supports those events.
Comment 61 Kevin Wern 2016-03-20 04:59:12 PDT
Created attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Add support for focusin and focusout in DOM.  The event sequence is either:
(for focuses)
- focusin
- focus
or
(for refocuses)
- focusout
- focusin
- blur
- focus
(for unfocuses)
- focusout
- blur

Additionally, add the necessary attributes to ensure only one focus occurs
as the result of a series of events. Also ensure that focusout doesn't
recurse if focus() is called in an event callback.

Include mochitest.

Review commit: https://reviewboard.mozilla.org/r/41261/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41261/
Comment 62 Kevin Wern 2016-03-20 11:09:17 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/1-2/
Comment 63 Kevin Wern 2016-03-20 13:15:11 PDT
Sorry, forgot the test the first time around.

Anyway, I tried the tests above, plus the mochitest (modified because there were some typos).  I need to do a few more things:
- Check that unfocusing using blur() during each event works.
- Implement focus change tests in mochitest.
- Additionally, implement test where a large chain of refocuses results in a cycle (a smaller example is in the above test case, if t4 is focused and refocus is initiated to t1, which should result in t4 again).
- Test blur attempt "inverse semaphore" doesn't cause regression with other events/situations.
- Test other browsers (tested with chrome and safari) for anything dramatically different in behavior.
- Bug I just noticed: focusout case in event-triggered focus test -- there should be a focusout event for t2/t5, as well as t2/(original item to be focused), but currently the former event is suppressed.  Basically, mLastValidFocusOut should be used with a secondary pointer to relatedTarget, because focusout should only be considered the 'same' event if the target AND relatedTarget are the same.
- Additional bug I just noticed: focusing from address bar to something in document does not trigger focus in/out, which differs from Chrome's behavior.  I think this is related to how document-to-document focusing is handled.

Anyway, just keeping everyone in the loop.  Feedback on what I have is appreciated.
Comment 64 Olli Pettay [:smaug] 2016-03-22 16:02:37 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

https://reviewboard.mozilla.org/r/41261/#review38307

Looks like some more tests are needed, and relatedTarget handling doesn't look quite right.

::: dom/base/nsFocusManager.cpp:2024
(Diff revision 2)
> +    , mEventMessage(aEventMessage)
> +    , mRelatedTarget(aRelatedTarget)
> +  {
> +  }
> +
> +  NS_IMETHOD Run()

I don't see anything guaranteeing that target has the same ownerDocument as related target.
For example what happens if focusout removes it's target element from document and moves it to some other document. If I read the code right, looks like we might get then focusin event where .target and .relatedTarget have different ownerDocuments

https://w3c.github.io/uievents/#dom-focusevent-relatedtarget

::: dom/events/test/test_bug687787.html:1
(Diff revision 2)
> +<!DOCTYPE HTML>

So the test doesn't check that document.activeElement hasn't changed yet when focusout/in are dispatched.
Per spec focusout
"This event type MUST be dispatched before the element loses focus."
and focusin
" This event type MUST be dispatched before the element is given focus. "

But please test also what other browsers do.

::: dom/events/test/test_bug687787.html:2
(Diff revision 2)
> +<!DOCTYPE HTML>
> +<html>

Please add tests for cases when .target or .relatedTarget is moved to the document in the iframe, or just removed from its current document.


(it might be nice to have the test as web-platform-test so that other browsers would start to use it for testing too)
Comment 65 Olli Pettay [:smaug] 2016-03-22 16:03:26 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Looking promising! Keep new versions of the patch coming.
Comment 66 Olli Pettay [:smaug] 2016-04-23 13:03:45 PDT
Kevin, any news on this one? Do you need some help?

overholt, is this in some of the tw-dom todo lists.
Comment 67 Kevin Wern 2016-04-23 13:21:55 PDT
Sorry I kind of fell off for a bit; I had a bunch of moving deadlines at work that extended until last Monday--I've been trying to catch up on all my open-source commitments since then. I've been reworking the tests whenever I can and can have an updated version (with the documentOwner fix, as well) by tonight or tomorrow night (PST).

I should have kept you in the loop better, but I wasn't really expecting the work period to last as long as it did when it initially started so time kind of snuck up on me.
Comment 68 Andrew Overholt [:overholt] 2016-04-23 13:24:42 PDT
(In reply to Olli Pettay [:smaug] from comment #66)
> overholt, is this in some of the tw-dom todo lists.

Nope.
Comment 69 Kevin Wern 2016-04-30 02:14:11 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/2-3/
Comment 70 Kevin Wern 2016-04-30 02:21:28 PDT
Sorry about the wait.

Really refactored the test file, and added a check that contentToFocus and content share the same ownerDocument.  I still have to add behavior to focus/blur between documents, but I'm not sure where to start with that (in chrome, focus/blur between documents has a corresponding focusin/focusout).

Additionally, I ran into an issue where the tests for events when switching focus to an iframe's element resulted in document-level focuses, whereas switching focus to an element moved during focusout skipped the document-level focus.  You can see those two iframe tests for more details.
Comment 71 Olli Pettay [:smaug] 2016-05-03 05:00:25 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

https://reviewboard.mozilla.org/r/41261/#review46983

Based on the comment this patch ends up dispatching focusin/out at different times than what other browser do.
Did you test what Chrome and Edge for example do?
The patch doesn't apply cleanly to mozilla-inbound so I couldn't build it and test it myself.
Comment 72 Olli Pettay [:smaug] 2016-05-03 05:01:20 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

So could you please compare to other browsers and then upload a patch which compiles with the latest Gecko code.
Comment 73 Kevin Wern 2016-05-14 12:55:29 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/3-4/
Comment 74 Kevin Wern 2016-05-14 12:58:03 PDT
Hey, Olli, sorry about the delay. I was running into issues and had to upgrade from Mountain Lion to test with inbound.

First, the focusin/focusout between documents/windows does NOT occur in chrome/safari.  That was just a mistake in ad hoc testing I made.  Sorry about that.

Anyway, I uploaded the identical patch (excluding some changes to the mochitest because we now have relatedTarget for focus/blur) rebased against mozilla-inbound for you to test. I'm also going to attach the test I've been using for other browsers, which is basically actions of the mochitest but logging the event sequence to console.

Based on testing, there are a lot of differences regarding events fired that are all due to the same underlying cause: focusin and focusout occur AFTER focus/blur respectively in other browsers.  So the proper object is NOT focused in either event, and when redirects happen, relatedTarget doesn't reflect the correct element (normally null/document instead of the original object).

I'm uploading for you to test because I'm confused with how to proceed and want to make sure we're on the same page.  I'm not sure what takes priority here. If it is spec, then we still need to add window level focusin/focusout.  If it is what other browsers are doing, then the order of the events needs to be changed.  I think other browsers are doing this sequence because it is easier to implement and raises fewer questions about how to handle the resulting cases.

On a related note, it seems that browser window focus fire on window in Chrome/Safari, but document in Firefox. Is this a separate discrepancy we should be concerned about, or am I testing something incorrectly?
Comment 75 Kevin Wern 2016-05-14 13:00:04 PDT
Created attachment 8752509 [details]
Mochitest cases (logs to console)
Comment 76 Oleg 2016-05-15 23:51:08 PDT
Glad to see progress on this, keep in mind though that most use of such events is probably comes from using jquery events, which are implemented according to the spec.

It would be otherwise painful to see violation of the spec in here.

Also, and its very important for at least jQuery core team, that firefox would have the ability to "show" on the DOM level how these events are implemented - according to the spec or with some different behaviour in mind.

Currently, such check look like this - https://github.com/jquery/jquery/blob/95c7ab68970ce201a2bbff48c8e951d38c228ce8/src/event/support.js#L7

Which (thank luck for that) distinguish old versions of IE (in which focus{in | out}) events follow the spec and modern browsers who don't.

So it would be really preferable to either add `focus(in | out)` properties to the `window` object (if implemented as described in the spec) or provide other means for it.

Thank you
Comment 77 Chris Rebert 2016-05-16 00:06:40 PDT
(In reply to Kevin Wern from comment #74)
> On a related note, it seems that browser window focus fire on window in
> Chrome/Safari, but document in Firefox. Is this a separate discrepancy we
> should be concerned about, or am I testing something incorrectly?

Perhaps you're referring to the same problem as bug 1228802 ?
Comment 78 Kevin Wern 2016-05-16 00:38:03 PDT
(In reply to Chris Rebert from comment #77)
> (In reply to Kevin Wern from comment #74)
> > On a related note, it seems that browser window focus fire on window in
> > Chrome/Safari, but document in Firefox. Is this a separate discrepancy we
> > should be concerned about, or am I testing something incorrectly?
> 
> Perhaps you're referring to the same problem as bug 1228802 ?

Yes. That is to say, they all fire focus events on window but firefox *also* fires focus events on document.
Comment 79 Kevin Wern 2016-05-16 00:48:25 PDT
Created attachment 8752683 [details]
Mochitest cases (logs to console), fixed incorrect label
Comment 80 Kevin Wern 2016-05-16 01:02:52 PDT
> I'm uploading for you to test because I'm confused with how to proceed and
> want to make sure we're on the same page.  I'm not sure what takes priority
> here. If it is spec, then we still need to add window level
> focusin/focusout.  If it is what other browsers are doing, then the order of
> the events needs to be changed.  I think other browsers are doing this
> sequence because it is easier to implement and raises fewer questions about
> how to handle the resulting cases.

Additionally, to clarify here, focusin/focusout is not fired on window in Chrome/Safari.  So that is what adds the question of priority to implementing focusin/focusout on window, in addition to the event order mentioned in the previous paragraph.
Comment 81 Michał Gołębiowski [:m_gol] 2016-05-16 10:25:25 PDT
I checked the order of events in various browsers and jQuery's emulated focusin/focusout w/ native focus/blur. tl;dr: it's a mess:

https://github.com/jquery/jquery/issues/3123
Comment 82 Olli Pettay [:smaug] 2016-05-21 14:15:55 PDT
(In reply to Kevin Wern from comment #74)
> Hey, Olli, sorry about the delay. I was running into issues and had to
> upgrade from Mountain Lion to test with inbound.
Sorry, I'm also late here. Have had tons of stuff to review.

> Based on testing, there are a lot of differences regarding events fired that
> are all due to the same underlying cause: focusin and focusout occur AFTER
> focus/blur respectively in other browsers.  So the proper object is NOT
> focused in either event, and when redirects happen, relatedTarget doesn't
> reflect the correct element (normally null/document instead of the original
> object).
Sad, but we really need to follow the other browsers here and get the spec changed to specify what browsers actually do.
 
> I'm uploading for you to test because I'm confused with how to proceed and
> want to make sure we're on the same page.  I'm not sure what takes priority
> here. If it is spec, then we still need to add window level
> focusin/focusout. 
It is compatibility with the web which counts here, and that means being compatible with other browsers.

> If it is what other browsers are doing, then the order of
> the events needs to be changed.
Yes, that. Unfortunately. I have no idea why they totally crazy ordering, but we can't really do it differently.


  I think other browsers are doing this
> sequence because it is easier to implement and raises fewer questions about
> how to handle the resulting cases.
right. Yeah, we need to follow here what others are already doing and then get the spec changed.
(Though, since other browsers are so broken with focusin/out, it is rather mystery to me why people are asking us to implement these events)

> On a related note, it seems that browser window focus fire on window in
> Chrome/Safari, but document in Firefox.
There is a separate bug to remove support for document level focus/blur in Gecko.
Gecko does fire also window and element level focus/blur

I'll ping Edge and Blink folks about this stuff next week to ask why they have implemented this so oddly.
Comment 83 Olli Pettay [:smaug] 2016-05-21 14:19:36 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

So sounds like this is trying to follow the spec, not what other browsers are doing.

(it would be great to have some web-platform-tests for this. That would make it easier to run the tests in different browsers.)

Clearing the request while we're sorting this spec mess out.

Please keep pinging me or folks from other browser vendors about this stuff. It is really sad to see these possibly useful events to be so broken everywhere, but I'm afraid other implementations can't be fixed anymore. Web probably relies on the behavior.
Comment 84 Michał Gołębiowski [:m_gol] 2016-05-21 15:20:07 PDT
(In reply to Olli Pettay [:smaug] from comment #82)
> (In reply to Kevin Wern from comment #74)
> > Based on testing, there are a lot of differences regarding events fired that
> > are all due to the same underlying cause: focusin and focusout occur AFTER
> > focus/blur respectively in other browsers.  So the proper object is NOT
> > focused in either event, and when redirects happen, relatedTarget doesn't
> > reflect the correct element (normally null/document instead of the original
> > object).
> Sad, but we really need to follow the other browsers here and get the spec
> changed to specify what browsers actually do.

Please read my analysis that I linked to in [1]. Other browsers don't even agree between each other, Chrome/Safari have one behavior, Edge has another, IE 11 is following the spec here.

Please don't land anything disagreeing with the spec until you get all of the browsers to agree on one behavior, preferably a useful one.

Also, note that the spec order (see [2]) was written in this way so that you can react to the focus change before it happens via focusin/focusout events, it'd be good to preserve that specced behavior unless it's web-incompatible; but it shouldn't considering that IE 11 implemented this event order.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=687787#c81
[2] https://www.w3.org/TR/uievents/#h-events-focusevent-event-order
Comment 85 Olli Pettay [:smaug] 2016-05-22 00:37:03 PDT
I know the spec'ed behavior makes most sense but Blink/webkit don't follow the spec. And in Edge MS has decided to change their behavior, there probably has been some reason for that (like some compatibility with Blink?).
But I'll ask around the other browser vendors whether they'd be willing to change the behavior.
I wouldn't be too optimistic about that.
Comment 86 Olli Pettay [:smaug] 2016-05-22 01:01:32 PDT
m_gol, if, say blink, changed its behavior to follow the spec, would pages using jquery be broken?
(sounds like jquery is a major user for the events, https://www.w3.org/Bugs/Public/show_bug.cgi?id=25897#c6)
Comment 87 Kevin Wern 2016-06-01 22:02:31 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/4-5/
Comment 88 Kevin Wern 2016-06-01 23:08:08 PDT
Didn't flag it for review, but I added two web-platform-tests—one for the w3 standard and one for blink/webkit.  There is a test that I am unsure of, which is the test that moves the target to a different document ('iframe-moves-target').  I am including it because the concept was discussed earlier (comment 64), but I think the combined info we have is only enough for the first two tests—IE was not even a good model for the ‘refocus’ tests created in the mochitest.

In FF, the correct element is focused, but the focus is not consistent through the tree, i.e. the element is focused, but the parent iframe is not.  In other browsers, (IE, Chrome, Safari), the same thing results in nothing being focused.  However, this other behavior existed in FF before these changes so I don't think it is within the scope of this bug.

The other two tests pass on their intended groups, with the exception of IE, which does not fire events at the right time relative to actual focuses (i.e. target element is already focused on focusin).  This, combined with perceivably buggier behavior compared to other browsers in ad hoc testing (doesn't shift focus if focusin calls .focus() on another element, and other .focus() shifts results in events from both the old and redirect scenario being pushed to the queue), doesn't seem to make it a very reliable 'model' for w3 behavior like I had hoped.

Edge's behavior is excluded, like we discussed.

Olli, have you heard anything from the other browser devs?  I've only seen the two comments on the github issue, which seems to suggest that the behavior will not be changed.  I think we should match webkit's behavior if we haven't heard anything.
Comment 89 Neil Deakin 2016-06-02 02:50:19 PDT
Judging from https://bugs.webkit.org/show_bug.cgi?id=36266 it looks like webkit just implemented focusin/focusout as if they were renamed versions of DOMFocusIn/DOMFocusOut and that seems to be how they behave in Safari/Chrome, meaning that they are only useful over focus/blur because they bubble.

IE has always implemented focusin/focusout as firing before focus shifts, which is the behaviour the spec is based on, and I think what this patch is doing.

Reading the patch though, it doesn't look like it fires focusin if nothing is currently focused, and I don't see a test for this, although I didn't look in detail.
Comment 90 Kevin Wern 2016-06-04 18:37:24 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/5-6/
Comment 91 Kevin Wern 2016-06-04 18:41:55 PDT
Comment on attachment 8732615 [details]
Bug 687787 - Support focusin/focusout events

Sorry, clearing review flag.  For some reason clearing the field in MozReview doesn't work now.
Comment 92 Kevin Wern 2016-06-04 18:42:38 PDT
(In reply to Kevin Wern from comment #91)
> Comment on attachment 8732615 [details]
> Sorry, clearing review flag.  For some reason clearing the field in
> MozReview doesn't work now.

Or rather, didn't* work.
Comment 93 Kevin Wern 2016-06-04 19:01:23 PDT
(In reply to Neil Deakin from comment #89)
> IE has always implemented focusin/focusout as firing before focus shifts,
> which is the behaviour the spec is based on, and I think what this patch is
> doing.
>
> Reading the patch though, it doesn't look like it fires focusin if nothing
> is currently focused, and I don't see a test for this, although I didn't
> look in detail.

Good catch, thanks! Added that functionality, with mochitests, to this iteration.

Note You need to log in before you can comment on or make changes to this bug.