Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implementing HeadingLevel and IsDialog automation properties #4751
Implementing HeadingLevel and IsDialog automation properties #4751
Changes from 42 commits
b98bb57
9a5eb0e
0174765
1a5afaf
adc3050
0cf1bdc
03125c0
edd1008
0bcb251
0e3ff1d
3a8be23
ac060ac
e49e2b0
7393512
81a1df3
1ee85b4
cbbc720
3921923
ba44bdd
118a26a
f092113
d223aa8
df19535
df85bbd
d01beaa
a3ee3c1
3944e81
a2ad746
fed5d29
23cc6b6
0afb956
764d246
ee46685
c203d9e
9ebec44
cdf3786
a4df4cd
0b283ac
1ae4a20
d29ff29
7ef5140
f7f4ff4
1c09b7d
c113364
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought .NET Core wasn't using switches, but obviously the code above here is doing so and you're just following suit. But you should use a new switch named UseNet50CompatibleAccessibilityFeatures, as you're protecting features that are new in 6.0. Adding new switches to AccessibilitySwitches is a little tricky, as it's generated from a template; let me know if you can't figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think .NET (Core) is using switches - or at least it wasn't supposed to.
WPF has the switch infrastructure because I introduced it for the sake of back-compat in .NET Core 3.0 - in the sense that we'd wanted to give customers who were migrating from .NET 4.x to .NET Core 3.0 the option of either using the 'on' way of doing things, or the 'off' way of doing things for the various WPF switches (i.e., let them do whatever they used to do).
My 2c: At this point, you'll probably only stop using switches if you break the inertia and stop using switches. So consider what your 'compat switch' philosophy going forward is going to be. These types of changes are great opportunities to break away from the default-choice (adding a switch) and doing something else that's more aligned with the development practices across the board (and may very well improve overall maintainability, testing etc., and also simplify documentation of breaking-changes).
In fact, you might even consider removing most (but not all - some will likely persist forever) of these switches (there is a sample that lists all of them) in .NET 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vatsan-madhavan - I'm puzzled how you can say .NET wasn't supposed to use switches, and in the next sentence admit that you added them.
Perhaps what Vatsan means is that .NET (Core) shouldn't add any new switches, and that he retained the switches that were already in 4.8 for compat. That sounds right to me. (Although we can't get full 4.8-compat because CLR now only supports setting switches programmatically, not by app.config or registry.)
That said, let's remove the switches in the new code. There's not really any point for switches around the new properties anyway - if people don't want the new properties to work (a doubtful situation already), they can use .NET 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant it only in the sense that .NET Core wasn’t supposed to use the switches paradigm in an ongoing manner for compatibility-protection because runtime-updates are no longer in-place. You’re correctly surmising that I’d intended to add this infrastructure for .NET Framework 4.8 compat (only).
That said, individual frameworks like WindowsDesktop and teams like WPF obviously have a lot of discretion here.
Though app.config doesn’t work in core, runtimeconfig.json ought to work (our sample shows how).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guimafelipe - remember to remove the Accessibility switches from these two clauses. (Keep the null check, of course)