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: Return to handling focus events directly, after generic node changes #409

Merged
merged 1 commit into from
May 13, 2024

Conversation

mwcampbell
Copy link
Contributor

I now believe that #354 was a mistake. So I've re-added the code in focus_moved to emit focus change events. But now, we specifically skip the focused state when looking for generic state changes. Reasons to do it this way:

  1. If a node has just been added, whether actually added to the tree or just newly visible in the filtered tree, we don't generally want to emit state change events for that node, but if it's the focused node, we do have to emit a focus event.
  2. If a node has other state changes at the same time that it receives focus, we want to emit the other state change events before the focus event. If we do it the other way around, as might happen depending on the order of the states, then Orca might read some states twice, once when presenting the newly focused node, and again in response to the state change event that was emitted after the focus change. I heard this happen; when leaving a menu and returning focus to the menu button, Orca would say something like, "Primary Menu, toggle button, not pressed. Not pressed."
  3. Also on the topic of event order, we should emit the activation event for the window before emitting the focus event for the node within the window. This change makes sure that it happens in that order.

@DataTriny
Copy link
Member

@mwcampbell CI is either stuck here or its status report is bugged. Can you please rebase to trigger it again?

@mwcampbell mwcampbell force-pushed the atspi-common-revert-focus-handling branch from 5a2840b to ab6a854 Compare May 13, 2024 20:45
Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

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

Subtle bugs I might not have encountered during my limited testing. We badly need integration tests...

@DataTriny
Copy link
Member

@mwcampbell The PR title doesn't make sense to me, and it's probably a bit vague to be included in the changelog. Could you please rename before merging?

@mwcampbell mwcampbell changed the title fix: Go back to handling focus change specially fix: Return to handling focus events directly, after generic node changes May 13, 2024
@mwcampbell
Copy link
Contributor Author

OK, is the new name better?

@DataTriny
Copy link
Member

Yes. It still fails to mention that it only applies to the Unix adapter but it's quite long already. Feel free to merge as is if you can't rephrase it. It's acceptable now thanks.

@mwcampbell mwcampbell merged commit cd2e35e into main May 13, 2024
10 checks passed
@mwcampbell mwcampbell deleted the atspi-common-revert-focus-handling branch May 13, 2024 20:57
@mwcampbell mwcampbell mentioned this pull request May 13, 2024
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