I think I see it now, you are not actually touching the line which was broken, which is why I missed it. You are changing a lot more than necessary to fix the bug.

The bug probably comes from NextFromChildCollection calling Marshal.GetNativeVariantForObject and being passed an uint instead of an int - by changing all the types to int (even those that shouldn't be according to native spec) you silently fixed the bug without touching the line causing the issue.

The "minmal" (and IMHO correct) fix would be just adding a typecast to Marshal.GetNativeVariantForObject argument instead of changing unrelated signatures from uint to int. The IEnumVariant method signatures don't need to be changed. Perhaps the currentChild should remain changed to int, can't evaluate that since I don't know too much context of its meaning.

TLDR: while the PR fixes the bug I'd argue the signature changes to IEnumVariant are wrong and should be reverted. They are not wrong enough to break anything, but they deviate from the usual guideline to keep signedness when transcribing native APIs.

I had a brief chat with @JeremyKuhne, and whilst our definitions match those in oaidl.h (uint == ULONG), there's probably a contraction/expansion of the underlying value somewhere in the flow that leads to the loss/incorrect signedness...

The "minmal" (and IMHO correct) fix would be just adding a typecast to Marshal.GetNativeVariantForObject argument

👍

Codecov Report

Merging #4168 (ad70d15) into master (8dccbc0) will decrease coverage by 0.00774%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##              master       #4168         +/-   ##
===================================================
- Coverage   98.12512%   98.11738%   -0.00774%     
===================================================
  Files            497         497                 
  Lines         258523      258523                 
  Branches        4492        4492                 
===================================================
- Hits          253676      253656         -20     
- Misses          4089        4105         +16     
- Partials         758         762          +4     

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

@weltkante, you are cool as always! You are right.
The typecasting inside Marshal.GetNativeVariantForObject method is a sufficient change for the accessiblity tree to work correctly.

I really agree that changing IEnumVariant methods signature looks confusing because IEnumVARIANT.Next method has ULONG arguments (A ULONG is a 32-bit unsigned integer (range: 0 through 4294967295 decimal)) and it means (uint == ULONG). So it seems using uint for IEnumVARIANT is more correct.

Maybe @hughbe could say should we change currentChild type from uint to int in EnumVariantObject? Or can we keep it as is now?

there's probably a contraction/expansion of the underlying value somewhere in the flow that leads to the loss/incorrect signedness

I didn't see anything of that sort, but Marshal.GetNativeVariantForObject will chose the type of the variant based on the type of the argument you pass in, so if the native code wants an "enumerator of signed integers" you need to pass a signed integer to get a I4 variant.

The signedness of the IEnumVARIANT method arguments has nothing to do with the signedness of the values you are enumerating. Thats why it may be more correct to use int for currentChild (if it represents the enumerated values), but if you are confident that it never becomes negative its just as fine to keep it unsigned and only cast when handing it out through the native API. Especially if that saves you from a lot of typecasts in the implementation.

Testers approved the fix ✔️