-
Notifications
You must be signed in to change notification settings - Fork 90
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
test: Add hack to avoid losing focus on #containers-filter #1356
Conversation
First thing that came to mind was #439 (we have workaround for this in tests) The workaround is indeed very hacky and very much focus on one specific place but I would think that if focus is lost it will happen on more places and I would either prefer more generic hack or at least knowing what steals the focus since we don;t have this problem in other plugins. You mentioned in #1324 that there are status updates - have you checked if these actually are linked to losing of focus? |
Yeah, fair enough. I already tried a generic fix in cockpit-project/cockpit#19078 , and it was a spectacular failure -- both broken and not even fixing this bug. At this point I've sank enough hours into this that I'll just give up (I need to work on some other things), and leave this for the next round of flake hunting. |
For some reason, .focus() sometimes does not take effect, and the element immediately loses the focus again. This causes set_input_text() to select the whole page text instead of the input element, and failing to set the intended value.
We have to do something about this. This still breaks upstream cockpit-podman runs in podman way too often and creates noise, false negatives, and frustration. |
e543081
to
852fe88
Compare
Okay, so this still reproduces very well on our infrastructure, it's especially pronounced on /devel. Curiously, it passed with firefox, but TF runs firefox. It sometimes reproduces locally with Continuing from #1324 (comment) , it is still true that there are no podman events during this operation, although of course the log ordering is not very reliable. This also shows when ContainerHeader() gets-re-rendered; that does not happen in between the focus and key input, but it does get re-rendered again afterwards with the old value:
With the confirm focus patch the test fails on that then:
So indeed the |
4f42f80
to
0ce1e25
Compare
Pushing the original hack again. There is absoeffingly nothing to grab on here, and locally this has survived
Let's see whether CI agrees. |
OK, it's stable now with the 10x amplified test in commit 0ce1e25 . So really, this is it from me -- I am not going to look at this minefield again. If someone else has an idea, please have at it 😁 |
0ce1e25
to
91cbc7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit reluctant to approve this, however the podman reverse dependency tests failing are also getting a bit on my nerves.
Wondering if we should make an issue to debug this further? Can it be a re-draw? Update in between which loses focus of the input?
@jelly See above, I logged re-renders, and didn't find anything. I have no ideas any more how to debug this.. |
For some reason, .focus() sometimes does not take effect, and the element immediately loses the focus again. This causes set_input_text() to select the whole page text instead of the input element, and failing to set the intended value.
This fixes this common flake, another example. So far this breaks pretty much every PR which runs tests three times (affected).
I realize this is a rather lame workaround, not a true fix. I investigated that in #1324 (comment) and attempted a better fix, but it didn't work, and I gave up after some hours ("pick your battles"). If someone is interested in tracking this down, please do!
I stress-tested this in #1324, and it passed 15 times in all operating systems at the first try (I amplified the test)