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

Ensure focusin/focusout fires with .focus()/.blur() #2996

Merged
merged 6 commits into from Jul 5, 2020

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jun 24, 2020

This PR alters how HTMLOrSVGElement-impl so that the focus and blur methods also dispatch the relevant focusin and focusout events in the bubble phase, as per how they occur on modern browsers. I wasn't sure where to add any tests for this change specifically, if anyone has any pointers, that would be great.

Copy link
Contributor

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I wasn't sure where to add any tests for this change specifically, if anyone has any pointers, that would be great.

diff --git a/test/web-platform-tests/to-run.yaml b/test/web-platform-tests/to-run.yaml
index c2cff296..3f08f075 100644
--- a/test/web-platform-tests/to-run.yaml
+++ b/test/web-platform-tests/to-run.yaml
@@ -1006,7 +1006,7 @@ idlharness.window.html: [fail, Depends on fetch]
 legacy-domevents-tests/approved/ProcessingInstruction.DOMCharacterDataModified.html: [timeout, Mutation Events not implemented]
 legacy-domevents-tests/approved/domnodeinserted.html: [timeout, Mutation Events not implemented]
 mouse/**: [timeout, Uses testdriver.js]
-order-of-events/**: [timeout, Unknown]
+order-of-events/mouse-events/*: [timeout, Unknown]
 
 ---
 

which cause yarn test-wpt to test test/web-platform-tests/tests/uievents/order-of-events/focus-events/focus-automated-blink-webkit.html. Though this is failing on this branch with Maximum callstack size exceeded.

I guess you could add a smaller test to test/web-platform-tests/to-upstream/uievents to check (with yarn test-tuwpt if the test would require more work or if the current implementation is problematic.

Also: This PR closes #2708

@trueadm
Copy link
Contributor Author

trueadm commented Jun 24, 2020

Thanks for the help @eps1lon. I found focusing-focused-noop.html and updated that to include the logic from this test as it seemed the most reasonable approach.

@domenic
Copy link
Member

domenic commented Jun 24, 2020

Can we make sure that this doesn't cause infinite recursion, by enabling the test as @eps1lon suggests and then making it pass, instead of making it fail with Maximum callstack size exceeded?

@trueadm
Copy link
Contributor Author

trueadm commented Jun 24, 2020

@domenic Yep, I plan on looking into that later once I get some time to. :)

I was looking into why my changes caused the build to fail with:

/home/travis/build/jsdom/jsdom/test/web-platform-tests/tuwpt-manifest.jso

@trueadm
Copy link
Contributor Author

trueadm commented Jun 24, 2020

I've fixed the failing test. I ended up spending quite a bit longer on it, but using a build of Chrome and the test to cover the same sequence of how activeElement and relatedTarget get set, especially around iframes.

@trueadm trueadm force-pushed the add-focus-in-focus-out branch 2 times, most recently from 00780a4 to cc7a7cc Compare June 25, 2020 09:26
@trueadm
Copy link
Contributor Author

trueadm commented Jun 30, 2020

@domenic Are you happy with this PR or is there something you'd like me to look into still?

@domenic
Copy link
Member

domenic commented Jul 3, 2020

Can you enable the tests @eps1lon suggests?

@trueadm
Copy link
Contributor Author

trueadm commented Jul 3, 2020

@domenic Ah sorry, I thought that mention @eps1lon was in reference to enabling specific tests to run, not exclude. I've commited that change to this PR now too.

@domenic domenic self-assigned this Jul 4, 2020
@trueadm
Copy link
Contributor Author

trueadm commented Jul 5, 2020

The latest failures seem to be due to #2951 being merged on master.

Looking into why focus-management-expectations.html is failing, it seems to be with some other focus related logic, rather than something introduced in this PR.

@domenic domenic merged commit a52b77b into jsdom:master Jul 5, 2020
@trueadm trueadm deleted the add-focus-in-focus-out branch July 6, 2020 09:40
@trueadm
Copy link
Contributor Author

trueadm commented Jul 6, 2020

@domenic Would it be possible to get a new release of JSDOM? We want to incorporate this change into the next version of React and thus ensure our Jest test suite is able to pass with the changes to React's event system. We can upgrade Jest once JSDOM releases a new version. :)

@domenic
Copy link
Member

domenic commented Jul 6, 2020

For sure; I'll definitely do a release by this upcoming weekend, but hopefully can get it out sooner.

@trueadm
Copy link
Contributor Author

trueadm commented Jul 6, 2020

Thank you so much! I really appreciate it :)

@domenic
Copy link
Member

domenic commented Jul 10, 2020

FYI I published v16.3.0 today.

@rickhanlonii
Copy link

Thanks @domenic!

eps1lon pushed a commit to facebook/react that referenced this pull request Feb 13, 2023
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

This TODO mentions an issue with JSDOM that [seems to have been
resolved](jsdom/jsdom#2996).

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

## How did you test this change?

- Ensured that the `document.activeElement` is no longer `node` after
`node.blur` is called.
- Verified that the tests still pass.
- Looked for [a merged PR that fixes the
issue](jsdom/jsdom#2996).

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->
github-actions bot pushed a commit to facebook/react that referenced this pull request Feb 13, 2023
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

This TODO mentions an issue with JSDOM that [seems to have been
resolved](jsdom/jsdom#2996).

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

## How did you test this change?

- Ensured that the `document.activeElement` is no longer `node` after
`node.blur` is called.
- Verified that the tests still pass.
- Looked for [a merged PR that fixes the
issue](jsdom/jsdom#2996).

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

DiffTrain build for [fccf3a9](fccf3a9)
[View git log for this commit](https://github.com/facebook/react/commits/fccf3a9fba5fd778c678657c556344b333111cfb)
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

This TODO mentions an issue with JSDOM that [seems to have been
resolved](jsdom/jsdom#2996).

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

## How did you test this change?

- Ensured that the `document.activeElement` is no longer `node` after
`node.blur` is called.
- Verified that the tests still pass.
- Looked for [a merged PR that fixes the
issue](jsdom/jsdom#2996).

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

DiffTrain build for [fccf3a9fba5fd778c678657c556344b333111cfb](facebook/react@fccf3a9)
[View git log for this commit](https://github.com/facebook/react/commits/fccf3a9fba5fd778c678657c556344b333111cfb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants