Steps to reproduce:
The URL field won't be selected anymore. It takes an extra click or three tab key presses to select the field, disrupting editing flow. If the user does not notice and tries to paste from the clipboard, the citation dialog gets cancelled, which can be confusing.
Seen in latest Chrome.
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Don't focus lookupInput when it isn't visible | mediawiki/extensions/Citoid | master | +7 -5 |
I can reproduce. I tried debugging this and I don't understand why it happens – we run the same code in both cases that should focus the field, but it doesn't get focussed in the second case. Also, the bug seemed to disappear when I placed breakpoints to debug it :)
The next step is probably to bisect to find out where the bug happened. We don't actually know if it's a problem with VE, Citoid, OOUI, or something else, so that will be a bit annoying (unless you get lucky). It works correctly on a REL1_35 wiki I created on Patchdemo (https://patchdemo.wmflabs.org/wikis/c3d72ef0fe/), and does not work correctly on master (https://patchdemo.wmflabs.org/wikis/d25b4b8cc1/), so the bug must have been introduced at some time after that.
It is broken by REL1_36 (https://patchdemo.wmflabs.org/wikis/34a662723b/wiki/Main_Page?veaction=edit), so the regression occurred sometime between 1.35 and 1.36.
I tried selective rolling back extensions and libraries to 1_35. Turns out it appears to be the jQuery upgrade (from 3.4.1 to 3.6.0) that is to blame.
The 3.6.0 release notes talk about a bug related to triggering a focus handler within a focus handler, so that's where my money is: https://blog.jquery.com/2021/03/02/jquery-3-6-0-released/
Edit: Reverting this change in jQuery fixes it, so it seems that update is to blame: https://github.com/jquery/jquery/pull/4813/files
CC @Krinkle
I'm not quite sure of the pros/cons of using .trigger( 'focus' ) vs just calling htmlelement.focus(). Using the latter makes this bug go away, but may just be sidestepping this root cause. Currently in OOUI we use trigger in 9 places and the direct native call in 1.
As I understand it, jQuery generally tries to approximate the standards compliant browser behaviour and do so across browsers. With events in particular, this is tricky because the mere act of wrapping the DOM logic at all creates a difference in behaviour, namely that there are some event loop mechanics that one can't easily break out of.
When an element receives focus, the focus events are fired, and after the event handlers are processed, the default behaviour will generally be applied by the browser (unless the default was "prevented"). Where this gets tricky, is if a new event is fired programmatically from within another event handler because that means the native event propagation and jQuery's emulation are both running to competion separately.
The native behaviour is that when a focus event handler moves focus elsewhere, then the last focus call will be honoured in terms of final render state, and all intermediaries are also run to completion recursively within that call, including applying of default behaviour between the inner event handler and the outer event handling.
jQuery 3.6 behaves the same way as the standard, which can be seen from this test case, where with jQuery 3.4 the focus is lost (kind of Cite in this bug report), and with jQuery 3.6 it preserves and forwards the focus correctly:
https://codepen.io/Krinkle/pen/RwZoXxJ?editors=1000
If jQuery simply fired its own "on" callbacks and then deferred to native, then the first elememt to start the chain would win instead of the last one as expected, because after all recursion is done from the first one and the later ones, the last thing that would happen is the native behaviour for the first one. This was fixed in 3.6.0 and thus the same logic written in native DOM vs jQuery now behave the same way.
When the two are mixed, behaviour is more or less undefined.
I was able to reproduce the bug described in this task on the en.wikipedia.org Sandbox article. However, I was not able to reproduce the issue in isolation on CodePen. I suspect OOUI/VE is likely relying on something unsupported here, but I can't tell for sure unless I'm looking at an isolated test case where it exhibits the currently seen bug.
As for why it only happens on the second call, my guess is that on the first call there is some asynchronous setup happening and thus the focus is in its own event loop tick later and this isn't competing with other focus code, and during subsequent calls it is already provisioned internally and calling it synchronously at which point it is competing and losing out.
Change 805204 had a related patch set uploaded (by Esanders; author: Esanders):
[mediawiki/extensions/Citoid@master] Don't focus lookupInput when it isn't visible
@Krinkle See the above fix for the root cause. This should make it reproduceable outside of VE, and maybe of interest to the jQuery team.
Minimal codepen: https://codepen.io/edg2s/pen/eYVbLOj
After making this I discovered that it was reported upstream around the same time we stopped looking at this: https://github.com/jquery/jquery/issues/4950
Patch was merged (Phabricator was down for a few minutes, so the automatic notification didn't go through)