Skip to content

Edge: Ignore multiple gotFocus events if we were the one who caused them#3399

Open
sratz wants to merge 1 commit into
eclipse-platform:masterfrom
sratz:edge-focus-events
Open

Edge: Ignore multiple gotFocus events if we were the one who caused them#3399
sratz wants to merge 1 commit into
eclipse-platform:masterfrom
sratz:edge-focus-events

Conversation

@sratz

@sratz sratz commented Jun 22, 2026

Copy link
Copy Markdown
Member

Follow-up on #1849 / #1848.

@sratz sratz requested a review from HeikoKlare June 22, 2026 11:39
@sratz sratz force-pushed the edge-focus-events branch from 920b65a to 3344006 Compare June 22, 2026 11:45
@github-actions

Copy link
Copy Markdown
Contributor

Test Results

  200 files  ±0    200 suites  ±0   27m 21s ⏱️ + 1m 40s
4 742 tests +2  4 718 ✅ +1   24 💤 +1  0 ❌ ±0 
6 880 runs  +6  6 712 ✅ +1  168 💤 +5  0 ❌ ±0 

Results for commit 3344006. ± Comparison against base commit f20619c.

@HeikoKlare HeikoKlare left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change looks sound to me. We had already shortly discussed it in #1849 and just didn't do it without knowing about a concrete case that needs it to keep complexity and risk as low as possible.
I also checked again that ignoreGotFocus is only accessed from the UI thread, such that no memory consistency mechanisms need to be applied.

Just a nit question: shouldn't the test succeed on every browser (not only Edge), so that removing the assumption may be reasonable?

@sratz

sratz commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

Just a nit question: shouldn't the test succeed on every browser (not only Edge), so that removing the assumption may be reasonable?

In principle, yes. But timing is slightly different. Also, for some reason on Linux/WebKit I am receiving 2 Events for each Browser.setFocus().

Not sure what exactly is going on there, haven't looked into this further. It's probably fine and the general assumption that "1 SWT.FocusIn event per Browser.setFocus()" just doesn't hold generally.

So only for the Edge case we know that should be the case and can attribute a mismatch to a regression for this particular issue.

I propose to proceed with having the test flagged Edge-only, WDYT?

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.

2 participants