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(cdk/testing): simulate focusin/focusout events #23795

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 20, 2021

This is a resubmit of #23768.

Fixes that our fake fallback focus events weren't dispatching focusin and focusout events as well.

Fixes #23757.

Caretaking Note: spent a couple of hours looking into this, but it turned out to be trickier than expected. The extra events trigger a "changed after checked" error in some apps. Tried to simulate browser events even closer by only emitting the events when elements have focus, but that broke even more apps.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release labels Oct 20, 2021
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 20, 2021
@crisbeto
Copy link
Member Author

Caretaker note: this change previously caused a few failures in g3.

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Oct 20, 2021
andrewseguin
andrewseguin previously approved these changes Oct 20, 2021
devversion
devversion previously approved these changes Oct 20, 2021
@crisbeto crisbeto force-pushed the fake-focus-events-sequence-resubmit branch from 5cddadf to 09a46af Compare October 26, 2021 15:23
@crisbeto crisbeto requested a review from a team as a code owner October 26, 2021 15:23
@crisbeto crisbeto force-pushed the fake-focus-events-sequence-resubmit branch from 09a46af to d5f5022 Compare October 26, 2021 15:24
@devversion devversion removed the request for review from a team November 5, 2021 12:56
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@zarend
Copy link
Contributor

zarend commented Feb 4, 2022

Ran presubmit recently and got 39 failures. I briefly looked at the test failures and many of them are because the autocomplete element cannot be found. Needs more investigation, but I'm noticing tests that are manually invoking the focusin and focusout event. Perhaps the tests do not need to do that anymore with this change.

@crisbeto crisbeto force-pushed the fake-focus-events-sequence-resubmit branch from d5f5022 to 2f649ef Compare March 7, 2022 08:16
@crisbeto crisbeto force-pushed the fake-focus-events-sequence-resubmit branch 2 times, most recently from ac78558 to c0cbdad Compare March 8, 2022 09:18
Fixes that our fake fallback focus events weren't dispatching `focusin` and `focusout` events as well.

Fixes angular#23757.
@crisbeto crisbeto force-pushed the fake-focus-events-sequence-resubmit branch from c0cbdad to 5fefbd8 Compare March 8, 2022 09:24
@andrewseguin andrewseguin removed the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Mar 28, 2022
@mmalerba
Copy link
Contributor

mmalerba commented Apr 1, 2022

@crisbeto it looks like a bunch of tests call .blur() in order to mark form controls as touched and cause error messages to appear, etc. I see the original PR did not have the logic to prevent calling blur if the element was already blurred, so I assume it was added in this re-submit to fix some issue that came up last time.

I wonder could we still fire the focus/blur and just skip the focusin/focusout depending on the current state? I started going through all of the tests to change them to focus and then blur, but the more tests I change the more I feel that this isn't really a desirable change, I think its just going to confuse people about why their blur logic isn't working

@crisbeto
Copy link
Member Author

crisbeto commented Apr 1, 2022

When I first submitted the PR, it didn't have all the extra checks around focus and blur, but the new focusin and focusout events ended up causing some "Changed after checked" errors internally. The current state of the PR is me trying to reduce the amount of breakages, but it didn't help much. Ideally we wouldn't have those checks. I can revert it to the initial state if you want to see what failures it was causing.

@mmalerba
Copy link
Contributor

mmalerba commented Apr 1, 2022

I figured it was something like that. That's why I'm asking can we always fire the focus/blur events and just guard the focusin/focusout events with the extra hasFocus logic?

@crisbeto
Copy link
Member Author

crisbeto commented Apr 1, 2022

Looking at my CL from when I tried to land this (cl/433124526), that's what I tried doing as well but there were still a handful of failures. I can change it here if you want to give it a shot.

@andrewseguin andrewseguin dismissed stale reviews from devversion and themself via 5fefbd8 May 2, 2022 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(autocomplete): harness does not fire focusin event
6 participants