Bug 179990 - Phantom focus/blur events fire on clicking between text input fields when listening with addEventListener
Summary: Phantom focus/blur events fire on clicking between text input fields when lis...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: Safari 11
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-23 19:30 PST by Chris Lienert
Modified: 2020-03-10 23:23 PDT (History)
10 users (show)

See Also:


Attachments
Fixes the bug (7.17 KB, patch)
2018-11-21 18:37 PST, Ryosuke Niwa
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lienert 2017-11-23 19:30:18 PST
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
Comment 1 Radar WebKit Bug Importer 2017-11-24 22:53:58 PST
<rdar://problem/35685975>
Comment 2 Ryosuke Niwa 2018-11-20 17:11:54 PST
Hm... I can't reproduce this on 12.0.1 or STP70.
Comment 3 Ryosuke Niwa 2018-11-20 17:29:34 PST
Can't reproduce on Safari 11.1 (13605.1.33.1.4) either.
Comment 4 Ryosuke Niwa 2018-11-20 17:46:14 PST
Oh, that's because I always disable force click. I can reproduce reliably now.
Comment 5 Ryosuke Niwa 2018-11-20 19:41:13 PST
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.
Comment 6 Tim Horton 2018-11-20 23:06:05 PST
(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?
Comment 7 Ryosuke Niwa 2018-11-20 23:12:22 PST
(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.
Comment 8 Tim Horton 2018-11-20 23:18:40 PST
(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.
Comment 9 Ryosuke Niwa 2018-11-21 18:37:23 PST
Created attachment 355449 [details]
Fixes the bug
Comment 10 Tim Horton 2018-11-21 19:14:42 PST
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".
Comment 11 Tim Horton 2018-11-21 19:16:02 PST
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.
Comment 12 Ryosuke Niwa 2018-11-21 22:26:10 PST
(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.
Comment 13 Ryosuke Niwa 2018-11-21 22:38:14 PST
(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!
Comment 14 Ryosuke Niwa 2018-11-21 22:39:17 PST
Committed r238440: <https://trac.webkit.org/changeset/238440>
Comment 15 Daniel Bates 2018-11-21 23:11:51 PST
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.
Comment 16 Ryosuke Niwa 2018-11-21 23:42:27 PST
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.
Comment 17 Daniel Bates 2018-11-21 23:49:20 PST
(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 & :/
Comment 18 fellaslimspam 2019-02-28 22:14:06 PST
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.
Comment 19 Ryosuke Niwa 2019-03-01 11:06:20 PST
(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.
Comment 20 Mark 2019-04-08 14:51:32 PDT
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
Comment 21 Ryosuke Niwa 2019-04-11 01:24:51 PDT
(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?
Comment 22 David 2019-07-15 09:01:40 PDT
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>
Comment 23 David 2019-07-15 09:07:19 PDT
(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.
Comment 24 Ryosuke Niwa 2020-03-10 23:23:51 PDT
(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?