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

Accessibility: NRE announcing changed item in PropertyGrid #1998

Merged

Conversation

M-Lipin
Copy link
Contributor

@M-Lipin M-Lipin commented Sep 30, 2019

Fixes #1997

Proposed changes

  • Adding test for null before announcement changed item details as changed item can be disposed in case Property grid is recreated on new value submit.
  • Announcement is not broken and this case is rare. It is ok to not announce the value of disposed and recreated PropertyGrid.

Customer Impact

  • Customers will not experience application crash in case the PropertyGrid control can be recreated in Form lifecycle.

Regression?

  • Yes (before introducing announcements for PropertyGrid value updates there was no issue)

Risk

  • None (just one additional null value test)

Screenshots

Before

Crash stack:

  System.Windows.Forms.dll!System.Windows.Forms.PropertyGridInternal.PropertyGridView.GridViewEdit.ProcessDialogKey(System.Windows.Forms.Keys keyData) Line 6808 C# Symbols loaded.
  System.Windows.Forms.dll!System.Windows.Forms.PropertyGridInternal.PropertyGridView.GridViewEdit.OnKeyDown(System.Windows.Forms.KeyEventArgs ke) Line 6693 C# Symbols loaded.
  System.Windows.Forms.dll!System.Windows.Forms.Control.ProcessKeyEventArgs(ref System.Windows.Forms.Message m) Line 10649 C# Symbols loaded.
  System.Windows.Forms.dll!System.Windows.Forms.Control.ProcessKeyMessage(ref System.Windows.Forms.Message m) Line 10695 C# Symbols loaded.
  System.Windows.Forms.dll!System.Windows.Forms.Control.WmKeyChar(ref System.Windows.Forms.Message m) Line 13328 C# Symbols loaded.

After

No crash.

Test methodology

  • Manual testing.
  • Automation testing (to be run)
  • Unit tests (to be implemented)

Accessibility testing

Test environment(s)

.NET Core SDK (reflecting any global.json):
Version: 3.1.100-preview1-014044
Commit: dca04e7030
Runtime Environment:
OS Name: Windows
OS Version: 10.0.18362
OS Platform: Windows

Microsoft Reviewers: Open in CodeFlow

@M-Lipin M-Lipin requested a review from a team as a code owner September 30, 2019 23:22
@M-Lipin M-Lipin self-assigned this Sep 30, 2019
@M-Lipin M-Lipin 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 Sep 30, 2019
@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

Merging #1998 into release/3.1 will increase coverage by 0.00193%.
The diff coverage is 0%.

@@                  Coverage Diff                  @@
##           release/3.1       #1998         +/-   ##
=====================================================
+ Coverage      26.5056%   26.50754%   +0.00194%     
=====================================================
  Files              805         805                 
  Lines           268083      268086          +3     
  Branches         38068       38069          +1     
=====================================================
+ Hits             71057       71063          +6     
+ Misses          191947      191942          -5     
- Partials          5079        5081          +2
Flag Coverage Δ
#Debug 26.50754% <0%> (+0.00193%) ⬆️
#production 26.50754% <0%> (+0.00193%) ⬆️
#test ?

RussKie
RussKie previously approved these changes Oct 1, 2019
@RussKie RussKie changed the title WIP: PropertyGrid Accessibility: Adding test changed item for null before announcement PropertyGrid Accessibility: Adding test changed item for null before announcement Oct 1, 2019
zsd4yr
zsd4yr previously approved these changes Oct 8, 2019
@merriemcgaw
Copy link
Member

Approved in tactics

@zsd4yr
Copy link
Contributor

zsd4yr commented Oct 10, 2019

@msftbot please merge after 24 hours

@ghost ghost added the :octocat: automerge label Oct 10, 2019
@ghost
Copy link

ghost commented Oct 10, 2019

Hello @zsd4yr!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Fri, 11 Oct 2019 19:30:07 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@zsd4yr
Copy link
Contributor

zsd4yr commented Oct 10, 2019

@msftbot nevermind.

Tactics has technically not opened the 3.1 gate yet

@zsd4yr zsd4yr removed the :octocat: automerge label Oct 10, 2019
@RussKie
Copy link
Member

RussKie commented Oct 11, 2019

@zsd4yr we're still waiting for QA sign off, so we shouldn't be merging it just yet

@merriemcgaw
Copy link
Member

@Vino-Wang has CTI picked up this PR yet?

@Vino-Wang
Copy link

@merriemcgaw, we got it today, will test it and update test results in Teams thread.

@M-Lipin M-Lipin dismissed stale reviews from zsd4yr and RussKie via 3dd3964 October 22, 2019 10:23
@M-Lipin M-Lipin force-pushed the dev/v-milipi/Issue_1997_PropertyGrid_Submit_value branch from 6f4cd5c to 3dd3964 Compare October 22, 2019 10:23
@M-Lipin
Copy link
Contributor Author

M-Lipin commented Oct 23, 2019

Rebased on top of release/3.1 to get all new fixes in the feature branch.

@M-Lipin
Copy link
Contributor Author

M-Lipin commented Oct 23, 2019

Fix passed QA verification.

@M-Lipin M-Lipin 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 Oct 23, 2019
@RussKie RussKie changed the title PropertyGrid Accessibility: Adding test changed item for null before announcement Accessibility: NRE announcing changed item in PropertyGrid Oct 24, 2019
@RussKie RussKie added Ask-Mode Servicing-approved .NET Shiproom approved the PR for merge and removed 📫 waiting-approval labels Oct 24, 2019
@RussKie RussKie merged commit d890296 into release/3.1 Oct 24, 2019
@RussKie RussKie deleted the dev/v-milipi/Issue_1997_PropertyGrid_Submit_value branch October 24, 2019 06:26
@RussKie RussKie added this to the 3.1 milestone Oct 24, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved .NET Shiproom approved the PR for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants