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

Fix unexpected ViewFocus events when Text Editing utilities change focus in the middle of a blur call. #54965

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

tugorez
Copy link
Contributor

@tugorez tugorez commented Sep 5, 2024

In some cases, text editing utilities re-focus the <input /> element during a blur event. This causes an unusual sequence of focusin and focusout events, leading to the engine sending unintended events.

Consider the following HTML code:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>
  <div id="container">
    <input type="" value="1" id="input1">
    <input type="" value="2" id="input2">
    <input type="" value="3" id="input3">
  </div>

  <script>
    container.addEventListener('focusin', (ev) => {
      console.log('focusin: focus was gained by', ev.target);
    });
    container.addEventListener('focusout', (ev) => {
      console.log('focusout: focus is leaving', ev.target, 'and it will go to', ev.relatedTarget);
    });
  </script>
</body>
</html>

Clicking input1, then input2, then input3 produces the following console logs:

// Input1 is clicked
focusin: focus was gained by <input type value=​"1" id=​"input1">​

// Input2 is clicked
focusout: focus is leaving <input type value=​"1" id=​"input1">​ and it will go to <input type value=​"2" id=​"input2">​
focusin: focus was gained by <input type value=​"2" id=​"input2">​

// Input3 is clicked
focusout: focus is leaving <input type value=​"2" id=​"input2">​ and it will go to <input type value=​"3" id=​"input3">​
focusin: focus was gained by <input type value=​"3" id=​"input3">​

Now, let's add a blur handler that changes focus:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>
  <div id="container">
    <input type="" value="1" id="input1">
    <input type="" value="2" id="input2">
    <input type="" value="3" id="input3">
  </div>

  <script>
    container.addEventListener('focusin', (ev) => {
      console.log('focusin: focus was gained by', ev.target);
    });
    container.addEventListener('focusout', (ev) => {
      console.log('focusout: focus is leaving', ev.target, 'and it will go to', ev.relatedTarget);
    });
    input2.addEventListener('blur', (ev) => {
      input2.focus();
    });
  </script>
</body>
</html>

The log sequence changes and gives the wrong impression that no dom element has focus:

// Input1 is clicked
focusin: focus was gained by <input type value=​"1" id=​"input1">​

// Input2 is clicked
focusout: focus is leaving <input type value=​"1" id=​"input1">​ and it will go to <input type value=​"2" id=​"input2">​
focusin: focus was gained by <input type value=​"2" id=​"input2">​

// Input3 is clicked, but the handler kicks in and instead of the following line being a focusout, it results in a focusin call first.
focusin: focus was gained by <input type value=​"2" id=​"input2">​
focusout: focus is leaving <input type value=​"2" id=​"input2">​ and it will go to null

In addition to that, during focusout processing, activeElement typically points to <body />. However, if an element is focused during a blur event, activeElement points to that focused element. Although, undocumented it can be verified with:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>
  <div id="container">
    <input type="" value="1" id="input1">
    <input type="" value="2" id="input2">
    <input type="" value="3" id="input3">
  </div>

  <script>
    container.addEventListener('focusin', (ev) => {
      console.log('focusin: was gained by', ev.target);
    });
    container.addEventListener('focusout', (ev) => {
      console.log('document.hasFocus()', document.hasFocus());     
      console.log('document.activeElement', document.activeElement);
      console.log('focusout: focus is leaving', ev.target, 'and it will go to', ev.relatedTarget);
    });
    input2.addEventListener('blur', (ev) => {
      input2.focus();
    });
  </script>
</body>
</html>

We leverage these behaviors to ignore focusout events when the document has focus but activeElement is not <body />.

flutter/flutter#153022

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Sep 5, 2024
@tugorez tugorez changed the title focusout works when focus is changed in the middle of a blur call. Fix unexpected ViewFocus events when Text Editing utilities change focus in the middle of a blur call. Sep 5, 2024
@tugorez tugorez marked this pull request as ready for review September 5, 2024 03:00
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to that, during focusout processing, activeElement typically points to <body />. However, if an element is focused during a blur event, activeElement points to that focused element. Although, undocumented it can be verified with:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>
  <div id="container">
    <input type="" value="1" id="input1">
    <input type="" value="2" id="input2">
    <input type="" value="3" id="input3">
  </div>

  <script>
    container.addEventListener('focusin', (ev) => {
      console.log('focusin: was gained by', ev.target);
    });
    container.addEventListener('focusout', (ev) => {
      console.log('document.hasFocus()', document.hasFocus());     
      console.log('document.activeElement', document.activeElement);
      console.log('focusout: focus is leaving', ev.target, 'and it will go to', ev.relatedTarget);
    });
    input2.addEventListener('blur', (ev) => {
      input2.focus();
    });
  </script>
</body>
</html>

I tried this on multiple browsers to make sure they have the same behavior. Chrome and Safari match the behavior you described. Firefox didn't. In fact, Firefox seems to be ignoring input2.focus() completely.

Anyhow, I think your change is okay. On Firefox, it has no effect because that condition is never true.

@tugorez
Copy link
Contributor Author

tugorez commented Sep 5, 2024

In addition to that, during focusout processing, activeElement typically points to <body />. However, if an element is focused during a blur event, activeElement points to that focused element. Although, undocumented it can be verified with:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>
  <div id="container">
    <input type="" value="1" id="input1">
    <input type="" value="2" id="input2">
    <input type="" value="3" id="input3">
  </div>

  <script>
    container.addEventListener('focusin', (ev) => {
      console.log('focusin: was gained by', ev.target);
    });
    container.addEventListener('focusout', (ev) => {
      console.log('document.hasFocus()', document.hasFocus());     
      console.log('document.activeElement', document.activeElement);
      console.log('focusout: focus is leaving', ev.target, 'and it will go to', ev.relatedTarget);
    });
    input2.addEventListener('blur', (ev) => {
      input2.focus();
    });
  </script>
</body>
</html>

I tried this on multiple browsers to make sure they have the same behavior. Chrome and Safari match the behavior you described. Firefox didn't. In fact, Firefox seems to be ignoring input2.focus() completely.

Anyhow, I think your change is okay. On Firefox, it has no effect because that condition is never true.

That is actually true. Super interesting. This is a different, special, behavior tho. If the same node tries to get refocused firefox "prevents it". So in a sense, firefox doesn't allow a node to keep focus against the user will. If you change the code to something like this, you'll see the originally described behavior.

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>
  <div id="container">
    <input type="" value="1" id="input1">
    <input type="" value="2" id="input2">
    <input type="" value="3" id="input3">
  </div>

  <script>
    container.addEventListener('focusin', (ev) => {
      console.log('focusin: was gained by', ev.target);
    });
    container.addEventListener('focusout', (ev) => {
      console.log('document.hasFocus()', document.hasFocus());     
      console.log('document.activeElement', document.activeElement);
      console.log('focusout: focus is leaving', ev.target, 'and it will go to', ev.relatedTarget);
    });
    input2.addEventListener('blur', (ev) => {
      input1.focus();
    });
  </script>
</body>
</html>

…ng.dart

Co-authored-by: Mouad Debbar <mouad.debbar@gmail.com>
@tugorez tugorez added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2024
@auto-submit auto-submit bot merged commit 0462d19 into flutter:main Sep 5, 2024
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 5, 2024
…154691)

flutter/engine@c50eb8a...015f3b1

2024-09-05 jonahwilliams@google.com [engine] always force platform channel responses to schedule a task. (flutter/engine#54975)
2024-09-05 tugorez@users.noreply.github.com Fix unexpected ViewFocus events when Text Editing utilities change focus in the middle of a blur call. (flutter/engine#54965)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC codefu@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot added a commit to flutter/flutter that referenced this pull request Sep 6, 2024
…isions) (#154691)" (#154726)

Reverts: #154691
Initiated by: jtmcdole
Reason for reverting: breaking flutter-dashboard
Original PR Author: engine-flutter-autoroll

Reviewed By: {fluttergithubbot}

This change reverts the following previous change:

flutter/engine@c50eb8a...015f3b1

2024-09-05 jonahwilliams@google.com [engine] always force platform channel responses to schedule a task. (flutter/engine#54975)
2024-09-05 tugorez@users.noreply.github.com Fix unexpected ViewFocus events when Text Editing utilities change focus in the middle of a blur call. (flutter/engine#54965)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC codefu@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024
@tugorez tugorez deleted the text-fields branch September 6, 2024 21:13
zanderso added a commit to flutter/flutter that referenced this pull request Sep 6, 2024
flutter/engine@c50eb8a...419fb8c

2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[engine] always force platform channel responses to schedule a task.
(#54975)"
(flutter/engine#55000)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Skia from b6bab0fde426 to 6ad117bd2efe (2 revisions)
(flutter/engine#54999)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Fuchsia Test Scripts from D9INMR2u4wcyiZ750... to
5dqcFlKzRjJb6V95W... (flutter/engine#54998)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Skia from a09312b70d37 to b6bab0fde426 (3 revisions)
(flutter/engine#54997)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Skia from 368f209ccca5 to a09312b70d37 (1 revision)
(flutter/engine#54995)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Skia from aec11ae18bb6 to 368f209ccca5 (3 revisions)
(flutter/engine#54992)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Fuchsia Linux SDK from xNv47d1TZmK9XgTxu... to PBeI0gGvgFdXV6hCg...
(flutter/engine#54990)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Skia from 809f868ded1c to aec11ae18bb6 (22 revisions)
(flutter/engine#54988)
2024-09-06
[30870216+gaaclarke@users.noreply.github.com](mailto:30870216+gaaclarke@users.noreply.github.com)
Removes the int storage from Color
(flutter/engine#54714)
2024-09-06 [chris@bracken.jp](mailto:chris@bracken.jp) iOS,macOS: Add
logging of duplicate codesign binaries
(flutter/engine#54987)
2024-09-06
[skia-flutter-autoroll@skia.org](mailto:skia-flutter-autoroll@skia.org)
Roll Fuchsia Test Scripts from k4lKsecg0pdIp-U7c... to
D9INMR2u4wcyiZ750... (flutter/engine#54984)
2024-09-05
[a-siva@users.noreply.github.com](mailto:a-siva@users.noreply.github.com)
Manual roll of Dart. (flutter/engine#54983)
2024-09-05 [chris@bracken.jp](mailto:chris@bracken.jp) iOS,macOS: add
unsigned_binaries.txt (flutter/engine#54977)
2024-09-05
[jason-simmons@users.noreply.github.com](mailto:jason-simmons@users.noreply.github.com)
Manual Skia roll to 809f868ded1c
(flutter/engine#54972)
2024-09-05
[1961493+harryterkelsen@users.noreply.github.com](mailto:1961493+harryterkelsen@users.noreply.github.com)
[canvaskit] Fix incorrect calculation of ImageFilter paint bounds
(flutter/engine#54980)
2024-09-05 [jonahwilliams@google.com](mailto:jonahwilliams@google.com)
[engine] always force platform channel responses to schedule a task.
(flutter/engine#54975)
2024-09-05
[tugorez@users.noreply.github.com](mailto:tugorez@users.noreply.github.com)
Fix unexpected ViewFocus events when Text Editing utilities change focus
in the middle of a blur call.
(flutter/engine#54965)

Also rolling transitive DEPS:
fuchsia/sdk/core/linux-amd64 from xNv47d1TZmK9 to PBeI0gGvgFdX

---------

Co-authored-by: Christopher Fujino <christopherfujino@gmail.com>
Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants