Phantom focus/blur events fire on clicking between text input fields when listening with addEventListener

• Component: UI Events
• Status: RESOLVED FIXED

[Chris Lienert]

With two or more text inputs on a form, switching focus by click from one to another sometimes causes an extra focus/blur to fire.

Steps to reproduce

Start with a form with two or more text inputs and watch focus/blur events.
Demo: https://codepen.io/cliener/pen/ooGpwW

1. Click on one of the text inputs (#1)
2. Click on another text input (#2)
3. Repeat the above until extra focus/blur events show.

The phantom events only seem to fire on the second and subsequent text inputs.


Expected behaviour

Each clicked focus shift should fire single blur/focus events.
Element #1 blur
Element #2 focus
Element #2 click


Actual behaviour

Some clicked focus shifts fire secondary blur/focus events with the net effect of:
Element #1 blur
Element #2 focus
Element #1 blur
Element #2 focus
Element #2 click


Additional notes

Monitoring the events in the console with monitorEvents() does not show the phantom events. The extra events only appear to be visible when listening to the DOM focus/blur events with addEventListener.

Disabling "Look up & data detectors" (System Preferences: Trackpad: Point & Click: Look up & data detectors) fixes the bug.

Tested and recreated in:
- Safari Version 11.0.1 (13604.3.5) on MacOS 10.13.1 (17B48)
- Safari Technology Preview Release 44 (Safari 11.1, WebKit 13605.1.13.2)


Impact of this bug

Custom form elements which depend on focus/blur can get stuck in a loop as demonstrated in React Datepicker https://github.com/Hacker0x01/react-datepicker/issues/1077
This bug has also been noted in React where we're trying to find a workaround https://github.com/facebook/react/issues/10871

[Radar WebKit Bug Importer]

<rdar://problem/35685975>

[Ryosuke Niwa]

Hm... I can't reproduce this on 12.0.1 or STP70.

[Ryosuke Niwa]

Can't reproduce on Safari 11.1 (13605.1.33.1.4) either.

[Ryosuke Niwa]

Oh, that's because I always disable force click. I can reproduce reliably now.

[Ryosuke Niwa]

The issue here is that TemporarySelectionChange which is created by TextIndicator::createWithRange is setting & restoring the selection, which also sets & restores the focus. I'd have to talk with Beth / Tim to figure out what we ought to do instead.

[Tim Horton]

(In reply to Ryosuke Niwa from comment #5)
> The issue here is that TemporarySelectionChange which is created by
> TextIndicator::createWithRange is setting & restoring the selection, which
> also sets & restores the focus. I'd have to talk with Beth / Tim to figure
> out what we ought to do instead.

To get rid of that selection change, we'd need all of the paint-selection-only painting code to take an arbitrary range instead of always looking at the selection. I think that would be pretty nontrivial.

Instead ideally we'd just stop the events from propagating, I think?

[Ryosuke Niwa]

(In reply to Tim Horton from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > The issue here is that TemporarySelectionChange which is created by
> > TextIndicator::createWithRange is setting & restoring the selection, which
> > also sets & restores the focus. I'd have to talk with Beth / Tim to figure
> > out what we ought to do instead.
> 
> To get rid of that selection change, we'd need all of the
> paint-selection-only painting code to take an arbitrary range instead of
> always looking at the selection. I think that would be pretty nontrivial.
> 
> Instead ideally we'd just stop the events from propagating, I think?

We can just avoid firing of selection related events & avoid changing that focus. Would moving the focus important here? If not, FrameSelection::setSelection takes an option to not set focus.

[Tim Horton]

(In reply to Ryosuke Niwa from comment #7)
> (In reply to Tim Horton from comment #6)
> > (In reply to Ryosuke Niwa from comment #5)
> > > The issue here is that TemporarySelectionChange which is created by
> > > TextIndicator::createWithRange is setting & restoring the selection, which
> > > also sets & restores the focus. I'd have to talk with Beth / Tim to figure
> > > out what we ought to do instead.
> > 
> > To get rid of that selection change, we'd need all of the
> > paint-selection-only painting code to take an arbitrary range instead of
> > always looking at the selection. I think that would be pretty nontrivial.
> > 
> > Instead ideally we'd just stop the events from propagating, I think?
> 
> We can just avoid firing of selection related events & avoid changing that
> focus. Would moving the focus important here? If not,
> FrameSelection::setSelection takes an option to not set focus.

No, I don't think moving the focus is important.

[Ryosuke Niwa]

Created attachment 355449 [details]
Fixes the bug

[Tim Horton]

Comment on attachment 355449 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=355449&action=review

> Source/WebCore/editing/Editor.h:107
> +    DoNotSetFcous = 1 << 1,

Typo "Fcous".

[Tim Horton]

Comment on attachment 355449 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=355449&action=review

> Source/WebCore/ChangeLog:15
> +        Unfortunately, no new tests since force click testing is broken :( See <rdar://problem/31301721>.

There are other ways to generate textindicators, though (find-in-page, lookup, some kinds of drags; see TextIndicator::createWithSelection...). You might be able to get one of them to work.

[Ryosuke Niwa]

(In reply to Tim Horton from comment #11)
> Comment on attachment 355449 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355449&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Unfortunately, no new tests since force click testing is broken :( See <rdar://problem/31301721>.
> 
> There are other ways to generate textindicators, though (find-in-page,
> lookup, some kinds of drags; see TextIndicator::createWithSelection...). You
> might be able to get one of them to work.

But those either move the focus first or don't move the focus.

[Ryosuke Niwa]

(In reply to Tim Horton from comment #10)
> Comment on attachment 355449 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355449&action=review
> 
> > Source/WebCore/editing/Editor.h:107
> > +    DoNotSetFcous = 1 << 1,
> 
> Typo "Fcous".

Fixed.

Thanks for the review!

[Ryosuke Niwa]

Committed r238440: <https://trac.webkit.org/changeset/238440>

[Daniel Bates]

Comment on attachment 355449 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=355449&action=review

> Source/WebCore/editing/Editor.cpp:209
> +    if (options & TemporarySelectionOption::EnableAppearanceUpdates)

We should make option an OptionSet.

> Source/WebCore/editing/Editor.cpp:227
> +    if (m_options & TemporarySelectionOption::IgnoreSelectionChanges) {

We should make m_option an OptionSet.

[Ryosuke Niwa]

Comment on attachment 355449 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=355449&action=review

>> Source/WebCore/editing/Editor.cpp:209
>> +    if (options & TemporarySelectionOption::EnableAppearanceUpdates)
> 
> We should make option an OptionSet.

options is totally an OptionSet.

>> Source/WebCore/editing/Editor.cpp:227
>> +    if (m_options & TemporarySelectionOption::IgnoreSelectionChanges) {
> 
> We should make m_option an OptionSet.

It IS an OptionSet.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #16)
> Comment on attachment 355449 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355449&action=review
> 
> >> Source/WebCore/editing/Editor.cpp:209
> >> +    if (options & TemporarySelectionOption::EnableAppearanceUpdates)
> > 
> > We should make option an OptionSet.
> 
> options is totally an OptionSet.
> 
> >> Source/WebCore/editing/Editor.cpp:227
> >> +    if (m_options & TemporarySelectionOption::IgnoreSelectionChanges) {
> > 
> > We should make m_option an OptionSet.
> 
> It IS an OptionSet.

Cool. I guess I was looking for .contains() over & :/

This bug is still present in Safari Version 12.0.3 (14606.4.5).
Tested on macOS Mojave Version 10.14.3 (18D109) MacBook Pro (Retina, 13-inch, Early 2015)

Test case:
Two inputs with a value (entered by user or directly set).
1) Click on the first input field. Results in 1 focus event on the first input.
2) Click on the text within the second input field. Results in 3 focus events (second input, first input, second input)
3) Click beside the text in the first input. Results in 1 focus event on the first input.

The bug is persistent. 
Disabling "Look up & data detectors" in "System Preferences -> Trackpad" fixes the issue.

[Ryosuke Niwa]

(In reply to fellaslimspam from comment #18)
> This bug is still present in Safari Version 12.0.3 (14606.4.5).
> Tested on macOS Mojave Version 10.14.3 (18D109) MacBook Pro (Retina,
> 13-inch, Early 2015)

That's expected because the fix didn't go into 18D109. Please test it on macOS 10.14.4 and iOS 12.2 betas instead.

In general, Apple tends to (at least it has been happening for the last couple of years) include the latest version of WebKit in a major OS update and each OS's respective E-releases. Software updates between major "A" releases and "E" releases such as B, C, and D updates tend not to get all the bug fixes in WebKit.

[Mark]

I'm experiencing the double focus bug.
I have updated to 
OS X 10.14.4 (18E226)
and Safari Version 12.1 (14607.1.40.1.4)

The bug is also in
Safari Technology Preview
Release 79 (Safari 12.2, WebKit 14608.1.14

[Ryosuke Niwa]

(In reply to Mark from comment #20)
> I'm experiencing the double focus bug.
> I have updated to 
> OS X 10.14.4 (18E226)
> and Safari Version 12.1 (14607.1.40.1.4)
> 
> The bug is also in
> Safari Technology Preview
> Release 79 (Safari 12.2, WebKit 14608.1.14

Oh, interesting. Can you reproduce it using the example in comment #0? If so, would you mind what your settings are in System Preferences > Trackpad? Do you have force click & look up enabled?

If not, could you give us reproduction steps?

[David]

OSX 10.14.5 (18F132)
Safari 12.1.1 (14607.2.6.1.1)
I have the same issue, when i switch to tab it's fire event 'focus' twice, instead of once.
You can run following html to test:

<html>
<div>Times focus called: </div>
<div id="focustimes">0</div>
</html>
<script>
var i = 0;
window.addEventListener('focus', function(e){
    i = i +1;
    document.getElementById("focustimes").innerHTML = i;
})

</script>

[David]

(In reply to David from comment #22)
> OSX 10.14.5 (18F132)
> Safari 12.1.1 (14607.2.6.1.1)
> I have the same issue, when i switch to tab it's fire event 'focus' twice,
> instead of once.
> You can run following html to test:
> 
> <html>
> <div>Times focus called: </div>
> <div id="focustimes">0</div>
> </html>
> <script>
> var i = 0;
> window.addEventListener('focus', function(e){
>     i = i +1;
>     document.getElementById("focustimes").innerHTML = i;
> })
> 
> </script>

I just tried in Safari beta 13.0 (14608.1.32), it has same bug.

[Ryosuke Niwa]

(In reply to David from comment #23)
> (In reply to David from comment #22)
> > OSX 10.14.5 (18F132)
> > Safari 12.1.1 (14607.2.6.1.1)
> > I have the same issue, when i switch to tab it's fire event 'focus' twice,
> > instead of once.
> > You can run following html to test:
> > 
> > <html>
> > <div>Times focus called: </div>
> > <div id="focustimes">0</div>
> > </html>
> > <script>
> > var i = 0;
> > window.addEventListener('focus', function(e){
> >     i = i +1;
> >     document.getElementById("focustimes").innerHTML = i;
> > })
> > 
> > </script>
> 
> I just tried in Safari beta 13.0 (14608.1.32), it has same bug.

Is this still happening in the latest betas of Safari 13.1? If so, could you file a new bug?