Skip to content

Conversation

@sratz
Copy link
Member

@sratz sratz commented Feb 20, 2025

Fixes #1848.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2025

Test Results

   502 files  ±0     502 suites  ±0   7m 32s ⏱️ -29s
 4 334 tests ±0   4 320 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 575 runs  ±0  16 466 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit cb9b69d. ± Comparison against base commit 32032fb.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

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?

@sratz
Copy link
Member Author

sratz commented Feb 21, 2025

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.

@sratz
Copy link
Member Author

sratz commented Feb 21, 2025

Do you think we should get this into RC2?

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

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.

@sratz
Copy link
Member Author

sratz commented Feb 21, 2025

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.

@sratz sratz added the edge Edge Browser label Feb 21, 2025
@sratz sratz added this to the 4.36 M1 milestone Mar 5, 2025
@sratz sratz merged commit 7d86f5c into eclipse-platform:master Mar 5, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

edge Edge Browser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Edge: Focus jumps back to browser control when trying to leave

2 participants