Edge: Ignore gotFocus if we were the one who set focus to WebView2 #1849
Conversation
There was a problem hiding this comment.
In general, the change looks fine to me.
I was just wondering what happens in case multiple focus-in events happen. Let's say the user for some reason rapidly switched focus between the browser and some other control, such that browserFocusIn is called multiple times. Would it be possible that the async calls of handleGotFocus are all processed afterwards, such that the second and further calls will erroneously be processed again? In order words: might it be necessary to count the number of browserFocusIn calls and decrement them in handleGotFocus or would that be wrong?
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Show resolved
Hide resolved
Thought about this, too. Counting up/down an integer could be an option, but I was not sure if that would on the other hand cause other weird issues where we would somehow not react to an event that we should have. Ignoring one single event solved the issues in the scenarios that we tested so I think this is a good starting point. |
|
Do you think we should get this into RC2? |
There was a problem hiding this comment.
I was just wondering what happens in case multiple focus-in events happen. Let's say the user for some reason rapidly switched focus between the browser and some other control, such that browserFocusIn is called multiple times. Would it be possible that the async calls of handleGotFocus are all processed afterwards, such that the second and further calls will erroneously be processed again? In order words: might it be necessary to count the number of browserFocusIn calls and decrement them in handleGotFocus or would that be wrong?
Thought about this, too. Counting up/down an integer could be an option, but I was not sure if that would on the other hand cause other weird issues where we would somehow not react to an event that we should have.
Ignoring one single event solved the issues in the scenarios that we tested so I think this is a good starting point.
Agreed. Also thought about potential "side" effects of counting the number of events, so following your proposal of starting with this simple solution sounds fine (in particular since some broken focus handling, as the worse case of a remaining issue, is of course annoying but usually not a severe, blocking issue).
Do you think we should get this into RC2?
I am not sure about this. I would be slightly in favor of holding it back unless you found the issue significantly annoying or problematic in your use cases. The CTRL+E scenario does not seem to be that crucial to me and since we will effectively have no implicit testing of it between merging the PR and the release, we should be really sure that it cannot make anything worse than better. I am rather confident about that, but still we know that you never know.
Agreed, this came up rather later late with thorough systematic test tours. I guess it's not critical as it feels more like a fluke to users than an actual systematic issue. So I am fine with deferring to 2025-06. |
Fixes #1848.