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

Automatically set system focus to focusable elements: properly set focus when automatic focus mode is triggered #11668

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Sep 26, 2020

Many thanks to @lukaszgo1 for assisting in tracking this down!

This is opened against beta, since it fixes a regression introduced during 2020.3 development cycle

Link to issue number:

Fixes #11663
Replaces #11665

Summary of the issue:

With focus focusable elements disabled and Automatic focus mode for caret movements enabled when switching to focus mode as a result of caret movement focus was not moved to the control with caret. This made it impossible to, for example type text to the edit fields or properly interact with combo boxes.

Description of how this pull request fixes the issue:

Automatic focus mode is triggered when calling _set_selection on the browse mode document tree interceptor. The logic in this method has been changed to set focus to the object, either when Automatically set system focus to focusable elements is on or when we are going to pass through.

Testing performed:

Tested the steps to reproduce from #11663.

Known issues with pull request:

None

Change log entry:

Note, though this fixes a regression in 2020.3 beta, the auto set focus to focusable elements option was there before 2020.3.

michaelDCurran
michaelDCurran previously approved these changes Sep 29, 2020
@michaelDCurran
Copy link
Member

I'm not sure the changelog entry is quite right though...
"When automatically set focus to focusable elements is enabled..." shouldn't it be disabled? or do you mean the automatic focus mode for caret movement?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Sep 29, 2020 via email

@LeonarddeR
Copy link
Collaborator Author

I fixed the changelog entry.

@feerrenrut
Copy link
Contributor

This probably doesn't need a changelog entry if merged into beta, right?

feerrenrut
feerrenrut previously approved these changes Sep 29, 2020
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @LeonarddeR

@LeonarddeR
Copy link
Collaborator Author

Yes, it needs a changelog entry, as the feature, though experimental, has been there for several releases now.

@feerrenrut
Copy link
Contributor

But entry is for a bug fix that was never in a release. We typically don't include a note for these. The feature presumably had / has its own change log entry.

@feerrenrut
Copy link
Contributor

Though perhaps this could be worded such to mention that these two features are now compatible.

@feerrenrut
Copy link
Contributor

EG:

@feerrenrut feerrenrut dismissed stale reviews from michaelDCurran and themself via c561d35 September 29, 2020 12:06
@feerrenrut feerrenrut merged commit 7490226 into nvaccess:beta Sep 29, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.4 milestone Sep 29, 2020
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.

4 participants