Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

You cannot programmatically focus an element after you have attempted to focus it while it was hidden (display:none) #4950

Closed
JoolsCaesar opened this issue Oct 11, 2021 · 13 comments · Fixed by #5223
Milestone

Comments

@JoolsCaesar
Copy link

JoolsCaesar commented Oct 11, 2021

Description

Possibly related to #4867, I have replicated this in version 3.5.1 and 3.6. The linked example below uses 3.6. I haven't tested Firefox, but both Chrome and Edge display this issue.

See the following example. There's a focusable div at the top, which has a handler to report (at the bottom of the example) the number of times it is focused. When you click the div, it is only ever focused programmatically, because a handler calls preventDefault() on the mousedown.

There is a button to toggle the visibility (display) of the focusable element, but before showing the element, it attempts to programmatically focus it.

Focusing a display:none element is not expected to work, but the issue is what follows.

Once the element is shown, no future attempt to focus the element will work.

HTML:

  <div id="hideable">
    <div id="focusable" tabindex="1">
      I am focusable
    </div>
  </div>
  <button id="button">
    Toggle visibility
  </button>
  <div id="output">
    Click "I am focusable" to focus. Click "Toggle visibility" to hide/show the focusable element
  </div>

JS:

const $button = $('#button');
const $focusable = $('#focusable');
const $hideable = $('#hideable');
const $output = $('#output');

let focusinCount = 0;
$hideable.on('focusin', function() {
  focusinCount++;
  $output.text('I felt that ' + focusinCount);
});

let visible = true;
$button.click(function() {
  if (visible) {
    $hideable.hide();
  } else {
    $focusable.focus(); // Comment out this to fix
    $hideable.show();
  }
  visible = !visible;
});

$focusable.on('mousedown', function(event) {
  event.preventDefault();
  $focusable.focus();
})

Link to test case

https://jsfiddle.net/JoolsCaesar/2o089jga/

@mgol
Copy link
Member

mgol commented Oct 11, 2021

Thanks for the report. This looks like a valid issue.

@mgol mgol added this to the 3.6.1 milestone Oct 11, 2021
@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Oct 11, 2021
@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Oct 18, 2021
@jwmerrill
Copy link

jwmerrill commented Nov 10, 2021

I have bisected this problem to fe5f04d using the provided test case.

@jwmerrill
Copy link

It appears that commenting out this line

event.stopPropagation();

fixes the test case, but presumably that would cause other problems.

jwmerrill added a commit to desmosinc/mathquill that referenced this issue Nov 12, 2021
jQuery 3.x has introduced a bunch of different focus bugs in the process
of attempting to migrate to using native focus events in more cases,
some of which remain unfixed in the latest version (3.6.0).

Examples:
* jquery/jquery#4859
* jquery/jquery#4856
* jquery/jquery#4950
* jquery/jquery#4867

Some unknown bug in this general class can make it so that Mathquill
public api focus calls stop working because using jQuery to
programatically trigger focus breaks.

Work around this by switching to triggering native focus events instead
of jQuery focus events.
jwmerrill added a commit to desmosinc/mathquill that referenced this issue Nov 16, 2021
jQuery 3.x has introduced a bunch of different focus bugs in the process
of attempting to migrate to using native focus events in more cases,
some of which remain unfixed in the latest version (3.6.0).

Examples:
* jquery/jquery#4859
* jquery/jquery#4856
* jquery/jquery#4950
* jquery/jquery#4867

Some unknown bug in this general class can make it so that Mathquill
public api focus calls stop working because using jQuery to
programatically trigger focus breaks.

Work around this by switching to triggering native focus events instead
of jQuery focus events.
@fullermetric
Copy link

Thanks for this Joels.
The issue became more apparent after adding #focusable:focus { outline: 3px solid green; } to your test case.

Came here in an attempt to resolve some instances where using .focus() generates a keyup code of 13 (version 3.3.1)
In an attempt to resolve keycode generating on .focus I switched out 3.3.1 and tried jQuery versions 3.5 and 3.6 but found that .focus was not working at all ( as you pointed our why in your test case).

In the end, I switched back to 3.31 and now using .focus inside of a timeout

$(".tabs-stage #tab-1").addClass('hide'); $(".tabs-stage #tab-2").removeClass('hide'); $(".tabs-stage #tab-3").addClass('hide'); //fmp.triggeredOrdQtyFocus = 1; //$("#product-1").find("#product-qty").focus(); setTimeout(function(){ $("#product-1").find("#product-qty").focus(); },200);

@jquery jquery deleted a comment Apr 11, 2022
@jquery jquery deleted a comment Apr 11, 2022
joubu pushed a commit to Koha-Community/Koha that referenced this issue May 19, 2022
…ield

This patch updates the staff interface's global JavaScript to
accommodate changes in the way focus is being handled after the jQuery
upgrade (see: jquery/jquery#4950).

The "focus" class is removed from search header include files so that
there isn't a contradiction between which form field has the focus class
and which form field is displayed in the active tab.

To test, apply the patch and view various pages in the staff interface.

- On pages where focus is not being directed to a form field within the
  main content of the page, the form field in the active search header
  tab should have focus on page load:

  - Patron details
  - System preferences
  - Cities and towns

  Also test pages where a tab other than the first one is preselected:

  - Bibliographic details page
  - Patron lists

  On these pages, focus should move to the active tab's form field when
  you switch tabs.

- On pages where focus is being sent to another form field, it should
  work correctly:

  - Patrons home page
  - Check in
  - Acquisitions home page

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
@edg2s
Copy link
Contributor

edg2s commented Jun 13, 2022

We discovered this same issue at Wikipedia (upstream issue: https://phabricator.wikimedia.org/T285626, minimal codepen: https://codepen.io/edg2s/pen/eYVbLOj)

This occurs in one dialog that we know of, but the nature of our dialogs means they are regularly hiding and showing content, and moving user focus, so accidentally focusing a hidden input is probably happening elsewhere.

@JoolsCaesar
Copy link
Author

If anyone is desperate for a central workaround, you can use the following to ignore calls to focus() on hidden elements

var origFocus = $.fn.focus;
$.fn.focus = function(){
   if(arguments.length === 0 && !this.is(':visible')){
      return this;
   }else{
      return origFocus.apply(this, arguments);
   }
};

Disclaimer: Use at your own risk. Probably won't work if you have calls to .trigger('focus'), but you're an adult; figure it out.

@edg2s
Copy link
Contributor

edg2s commented Jun 14, 2022

Event shorthands like $.fn.focus are deprecated, so trigger should be used instead. Internally focus will call trigger so if you patch trigger instead you will fix both methods.

@JoolsCaesar
Copy link
Author

Does the API documentation need updating for that? I can't see any mention of it in the docs for focus or the deprecated functions page, though funnily enough, it does say that you should be careful to not call it on hidden elements for the sake of IE

Attempting to set focus to a hidden element causes an error in Internet Explorer. Take care to only use .focus() on elements that are visible.

https://api.jquery.com/focus/
https://api.jquery.com/category/deprecated/

@edg2s
Copy link
Contributor

edg2s commented Jun 14, 2022

Probably! I've found the documentation on deprecations to be quite inconsistent. The deprecation of shorthands are mentioned in the release notes for 3.5: https://blog.jquery.com/2020/04/10/jquery-3-5-0-released/

P.S. I maintain an ESLint plugin for jQuery, part of which is configs targetting announced deprecations by version: https://github.com/wikimedia/eslint-plugin-no-jquery/blob/master/src/index.js#L128 - that might be a good place to scan if you are auditing the deprecation docs.

P.P.S Another useful link: https://github.com/jquery/api.jquery.com/issues

@edg2s
Copy link
Contributor

edg2s commented Jun 14, 2022

Updated workaround using trigger:

var origTrigger = $.fn.trigger;
$.fn.trigger = function () {
	if ( arguments[ 0 ] === 'focus' ) {
		var args = arguments;
		return this.each( function () {
			var $this = $( this );
			if ( $this.is( ':visible' ) ) {
				origTrigger.apply( $this, args );
			}
		} );
	} else {
		return origTrigger.apply( this, arguments );
	}
};

@JoolsCaesar
Copy link
Author

I maintain an ESLint plugin for jQuery
Nice, I'll check that out. Thanks!

@jquery jquery deleted a comment Jul 8, 2022
wmfgerrit pushed a commit to wikimedia/mediawiki that referenced this issue Jul 11, 2022
An upstream bug in jQuery means this won't work and will
break future attempts to focus it too:
jquery/jquery#4950

Bug: T312770
Change-Id: I13921b7e9cec0a5a2bbce1cf68db843754fb96de
@timmywil timmywil modified the milestones: 3.6.1, 3.7.0 Aug 15, 2022
@mgol
Copy link
Member

mgol commented Mar 16, 2023

Interesting; it looks like my PR #5223 fixes both test cases shared in this issue. Perhaps some edge cases were hitting the code path intended mostly for IE?

mgol added a commit to mgol/jquery that referenced this issue Mar 17, 2023
In IE (all versions), `focus` & `blur` handlers are fired asynchronously
but `focusin` & `focusout` are run synchronously. In other browsers, all
those handlers are fired synchronously.

Simulate `focus` via `focusin` & `blur` via `focusout` in IE to
avoid these issues.

Fixes jquerygh-4856
Fixes jquerygh-4859
Fixes jquerygh-4950
mgol added a commit to mgol/jquery that referenced this issue Mar 17, 2023
Focusing an input with `display: none` no longer prevents it from
being focused when it gets shown later.
mgol added a commit to mgol/jquery that referenced this issue Mar 17, 2023
In IE (all versions), `focus` & `blur` handlers are fired asynchronously
but `focusin` & `focusout` are run synchronously. In other browsers, all
those handlers are fired synchronously.

Simulate `focus` via `focusin` & `blur` via `focusout` in IE to
avoid these issues.

Fixes jquerygh-4856
Fixes jquerygh-4859
Fixes jquerygh-4950
mgol added a commit to mgol/jquery that referenced this issue Mar 17, 2023
Focusing an input with `display: none` no longer prevents it from
being focused when it gets shown later.
mgol added a commit to mgol/jquery that referenced this issue Mar 20, 2023
In IE (all versions), `focus` & `blur` handlers are fired asynchronously
but `focusin` & `focusout` are run synchronously. In other browsers, all
those handlers are fired synchronously.

Simulate `focus` via `focusin` & `blur` via `focusout` in IE to
avoid these issues.

Fixes jquerygh-4856
Fixes jquerygh-4859
Fixes jquerygh-4950
mgol added a commit to mgol/jquery that referenced this issue Mar 20, 2023
Focusing an input with `display: none` no longer prevents it from
being focused when it gets shown later.
mgol added a commit to mgol/jquery that referenced this issue Mar 20, 2023
In IE (all versions), `focus` & `blur` handlers are fired asynchronously
but `focusin` & `focusout` are run synchronously. In other browsers, all
those handlers are fired synchronously.

Simulate `focus` via `focusin` & `blur` via `focusout` in IE to
avoid these issues.

Fixes jquerygh-4856
Fixes jquerygh-4859
Fixes jquerygh-4950
mgol added a commit to mgol/jquery that referenced this issue Mar 20, 2023
Focusing an input with `display: none` no longer prevents it from
being focused when it gets shown later.
mgol added a commit that referenced this issue Mar 27, 2023
In IE (all versions), `focus` & `blur` handlers are fired asynchronously
but `focusin` & `focusout` are run synchronously. In other browsers, all
those handlers are fired synchronously. Asynchronous behavior of these
handlers in IE caused issues for IE (gh-4856, gh-4859).

We now simulate `focus` via `focusin` & `blur` via `focusout` in IE to avoid
these issues. This also let us simplify some tests.

This commit also simplifies `leverageNative` - with IE now using `focusin`
to simulate `focus` and `focusout` to simulate `blur`, we don't have to deal
with async events in `leverageNative`. This also fixes broken `focus` triggers
after first triggering it on a hidden element - previously, `leverageNative`
assumed that the native `focus` handler not firing after calling the native 
`focus` method meant it would be handled later, asynchronously, which
was not the case (gh-4950).

Fixes gh-4856
Fixes gh-4859
Fixes gh-4950
Closes gh-5223

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
mgol added a commit that referenced this issue Mar 27, 2023
In IE (all versions), `focus` & `blur` handlers are fired asynchronously
but `focusin` & `focusout` are run synchronously. In other browsers, all
those handlers are fired synchronously. Asynchronous behavior of these
handlers in IE caused issues for IE (gh-4856, gh-4859).

We now simulate `focus` via `focusin` & `blur` via `focusout` in IE to avoid
these issues. This also let us simplify some tests.

This commit also simplifies `leverageNative` - with IE now using `focusin`
to simulate `focus` and `focusout` to simulate `blur`, we don't have to deal
with async events in `leverageNative`. This also fixes broken `focus` triggers
after first triggering it on a hidden element - previously, `leverageNative`
assumed that the native `focus` handler not firing after calling the native 
`focus` method meant it would be handled later, asynchronously, which
was not the case (gh-4950).

To preserve relative `focusin`/`focus` & `focusout`/`blur` event order
guaranteed on the 3.x branch, attach a single handler for both events in IE.

A side effect of this is that to reduce size the `event/focusin` module
no longer exists and it's impossible to disable the `focusin` patch
in modern browsers via the jQuery custom build system.

Fixes gh-4856
Fixes gh-4859
Fixes gh-4950
Ref gh-5223
Closes gh-5224

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@mgol
Copy link
Member

mgol commented Mar 27, 2023

Fixed on main (future 4.x) via #5223 and on 3.x-stable via #5224.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants