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

Fixed the issue where edit textBox loses focus when using up/down to switch property values ​​on edit textBox #12431

Conversation

LeafShi1
Copy link
Member

@LeafShi1 LeafShi1 commented Nov 6, 2024

Fixes #12440

Proposed changes

  • When setting a property with the [RefreshProperties(RefreshProperties.All)] attribute, suppress recalculation and reset constants for the entire property page

Customer Impact

  • The rectangle in the edit text box always stays in the edit textbox when changing the property value using the up/down buttons

Regression?

  • No

Risk

  • Mininal

Screenshots

Before

The rectangle always back to focus on entire row after using the up/down changed the property values directly in the edit textbox
BeforeChanges

After

The rectangle in the edit text box always stays in the edit textbox when changing the property value using the up/down buttons
AfterChanges

Test methodology

  • Manually

Test environment(s)

  • .net 10.0.0-alpha.1.24554.9
Microsoft Reviewers: Open in CodeFlow

@LeafShi1 LeafShi1 requested a review from a team as a code owner November 6, 2024 07:18
@LeafShi1 LeafShi1 changed the title Fix editor text box focus issue when switching value property grid view Fixed the issue where EditorTextBox loses focus when using up/down to switch property values ​​on EditorTextBox Nov 6, 2024
@LeafShi1 LeafShi1 changed the title Fixed the issue where EditorTextBox loses focus when using up/down to switch property values ​​on EditorTextBox Fixed the issue where edit textBox loses focus when using up/down to switch property values ​​on EditorTextBox Nov 6, 2024
@LeafShi1 LeafShi1 changed the title Fixed the issue where edit textBox loses focus when using up/down to switch property values ​​on EditorTextBox Fixed the issue where edit textBox loses focus when using up/down to switch property values ​​on edit textBox Nov 6, 2024
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.73581%. Comparing base (65f897b) to head (7b9e0f2).
Report is 47 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #12431         +/-   ##
===================================================
+ Coverage   75.65407%   75.73581%   +0.08173%     
===================================================
  Files           3150        3153          +3     
  Lines         635831      635797         -34     
  Branches       47020       46973         -47     
===================================================
+ Hits          481032      481526        +494     
+ Misses        151352      150839        -513     
+ Partials        3447        3432         -15     
Flag Coverage Δ
Debug 75.73581% <0.00000%> (+0.08173%) ⬆️
integration 18.27145% <0.00000%> (+0.02238%) ⬆️
production 49.31168% <0.00000%> (+0.11336%) ⬆️
test 97.05103% <ø> (+0.02079%) ⬆️
unit 46.28890% <0.00000%> (+0.13293%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

---- 🚨 Try these New Features:

@LeafShi1
Copy link
Member Author

LeafShi1 commented Nov 6, 2024

The private dlls have been tested, no regression issue was found.

@Tanya-Solyanik
Copy link
Member

Tanya-Solyanik commented Nov 7, 2024

@LeafShi1 - please fix a link to the issue, right now it points to another PR

Actually, this is a non-regression problem found when testing PR #12356. No issue was filed before.
I created an issue for it and the issue link has been updated.

@@ -3997,7 +3997,7 @@ private void Refresh(bool fullRefresh, int startRow, int endRow)
startRow = 0;
}

if (fullRefresh || OwnerGrid.HavePropertyEntriesChanged())
if (OwnerGrid.HavePropertyEntriesChanged())
Copy link
Member

Choose a reason for hiding this comment

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

COuld you please help me understand this fix? I would expect that in this scenario OwnerGrid.HavePropertyEntriesChanged is true.

Copy link
Member Author

@LeafShi1 LeafShi1 Nov 7, 2024

Choose a reason for hiding this comment

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

When setting a property, the SetPropertyValue method is called. In this method, the Refresh method is called to refresh the property list based on the Attribute of the property.

But in fact, when setting the AutoSize property, the number and structure of properties in the property list will not change. We only need to refresh the linked property values, so there is no need to recreate the property list.

Copy link
Member Author

Choose a reason for hiding this comment

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

When we switch the value of the edit text by pressing the up and down keys, the SelectAll event will be called, and this event will eventually enter the SelectInternal method. The AI ​​notification is sent from the SelectInternal method here, but the notification sent here is UIA_Text_TextSelectionChangedEventId. If we add notification UIA_AutomationFocusChangedEventId to it, this problem also can be resolved.

image

Copy link
Member

@Tanya-Solyanik Tanya-Solyanik Nov 12, 2024

Choose a reason for hiding this comment

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

But in fact, when setting the AutoSize property, the number and structure of properties in the property list will not change. We only need to refresh the linked property values, so there is no need to recreate the property list.

Please verify this with a Label control and Multiline property ( if still applicable to the new approach)

Copy link
Member

Choose a reason for hiding this comment

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

Please test with NVDA and JAWS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested with NVDA and JAWS, same results for both tools
And using the changes from commit1 and commit 2, same results too.
VeriftWithNVDA

Copy link
Member

@ricardobossan ricardobossan left a comment

Choose a reason for hiding this comment

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

Tested the changes and the TextBox does not lose focus when you press up/down.

Changes LGTM.

@Tanya-Solyanik Tanya-Solyanik added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 12, 2024
Tanya-Solyanik
Tanya-Solyanik previously approved these changes Nov 13, 2024
@Olina-Zhang
Copy link
Member

Tested this PR change with several regression issues found. Need to investigate.

@Olina-Zhang Olina-Zhang removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 13, 2024
@LeafShi1 LeafShi1 force-pushed the Fix_EditorTextBox_focusIssue_when_switch_value_in_PropertyGridView branch from 9bf352b to 7b9e0f2 Compare November 14, 2024 06:15
@LeafShi1 LeafShi1 added the waiting-review This item is waiting on review by one or more members of team label Nov 14, 2024
@LeafShi1
Copy link
Member Author

LeafShi1 commented Nov 15, 2024

@Tanya-Solyanik The second commit caused 3 unstabled regression issues, RaiseAutomationEvent(UIA_EVENT_ID.UIA_AutomationFocusChangedEventId) does not stably focus the accessibility rectangle on the edit text box, so consider the solution When setting a property with the [RefreshProperties(RefreshProperties.All)] attribute, only the relevant property values ​​are refreshed instead of recalculating and refreshing the entire property list in commit1. And commit1 has been tested before, no regression issue.

Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

ok, let's remove the fullRefresh test.

@Tanya-Solyanik Tanya-Solyanik added ready-to-merge PRs that are ready to merge but worth notifying the internal team. and removed waiting-review This item is waiting on review by one or more members of team labels Nov 20, 2024
@LeafShi1 LeafShi1 merged commit dc4314e into dotnet:main Nov 20, 2024
8 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0 Preview1 milestone Nov 20, 2024
@dotnet-policy-service dotnet-policy-service bot removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Nov 20, 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.

When using up/down to toggle property value on edit text box, edit text box loses focus
4 participants