Closed Bug 687787 Opened 13 years ago Closed 8 years ago

Add support for DOM3 focusin/focusout

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
platform-rel --- ?
firefox52 --- fixed

People

(Reporter: smaug, Assigned: kevin.m.wern, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active [platform-rel-jQuery] [platform-rel-React])

Attachments

(5 files, 4 obsolete files)

testcase
1.16 KB, text/html
Details
add focusin and focusout event
17.84 KB, patch
Details | Diff | Splinter Review
focusin, focusout mochitest
4.96 KB, patch
Details | Diff | Splinter Review
Bug 687787: support focusin/focusout based on webkit/blink
58 bytes, text/x-review-board-request
smaug
: review+
Details
Mochitest cases (logs to console), fixed incorrect label
17.36 KB, text/html
Details
Attached file testcase — 

    
  

        Attachment #563001 -
        Attachment mime type: text/plain → text/html

    
    

    
      
  

    
    

    
  
(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 ;)

    
    

    
  
Anybody have the ideas how to implement it. Our team is going to take this work.

    
    

    
  
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).

    
    

    
  
phoang: any progress here? this would be pretty useful to us at Facebook.

    
    

    
  
I'm still work on it but I'm kinda busy now. I can only work a few hours on weekend.

    
    

    
      
  

    
    

    
  
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.

    
  
Keywords: dev-doc-needed

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
When I or someone else finds the time to write the patch.
Trying to look at this soonish.

    
    

    
      
  

    
    

    
  
Apparently not so soon. Maybe next month.

    
    

    
  
If anyone has time to work on this, feel free to take the bug.

    
    

    
      
  

    
    

    
  
Not fmiliar with event compoment but I would like to give it a try.

    
    

    
  
Alfredo, take a look at comment 14 and see if it makes sense.
Assignee: bugs → ayang

    
    

    
  
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

        Attachment #751319 -
        Flags: review?(bugs)

    
    

    
  
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 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

        Attachment #751319 -
        Flags: review?(bugs)

    
    

    
  
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

    
    

    
  
Yes, iframes have their own browsing contexts.

    
    

    
  
(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

    
    

    
  
Current focus event uses Event interface, it should be updated to use FocusEvent.

    
    

    
  
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.
Depends on: 855741

        Attachment #757848 -
        Attachment is obsolete: true

        Attachment #758426 -
        Attachment is obsolete: true

    
    

    
  


  
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.

        Attachment #751319 -
        Attachment is obsolete: true

        Attachment #760277 -
        Flags: review?(bugs)

    
    

    
  
You should add automated tests for the patch (It's okay to be separated patch).

    
    

    
  


  
1. test focusin, focusout, blur, and focus event order amongs 3 text elements.
2. test relatedTarget when moving focus from nested iframe.

        Attachment #761891 -
        Flags: review?(bugs)

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
(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?

    
    

    
  
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,

    
    

    
  
Ok, I'll write another sample to test it.

    
    

    
  
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.

        Attachment #760277 -
        Flags: review?(bugs)

        Attachment #761891 -
        Flags: review?(bugs)

    
    

    
  
Well, from above test, the result is interesting.
I need more time to think how to handle the focus() command in the script.

    
  
Blocks: 934525

    
    

    
  
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.

    
    

    
  
Sorry, I'm busy for others recently. I'll pick it up 2 weeks later.

    
    

    
      
  
Blocks: 968240

    
    

    
      
  

    
    

    
  
Is this really needed? focus/blur have relatedTarget in some browsers (and the spec, now). Do we have a compat need for this?

    
    

    
  
(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).

    
    

    
  
Every other Browser does support it.

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

https://gist.github.com/nuxodin/9250e56a3ce6c0446efa

    
    

    
  
(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

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
Someone said that Edge dropped onfocusin and onfocusout (I think).

    
    

    
  
(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.

    
  
Assignee: ayang → kevin.m.wern

    
    

    
  


  
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/

        Attachment #8732615 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/1-2/

    
    

    
  
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.

        Attachment #8732615 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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 on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Looking promising! Keep new versions of the patch coming.

        Attachment #8732615 -
        Flags: review-

    
    

    
  
Kevin, any news on this one? Do you need some help?

overholt, is this in some of the tw-dom todo lists.
Flags: needinfo?(overholt)
Flags: needinfo?(kevin.m.wern)

    
    

    
  
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.
Flags: needinfo?(kevin.m.wern)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #66)
> overholt, is this in some of the tw-dom todo lists.

Nope.
Flags: needinfo?(overholt)
Whiteboard: btpp-active

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/2-3/

        Attachment #8732615 -
        Flags: review- → review?(bugs)

    
    

    
  
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 on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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.

        Attachment #8732615 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

So could you please compare to other browsers and then upload a patch which compiles with the latest Gecko code.

        Attachment #8732615 -
        Flags: review-

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/3-4/

        Attachment #8732615 -
        Flags: review- → review?(bugs)

    
    

    
  
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?
Flags: needinfo?(bugs)

    
    

    
  

      
      
      
      

        Attached file
          
        Mochitest cases (logs to console) (obsolete)
        — 
      

    


  

    
    

    
  
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

    
    

    
  
(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 ?

    
    

    
  
(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.

    
    

    
  


  

        Attachment #8752509 -
        Attachment is obsolete: true

    
    

    
  
> 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.

    
    

    
  
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

    
    

    
  
(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.
Flags: needinfo?(bugs)

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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.

        Attachment #8732615 -
        Flags: review?(bugs)

    
    

    
  
(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

    
    

    
  
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.

    
    

    
  
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 on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/4-5/

    
    

    
  
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.
Flags: needinfo?(bugs)

    
    

    
  
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 on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41261/diff/5-6/

        Attachment #8732615 -
        Attachment description: MozReview Request: Bug 687787 - Support focusin/focusout events r=smaug → Bug 687787 - Support focusin/focusout events

        Attachment #8732615 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

Sorry, clearing review flag.  For some reason clearing the field in MozReview doesn't work now.

        Attachment #8732615 -
        Flags: review?(bugs)

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
Would it be possible for this fix to be merged soon? The ticket has been open for five years now!

    
    

    
  
The issue here is that the specification has the sane behavior, actually useful behavior,
but blink/webkit/edge implement something else (something where the normal focus/blur should work just fine).
IE implements the sane behavior, the one the spec has.

See https://github.com/w3c/uievents/issues/88.

    
    

    
  
@smaug,

Being on the same side as jQuery folks I can only second their opinion: I don't really care which way focus/blur events come with regards to focusin/focusout as long as blur fires before focus and focusout fires before focusin. In theory IE event order might seem more helpful but intermixing focus/blur with focusin/focusout is not really helpful in practice so we're not doing that in Ext JS. These are just different events for different purposes.

The biggest advantage for focusin/focusout is that they bubble, which makes focus handling much saner for complex widgets like grids and trees. It's not to say that this can't be dealt with via capturing but you don't want to see the code. :/

Adding an insult to the injury, Firefox doesn't provide relatedTarget in focus events and AFAIK the fix is also in the patch attached to this ticket. Honestly I can't say which issue makes my life more miserable, lack of focusin/focusout or lack of relatedTarget but I'd really truly wish I could have them both in next Firefox release.

Pretty please?

    
    

    
  
(In reply to Alex Tokarev from comment #96)
> @smaug,
> 
> Being on the same side as jQuery folks I can only second their opinion: I
> don't really care which way focus/blur events come with regards to
> focusin/focusout as long as blur fires before focus and focusout fires
> before focusin. In theory IE event order might seem more helpful but
> intermixing focus/blur with focusin/focusout is not really helpful in
> practice so we're not doing that in Ext JS. These are just different events
> for different purposes.
We can't really implement new APIs "do whatever" way, but follow some spec.
So in this case we either need to agree with other browser vendors that the spec needs to be changed, 
or they start to follow the spec.



> Adding an insult to the injury, Firefox doesn't provide relatedTarget in
> focus events 
Yes it does, see Bug 962251
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #97)

> We can't really implement new APIs "do whatever" way, but follow some spec.
> So in this case we either need to agree with other browser vendors that the
> spec needs to be changed, or they start to follow the spec.

Right. What I'm trying to say is that in this case either way is acceptable from the practical point of view; as long as there's something we can work with it. Splitting hairs over tentative advantages of one approach over the other is not really helping us JS devs out here in the wild.

If you got to choose then again from the practical perspective it's much easier to run with the pack. IE was a very sane browser in some respects but it's dead. Most of the web development that is happening now is geared towards WebKit, Blink, and Edge, and they do things the same way -- just fall in line and move on, you won't be breaking anything. Specs can be changed later.

Again I'm not advocating wild shooting cowboy approach but this issue has been dragging for far too long. Seriously people, five years? Considering that Firefox is basically *the* only browser that Assistive Techs work with acceptably, it makes FF the only major pain in the butt left on the table now that IE has departed for greener pastures.

> Yes it does, see Bug 962251

Oh wow, I somehow missed that one. Sweet, thanks!

    
    

    
  
kevin, do you think you could write a patch following what blink/webkit/edge do?

I wonder if blink/webkit model is just to fire bubbling focusout/in right after blur/focus.
Flags: needinfo?(kevin.m.wern)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #99)
> I wonder if blink/webkit model is just to fire bubbling focusout/in right
> after blur/focus.

Yes, as shown here:

https://chromium.googlesource.com/chromium/blink/+/master/Source/core/dom/Document.cpp#3487

They just fire the three events one after each other. I believe that they misread the spec.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #99)
> kevin, do you think you could write a patch following what blink/webkit/edge
> do?

Yes, working on it now.
Flags: needinfo?(kevin.m.wern)

    
    

    
  
At this it is not clear what desired behaviour should be, there is real possibility of changing the spec so it would follow blink implementation. I would also urge you to wait until we (jQuery team) can provide our recommendation, which should happen at the next Monday

    
    

    
  
I agree with Alex. Just getting an *awareness* of when focus moves with the document would be a huge step.

Application state involves focus. Our applications use focus as a concrete indicator of state and behaviour, not as an afterthought. Focus changes have application consequences, and application code can cause focus changes. It's a fully two way street.

It's just not acceptable to be so far behind other browsers.

If the exact order changes in future, so be it.

    
    

    
  
What awareness? There are focus and blur events already.
(I'm having a bit trouble to see how the focusin/out implementation blink/webkit has is so useful. 
The spec model for focusin/out is way different.)

    
    

    
  
But yes, we'll add focusin/out. I've been just hoping to have reasonable behavior for them, but perhaps we'll need to just follow the model blink/webkit have.

    
    

    
  
Lack of a relatedTarget is just insanity. It reduces the event to uselessness.

    
    

    
  
relatedTarget is supported, and nothing to do with this bug.

    
    

    
      
  

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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

Did you test focus/blur handling on window object? Looks like blink does _not_ dispatch focusin/out on Window (but please verify this), which is rather crazy, but if we go with webkit/blink model, better to ensure in tests that we actually have that model.

::: dom/base/nsFocusManager.cpp:2155
(Diff revision 7)
> +      if (currentFocusedContent == newFocus) {
> +        EventMessage focusInOrOutMessage;
> +
> +        if (aEventMessage == eFocus) {
> +          focusInOrOutMessage = eFocusIn;
> +        }

nit, } else {

::: dom/base/nsFocusManager.cpp:2159
(Diff revision 7)
> +          focusInOrOutMessage = eFocusIn;
> +        }
> +        else {
> +          focusInOrOutMessage = eFocusOut;
> +        }
> +        SendFocusInOrOutEvent(focusInOrOutMessage, aPresShell, aTarget,

I don't think we can do it this way, using separate script runner for focusin/out. We need to dispatch focusin/out using the same script runner as blur/focus and do the check for the currently focused element there.
Or, actually we could just move currentFocusedContent check to the script runner which is used for focusin/out. That would give the same behavior what webkit/blink seem to have that if focus/blur event ends up moving focus, focusin/out won't be dispatched. 
Note, the patch is buggy only in case script runner isn't run synchronously.

::: testing/web-platform/tests/uievents/order-of-events/focus-events/focus-automated-blink-webkit.html:1
(Diff revision 7)
> +<!DOCTYPE html>

I assume you run this test on webkit or blink and it passed.

        Attachment #8732615 -
        Flags: review?(bugs) → review-

    
    

    
      
  

    
    

    
  
Not sure if this is the "best" way to check if the target is a window, or to check the focus was not changed within the focusin/focusout Runnable, so anything that might be more eloquent is appreciated.

Otherwise, this patch should have the complete behavior we are looking for.

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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

I tested it for the earlier patch, (it's in an earlier comment somewhere), but I missed it this time around. Tested it with this version.

> I assume you run this test on webkit or blink and it passed.

Yes, ran for both (Chromium and Midori)

    
    

    
  
jQuery team posted our view on the subject in https://github.com/w3c/uievents/issues/88#issuecomment-252672694, please note importance of the second item – 

> Browsers must provide some feature detect so that developers can know if focusin/out are implemented

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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

Still some tweaks to get this to behave similar to others.

(and whether or not we want to follow the spec after all should be discussed in the spec bug. I'll ping blink/edge/webkit folks there.)

::: dom/base/nsFocusManager.cpp:2061
(Diff revision 8)
> +  nsCOMPtr<nsISupports>   mTarget;
> +  RefPtr<nsPresContext>   mContext;
> +  EventMessage            mEventMessage;
> +  nsCOMPtr<nsPIDOMWindowOuter>    mOriginalFocusedWindow;
> +  nsCOMPtr<nsIContent>            mOriginalFocusedContent;
> +  nsCOMPtr<EventTarget>   mRelatedTarget;

Odd indentation for the member variables. Either just have one space after type, or align variable names.

::: dom/base/nsFocusManager.cpp:2166
(Diff revision 8)
>  
>    if (!dontDispatchEvent) {
>      nsContentUtils::AddScriptRunner(
>        new FocusBlurEvent(aTarget, aEventMessage, aPresShell->GetPresContext(),
>                           aWindowRaised, aIsRefocus, aRelatedTarget));
> +    if (!targetWindow) {

Could you add some comment why !targetWindow
(that we don't want to fire focusin/out for window).
But, I think we want similar check for document too.
We do still fire focus/blur on document, but IIRC others don't.

::: dom/events/test/test_bug687787.html:186
(Diff revision 8)
> +                    + eventStack[i].type + ','
> +                    + (eventStack[i].target ? eventStack[i].target.id : null) + ','
> +                    + (eventStack[i].relatedTarget ? eventStack[i].relatedTarget.id : null) + ')');
> +    }
> +
> +    while (content.firstChild) {

Why not just 
content.innerHTML = null;
Same also elsewhere

::: dom/events/test/test_bug687787.html:594
(Diff revision 8)
> +    expectedEventOrder = [
> +        {'type' : 'focus',
> +        'target' : document,
> +        'relatedTarget' : null},
> +        {'type' : 'focusin',
> +        'target' : document,

I don't think we want focusin on document.

::: dom/webidl/EventHandler.webidl:35
(Diff revision 8)
>             attribute EventHandler onblur;
>  // We think the spec is wrong here. See OnErrorEventHandlerForNodes/Window
>  // below.
>  //         attribute OnErrorEventHandler onerror;
>             attribute EventHandler onfocus;
> +           attribute EventHandler onfocusin;

Let's not add onfocusin/out handlers. Other browsers don't have them either.

        Attachment #8732615 -
        Flags: review?(bugs) → review-

    
    

    
  
(In reply to Oleg from comment #113)

> > Browsers must provide some feature detect so that developers can know if focusin/out are implemented

While theoretically I would love to have something like that, practically speaking it's a day late and a dollar short kind of situation. The only browser that have had focusin/focusout unimplemented for the last web eon is Firefox.

I don't know what browsers jQuery has to support these days but IE has had the right focusin/focusout implementation at least since 5.5. Any other browser old enough not to have it is obscure enough not to be supported; feature detecting is therefore close to being meaningless. Just assume (!browser.isFirefox || browser.version >= _focusin_implemented_) and you're good to go.

    
    

    
  
(In reply to Alex Tokarev from comment #115)
> (In reply to Oleg from comment #113)
> 
> > > Browsers must provide some feature detect so that developers can know if focusin/out are implemented
> 
> While theoretically I would love to have something like that, practically
> speaking it's a day late and a dollar short kind of situation. The only
> browser that have had focusin/focusout unimplemented for the last web eon is
> Firefox.
> 
> I don't know what browsers jQuery has to support these days but IE has had
> the right focusin/focusout implementation at least since 5.5. Any other
> browser old enough not to have it is obscure enough not to be supported;
> feature detecting is therefore close to being meaningless. Just assume
> (!browser.isFirefox || browser.version >= _focusin_implemented_) and you're
> good to go.

You misunderstood, we need a way to feature detect spec intended behaviour, not just presence of events called `focus(in | out)`.

IE behaviour also does not conform with spec btw, no browsers does at this point. Those events should have synchronous execution (according to the spec) in IE they are not.

    
    

    
  
(In reply to Oleg from comment #116)
> You misunderstood, we need a way to feature detect spec intended behaviour,
> not just presence of events called `focus(in | out)`.

Right, thanks for the clarification.

> IE behaviour also does not conform with spec btw, no browsers does at this
> point. Those events should have synchronous execution (according to the
> spec) in IE they are not.

This simply means that the spec is incorrect or incomplete. I don't see any value in having a spec that contradicts real world browser behavior for the sake of argument; I see even less value in browsers having a flag for "we don't care what the spec says but here, have a cookie".

It is easier to change the spec to fall in line with existing implementations than the other way around. IE is dead and won't change anymore; Edge team has much bigger fish to fry than focusin/focusout event sequence. I suspect the same can be said about other browser dev teams. It's just not reasonable to try and push an older spec based on already obsolete browser when everybody else disagrees with it.

It might have been another story if the older spec was much superior to the implemented approach, so much so that it would be worth pushing *every* browser out there to conform with it. However if that was the case, I suspect the other browsers would already be implementing focusin/focusout events close to the spec, rendering the point moot.

Coming from this angle, it's much easier and more productive to adopt the existing behavior and change the spec to match it. Then we'll have the cake and eat it, too - things will be working according to new expectations, and no feature detection will be necessary.

    
    

    
  
(In reply to Alex Tokarev from comment #117)
> (In reply to Oleg from comment #116)
> > You misunderstood, we need a way to feature detect spec intended behaviour,
> > not just presence of events called `focus(in | out)`.
> 
> Right, thanks for the clarification.
> 
> > IE behaviour also does not conform with spec btw, no browsers does at this
> > point. Those events should have synchronous execution (according to the
> > spec) in IE they are not.
> 
> This simply means that the spec is incorrect or incomplete. I don't see any
> value in having a spec that contradicts real world browser behavior for the
> sake of argument; I see even less value in browsers having a flag for "we
> don't care what the spec says but here, have a cookie".
> 
> It is easier to change the spec to fall in line with existing
> implementations than the other way around. IE is dead and won't change
> anymore; Edge team has much bigger fish to fry than focusin/focusout event
> sequence. I suspect the same can be said about other browser dev teams. It's
> just not reasonable to try and push an older spec based on already obsolete
> browser when everybody else disagrees with it.
> 
> It might have been another story if the older spec was much superior to the
> implemented approach, so much so that it would be worth pushing *every*
> browser out there to conform with it. However if that was the case, I
> suspect the other browsers would already be implementing focusin/focusout
> events close to the spec, rendering the point moot.
> 
> Coming from this angle, it's much easier and more productive to adopt the
> existing behavior and change the spec to match it. Then we'll have the cake
> and eat it, too - things will be working according to new expectations, and
> no feature detection will be necessary.

This discussion is happening in https://github.com/w3c/uievents/issues/88, we (jQuery team) recommend spec behaviour, it seems Blink and Edge teams are willing to change theirs implementations

    
    

    
  
(In reply to Alex Tokarev from comment #117)
> It is easier to change the spec to fall in line with existing
> implementations than the other way around.

That would, indeed, be simpler if there was common behavior in other implementation but there's not. Edge is different to Chrome/Safari and all three are different to IE [1]. Whatever gets standardized, either all of the browsers will need to change or all but one (two in case of the Chrome/Safari route).

The Edge folks seem interested in aligning to a common standard; see https://github.com/w3c/uievents/issues/88#issuecomment-247727901

The Chrome folks are interested as well:
https://github.com/w3c/uievents/issues/88#issuecomment-245280141

[1] https://github.com/jquery/jquery/issues/3123#issue-155077612

    
    

    
      
  

    
    

    
  
Comment on attachment 8732615 [details]
Bug 687787: support focusin/focusout based on webkit/blink

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

::: dom/base/nsFocusManager.cpp:2043
(Diff revision 9)
> +  {
> +  }
> +
> +  NS_IMETHOD Run() override
> +  {
> +    InternalFocusEvent event(true, mEventMessage);

Move InternalFocusEvent inside the 'if'

::: dom/base/nsFocusManager.cpp:2050
(Diff revision 9)
> +    event.mFlags.mCancelable = false;
> +    event.mRelatedTarget = mRelatedTarget;
> +    nsCOMPtr<nsIContent> originalWindowFocus = mOriginalFocusedWindow ?
> +        mOriginalFocusedWindow->GetFocusedNode() :
> +        nullptr;
> +    if (originalWindowFocus == mOriginalFocusedContent) {

So blink does this focused node check only on focusin events, not focusout.

in blink it is possible to have events for the same target in order: blur, focus, focusin, focusout and have the target be focused after that. Feels odd, but since we are now just trying to replicate the insanity blink has, perhaps better to copy also that
(so that focusout still works fine if focus is just moved in blur event listener to some 3rd element.)
So, could you make that check
mEventMessage == eFocusOut || originalWindowFocus == mOriginalFocusedContent 

Some test would be nice for this odd case, I guess by modifying TestBlurRedirectsFocus a bit in the test.

::: dom/base/nsFocusManager.cpp:2176
(Diff revision 9)
> +    //
> +    // As for document, we should not even fire focus/blur, but until then, we
> +    // need this check. targetDocument should be removed once bug 1228802 is
> +    // resolved.
> +    if (!targetWindow && !targetDocument) {
> +      EventMessage focusInOrOutMessage;

Might be a bit simpler to just have
EventMessage focusInOrOutMessage = 
  aEventMessage == eFocus ? eFocusIn: eFocusOut;

but up to you

        Attachment #8732615 -
        Flags: review?(bugs) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8379fb0aa9d&selectedJob=29517381

Earlier patch was to fix a naming error in nsBaseWidget.cpp.

    
    

    
  
I asked you on IRC, as well, but what does this earlier try run indicate?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8379fb0aa9d&selectedJob=29517381

Sorry, I'm not so great with the testing tools outside of ignoring clearly marked bugs or and fixing errors associated with my patch.
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Kevin Wern from comment #125)
> Sorry, I'm not so great with the testing tools outside of ignoring clearly
> marked bugs or and fixing errors associated with my patch.

or fixing errors*

    
    

    
  
Seems like known oranges only. So I think the patch could land.
Could you comment in bug 1301306 that focusin/out may need to be taken care of too.

If other browsers agree and actually start changing their implementations towards what is in the spec, then we should do it too. If that happens, hopefully it happens soon enough before this gets to release builds. But as of now, it is still unclear whether that will ever happen.

And once the patch lands and people start testing it and find issues, please file new bugs and make them block this one. No need to comment about possible issues in this already long bug.
Flags: needinfo?(bugs)

    
  
Keywords: checkin-needed

    
    

    
  
Please mark the pending issues in MozReview as resolved so this can autoland.
Keywords: checkin-needed

    
  
Keywords: checkin-needed

    
    

    
  
Autoland couldn't rebase this for landing. Please rebase and resubmit for checkin :(
Keywords: checkin-needed

    
    

    
      
  

    
    

    
      
  

    
    

    
  
The second commit is to re-update the wpt manifest after rebasing. In a try run, the linter complained about the new web platform test appearing in the wrong spot.

The new try run after this change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e335183a8c1

    
  
Keywords: checkin-needed

    
    

    
  
still has problems and needs rebasing

 conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(kevin.m.wern)

    
    

    
      
  

    
    

    
  
I just tried rebasing again and couldn't reproduce your issue. I pushed that to review just in case (there was a 500 error on my first attempt to push the original rebase, so maybe that was the issue?), but the diff is a no-op visually.

The tip at the time of this rebase:

changeset:   320548:374fb57de7e2
user:        Sebastian Hengst <archaeopteryx@coole-files.de>
date:        Wed Nov 02 16:54:08 2016 +0100
summary:     Bug 1305977 - Use HarfBuzz ot-math API to parse the OpenType MATH table: disable test multiscripts-1.html on Windows. r=developer-request on a C
Flags: needinfo?(kevin.m.wern)

    
    

    
  
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa39c1a962ff
support focusin/focusout based on webkit/blink r=smaug
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/aa39c1a962ff
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
platform-rel: --- → ?
Whiteboard: btpp-active → btpp-active [platform-rel-jQuery]
Whiteboard: btpp-active [platform-rel-jQuery] → btpp-active [platform-rel-jQuery] [platform-rel-React]

    
    

    
  
I've updated the relevant pages to say Fx is now supported:

https://developer.mozilla.org/en-US/docs/Web/Events/focusin
https://developer.mozilla.org/en-US/docs/Web/Events/focusout

I've also added a note to the Fx 52 release notes page:

https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

Let me know if this looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete

    
    

    
  
Hey, I've been watching this bug for a while now, and while I'm glad it is being implemented, it doesn't look like the event order is following the spec (https://w3c.github.io/uievents/#events-focusevent-event-order). Is there a reason you've decided to follow what Chrome did and fire `focus` before `focusin`?

    
    

    
  
Just read up on the discussion happening here: https://github.com/w3c/uievents/issues/88

Looks like the event order is just a total mess everywhere right now. I think that it is important to have a note in the MDN docs or the Release Notes that state that the implementation doesn't follow the spec, and why.

    
    

    
  
(In reply to oatmealsnap from comment #140)
> Just read up on the discussion happening here:
> https://github.com/w3c/uievents/issues/88
> 
> Looks like the event order is just a total mess everywhere right now. I
> think that it is important to have a note in the MDN docs or the Release
> Notes that state that the implementation doesn't follow the spec, and why.

This does sound like a concern. I'm happy to add a note, but I'm not sure what to put. What do the engineers make of this?

    
    

    
  
The order is what it is since no other browser supported what the spec said. (well IE might have been somewhat reasonable, but it is obsolete)

    
  
Depends on: 1347114

    
    

    
  
I'd love to get involved if I can. It seems that either all the browsers should make the shift to align with the spec, or the spec should change to match what the browsers should do. Currently, focus events are a mess, and we should work to clean this up, either by changing Firefox's code to match the spec, or by reaching out to the W3C and attempting to amend the spec.

    
    

    
  
Would you mind filing spec bugs to get the discussion started and post the references here?
Flags: needinfo?(oatmealsnap)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: