Closed Bug 962251 Opened 6 years ago Closed 4 years ago

focus and blur events should have relatedTarget

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: erik, Assigned: kershaw)

References

Details

Attachments

(3 files, 7 obsolete files)

Gecko returns null for the relatedTarget in both blur and focus events

http://www.w3.org/TR/DOM-Level-3-Events/#event-type-blur
http://www.w3.org/TR/DOM-Level-3-Events/#event-type-focus
I check relatedTarget for focus event type in other browser:

- Chrome support relatedTarget for all focus event type
- IE support relatedTarget only for focusin and focusout (blur and focus have null)
- Firefox always return null

This can be correct after resolve https://bugzilla.mozilla.org/show_bug.cgi?id=687787. But correct behavior for event its not clear, specifications D3E and HTML are not compatible (https://www.w3.org/Bugs/Public/show_bug.cgi?id=25279) so don't know if any changes have sense without unification. 

Some test for better understanding:

<!DOCTYPE html>
<html>

<head>

	<style>

		input:focus {outline: 5px solid green;}

	</style>

</head>

<body>

	<p>Change focus at the following inputs (by mouse or tabulator)</p>
	<input id="input1" type="text" value="First input">
	<input id="input2" type="text" value="Second input">
	<input id="input3" type="text" value="Third input">

	<p style="color: blue;">Detailed information for the captured events:</p>
	<p id="info"></p>
	
	<script>
	
		var input1 = document.getElementById("input1");
		var input2 = document.getElementById("input2");
		var info = document.getElementById("info");

		function readInfo(e){

			var data = "Interfejs: " + e
				+ "<br>" + "e.type: " + e.type
				+ "<br>" + "e.target: " + e.target
				+ "<br>" + "e.target.id: " + e.target.id
				+ "<br>" + "e.target.value: " + e.target.value
				+ "<br>" + "e.relatedTarget: " + e.relatedTarget;

			if (e.relatedTarget){
				data += "<br>" + "e.relatedTarget.id: " + e.relatedTarget.id
					+ "<br>" + "e.relatedTarget.value: " + e.relatedTarget.value + "<br><br>";
			}
			else{
				data += "<br><br>";
			}

			info.innerHTML = data + info.innerHTML;

		}

		input1.addEventListener("focus", readInfo, false);
		input2.addEventListener("focus", readInfo, false);
		input3.addEventListener("focus", readInfo, false);

	</script>

</body>

</html>
I think focusin and focusout are not necessary to implement relatedTarget.
relatedTarget is actually the only way to get the "from" or "to" DOM element for focus and blur. That is a very important feature that require ugly code to allow FF in some pages.

In old versions of FF, document.activeElement or explicitOriginalTarget was helpful, but they have been fixed and are now useless to get the relatedTarget, specially for the blur event.

DOM level3 Event:
relatedTarget of type EventTarget, readonly , nullable

    Used to identify a secondary EventTarget related to a Focus event, depending on the type of event.

    For security reasons with nested browsing contexts, when tabbing into or out of a nested context, the relevant EventTarget SHOULD be null.

    The un-initialized value of this attribute MUST be null.
+1 waiting for the fix.
Interestingly, MDN said the property is "implemented" in Fx24.

https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent/relatedTarget

Masayuki-san, is this some kind of misunderstanding? Did we implemented FocusEvent interface without relatedTarget at the time? Should I fix the MDN documentation there?
Flags: needinfo?(masayuki)
Confirmed the property (and doc) was added in bug 855741, and the impl there does not change the way FocusManager creates focus/blur events. The patch there simply make FocusEvent constructor available and relatedTarget properly set when called from content w/ the prop.
Depends on: 855741
Flags: needinfo?(masayuki)
I just tried to create a new `blur` event using the FocusEvent() constructor. In this case the `relatedTarget` property can be properly set.

However the property is still unavailable in events fired automatically (those which are not manually created using the constructor). Is there any progress in the implementation?
I just got bit by this. Works in IE>=9, Chrome, and Edge. MDN reports that firefox supports it.

Workaround is to use Event.explicitOriginalTarget.

Here's my testcase: https://jsfiddle.net/1qnyqt8c/1/
Unfortunately `event.explicitOriginalTarget` is not a comprehensive workaround. In cases where a `blur` event is fired due to tab-cycling through focusable targets, `event.explicitOriginalTarget` will be the same as `event.target`.

Also, +1; this missing feature would be really nice to have. There is an effective work-around in every browser but Firefox, it seems.
+1 to fix this. Working on a enterprise web app for a large, international corporation, and just got bit by this, even though it works fine in IE9+, Chrome, Safari, and Edge.
+1, similar situation to Judah.  Need to do some ugly workarounds until this is fixed...
+1, please fix it
This needs to be fixed ASAP. The things we have to do to work around the lack of relatedTarget in FF are just atrocious, and getting worse all the time because new corner cases pop up all the time.

focusin and focusout need to be implemented as well, they're in the standard for a reason. Just think of implementing focus management for widgets that contain iframes, like contentEditable rich text editors.

To add insult to the injury, Firefox has the best WAI-ARIA implementation to date but *no* proper focus events. :(

Come on guys! Even IE 5.5 had it right!
Hey Enn, I was told that you work on focus-related stuff. Do you mind checking this out to see what the issue might be, and if we can fix it?
Flags: needinfo?(enndeakin)
Well, the issue is that it isn't implemented. You would need to change nsFocusManager::Blur and nsFocusManager::Focus to pass the right element to the event.
Flags: needinfo?(enndeakin)
I don't want to rant, but this bug is very annoying. This makes the blur event unusable to determine if the focus left the application or not. It could just be another node inside the application that got the focus or the focus might have left the application. Since relatedTarget is always null, there is not any reasonable way to figure out what happened until the focus event occurs. But this will never happen if the focus left the application. Do we really have to use timeout() hacks to get this right? At the moment I'm ashamed that FF is the browser with the worst event handling on the field and that this bug is now nearly 2 years old. Nobody seams to care for the foundation?
In Chrome, relatedTarget is null when switching windows and when clicking on a blank area of the same page, so you can't use relatedTarget to detect an application shift anyway.

You could however listen for the focus/blur event on the window instead which, while not specified anywhere, works in Firefox, Chrome and IE.
(In reply to Neil Deakin from comment #16)

That is right. I'm wanted to use the null value to detect if the document lost the focus (e.g. if i click on the address bar). In my case, I wasn't really interested in knowing which "external" component got the focus, but that none of my own components got it.

Simplified example of what i wanted to do:

  document.body.addEventListener('focus', function (event) {
    currentlyFocussedElement = event.target;
  }, true);

  document.body.addEventListener('blur', function (event) {
    if (!event.relatedTarget) {
      currentlyFocussedElement = null; // something else got the focus
    } else {
      // one of my components got the focus, but this is already handled by 'focus'
    }
  }, true);

Since relatedTarget is always null in FF (and only in FF), i simply can't do this. I can listen to blur, but it won't help me to decide if the focus is now outside the document or not. Only listen to focus won't help either, because the event is never fired if the focus is now outside.

To avoid this issue i used the following "hack":

  document.body.addEventListener('focus', function (event) { 
    currentlyFocussedElement = event.target; // stays the same, since target is available
  }, true);

  document.body.addEventListener('blur', function (event) {
    setTimeout(function () {
      var relatedTarget = document.activeElement;
      if (!relatedTarget) {
        currentlyFocussedElement = null;
      }
    }, 1);
  }, true);

It works, but it just feels wrong.
+1 Please fix this, as toggling visual state based on focus of a component or it's children is very hard. I have a component that wants to know if it or any of it's children are focused - with every other browser, on the `blur` I know if it focus is still in my component... except FireFox, and I have to do a hack like above.
+1 --  No excuse for letting this problem persist for over 2 years.
Excuse being the limited resources. I'd be happy to review a patch for this.
Attached patch Add relatedTarget in FocusEvent (obsolete) — Splinter Review
Hi smaug,
Could you take a look a this patch?
Thanks.
Assignee: nobody → kechang
Attachment #8727736 - Flags: review?(bugs)
Attached patch Test case (obsolete) — Splinter Review
Attachment #8727777 - Flags: review?(bugs)
whoohoo! Yes. I'll try to review this during the next couple of days.
I only skimmed this, but I think you'll need to check if relatedTarget is in the same document as the target of the event, and if, not, use null instead.
Comment on attachment 8727777 [details] [diff] [review]
Test case

This is fine but we really need tests when focus is moving from some document A to document B and so. So add some iframe here and focus stuff inside it...

And could you add also some tests where blur() method is called.
And focus() and blur() should be tested also on window object, not only elements.
Attachment #8727777 - Flags: review?(bugs) → review-
Comment on attachment 8727736 [details] [diff] [review]
Add relatedTarget in FocusEvent

So what Neil said, we shouldn't leak information about different documents.
That would be quite easy fix I guess. In case EventTarget QIs to nsINode, get its OwnerDoc() and if it QIs to window object, get its ExtantDoc() and then compare documents, and if not the same, set related target to null.
That comparison should happen right before we dispatch the event.


And a case to test and compare to other browsers:
what happens if in a blur event listener script calls .focus() on some other element than in event.relatedTarget. Which events are fired and what the relatedTargets are?
Attachment #8727736 - Flags: review?(bugs) → review-
Attachment #8727736 - Attachment is obsolete: true
Attachment #8727777 - Attachment is obsolete: true
Attachment #8733772 - Flags: review?(bugs)
Attached patch Test case - v2 (obsolete) — Splinter Review
Attachment #8733773 - Flags: review?(bugs)
> And a case to test and compare to other browsers:
> what happens if in a blur event listener script calls .focus() on some other
> element than in event.relatedTarget. Which events are fired and what the
> relatedTargets are?

This is the test case that based on comment#1.
Test flow:
1. Let input1 get the foucs by clicking it.
2. In input1's blur event listener, call input3.focus().
3. Click input2 and input3 should get the focus.

Result:
Firefox, Chrome, and Safari have the same result.
Event order:
1. Input1 get a blur event. Event.relatedTarget is input2.
2. Input3 get a focus event. Event.relatedTarget is null.

Edge:
1. Input1 get a blur event. Event.relatedTarget is input2.
2. Input2 get a blur event. Event.relatedTarget is input3.
3. Input3 get a focus event. Event.relatedTarget is input2.
4. Input3 get a focus event. Event.relatedTarget is input1.
Comment on attachment 8733773 [details] [diff] [review]
Test case - v2



>+  function Test_FocusEventOnWindow() {
>+    var iframe1 = document.createElement("iframe");
>+    iframe1.id = "iframe1";
>+    iframe1.src = "about:blank";
>+
>+    document.getElementById("content").appendChild(iframe1);
>+    document.getElementById("button2").focus();
>+    var iframe = document.getElementById("iframe");
>+    iframe.contentWindow.addEventListener("focus", function onFocus(aEvent) {
>+      iframe.contentWindow.removeEventListener("focus", onFocus);
>+      ok(aEvent.target === iframe.contentWindow, "iframe is focused.");
and what the relatedTarget is in this case?

>+      runTests();
>+    });
>+    iframe1.contentWindow.addEventListener("focus", function onFocus(aEvent) {
>+      iframe1.contentWindow.removeEventListener("focus", onFocus);
>+      ok(aEvent.target === iframe1.contentWindow, "iframe1 is focused.");
and here

>+      iframe.contentWindow.focus();
>+    });
>+    iframe1.contentWindow.addEventListener("blur", function onBlur(aEvent) {
>+      iframe1.contentWindow.removeEventListener("blur", onBlur);
>+      ok(aEvent.target === iframe1.contentWindow, "iframe1 is not focused.");
and here

Need to also test what .relatedTarget is when listening window level focus event, but focusing actually some element inside that document, for example some <input> element.
Same with blur.

Gecko dispatches a separate event also to Document, but that is a legacy thing which other browsers don't have, but would be good to test also that one so that we don't put anything totally unexpected as relatedTarget.

Focus handling is tricky, so better to have good tests.
Attachment #8733773 - Flags: review?(bugs) → review-
Comment on attachment 8733772 [details] [diff] [review]
Add relatedTarget in FocusEvent - v2

>+
>+  if (aField.mRelatedTarget) {
>+    CycleCollectionNoteChild(aCallback, aField.mRelatedTarget.get(), aName, aFlags);
CycleCollectionNoteChild should be null safe, so now null check


>+namespace {
>+
>+nsIDocument* GetDocumentHelper(EventTarget* aTarget)
I'd prefer static method here, that is still what coding style suggests to use
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Anonymous_namespaces


>+{
>+  if (!aTarget) {
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsINode> node = do_QueryInterface(aTarget);
QI is null safe, so you don't need to null check aTarget


> nsFocusManager::SendFocusOrBlurEvent(EventMessage aEventMessage,
>                                      nsIPresShell* aPresShell,
>                                      nsIDocument* aDocument,
>                                      nsISupports* aTarget,
>                                      uint32_t aFocusMethod,
>                                      bool aWindowRaised,
>-                                     bool aIsRefocus)
>+                                     bool aIsRefocus,
>+                                     nsISupports* aRelatedTarget)
Couldn't the last param be nsIContent*, or perhaps EventTarget*


> {
>   NS_ASSERTION(aEventMessage == eFocus || aEventMessage == eBlur,
>                "Wrong event type for SendFocusOrBlurEvent");
> 
>   nsCOMPtr<EventTarget> eventTarget = do_QueryInterface(aTarget);
>+  nsCOMPtr<EventTarget> relatedTarget = do_QueryInterface(aRelatedTarget);
...then you wouldn't need to QI here.

>+  bool dontDispatchEvent
>+    = eventTargetDoc && nsContentUtils::IsUserFocusIgnored(eventTargetDoc);
Nit, I'd put = to previous line


r+, but you may find some issues after testing focus events dispatched to window object.
And when testing, remember that focus events don't bubble, so use capturing listeners.
Attachment #8733772 - Flags: review?(bugs) → review+
Address last review comments and carry r+.

I didn't find any issue in my test. In this patch, event.relatedTarget is null when dispatching to window and document.
Hope that I was not wrong.
Attachment #8733772 - Attachment is obsolete: true
Attachment #8735399 - Flags: review+
Attached patch Test case - v3 (obsolete) — Splinter Review
Add test case mentioned in comment#30.
Attachment #8733773 - Attachment is obsolete: true
Attachment #8735400 - Flags: review?(bugs)
Comment on attachment 8735400 [details] [diff] [review]
Test case - v3

>+  function Test_ListenFocusBlurEventOnWindow1() {
>+    var button2 = document.getElementById("button2");
>+    button2.focus();
>+
>+    var iframe = document.getElementById("iframe");
>+    var input = iframe.contentDocument.getElementById("textinput");
>+    var expectedEventTarget = [iframe.contentDocument, iframe.contentWindow, input];

You could listen blur event on the initial window (in capture phase) and ensure the expected events with right
.relatedTarget are get there.

>+    iframe.contentWindow.addEventListener("focus", function onFocus(aEvent) {
>+      var item = expectedEventTarget.shift();
>+      ok(aEvent.target === item, "Get a expected focus event.");
s/a/an/



>+  function Test_ListenFocusBlurEventOnWindow2() {
>+    var iframe = document.getElementById("iframe");
>+    var input = iframe.contentDocument.getElementById("textinput");
>+    var input1 = iframe.contentDocument.getElementById("textinput1");
>+
>+    ok(iframe.contentDocument.activeElement === input, "Current focused element should be input.");
>+    iframe.contentWindow.addEventListener("focus", function onFocus(aEvent) {
>+      iframe.contentWindow.removeEventListener("focus", onFocus, true);
>+      ok(aEvent.target === input1, "Input1 is focused.");
>+      ok(aEvent.relatedTarget === input, "relatedTarget should be input.");
>+      runTests();
>+    }, true);
>+    iframe.contentWindow.addEventListener("blur", function onBlur(aEvent) {
>+      iframe.contentWindow.removeEventListener("blur", onBlur, true);
>+      ok(aEvent.target === input, "Input is not focused.");
>+      ok(aEvent.relatedTarget === input1, "relatedTarget should be input1.");
>+    }, true);
>+
>+    input1.focus();
>+  }
Could you also add similar test to this, but not focus() anything but just explicitly call .blur() or some input element which is currently activeElement

those fixed, r+.
Attachment #8735400 - Flags: review?(bugs) → review+
rebase
Attachment #8735399 - Attachment is obsolete: true
Attachment #8735400 - Attachment is obsolete: true
Attachment #8735401 - Attachment is obsolete: true
Attachment #8736193 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/60a67e654ae7
https://hg.mozilla.org/mozilla-central/rev/31397f0f3154
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.