Replace UIA CompareInBounds with til::point comparators

[carlos-zamora]

This PR replaces the uses of Viewport::CompareInBounds() in the UIA code with til::point comparators. Additionally, it simplifies the logic further by using std::max and std::min.

In doing so, we are no longer hitting the assert in CompareInBounds().

Closes #14542

[DHowett]

Since this looks like a new version of #13244, which was reverted in #13907, I may need an explanation as to why the reason stated for the revert doesn't apply 😄

[DHowett]

as noted

[carlos-zamora]

@DHowett

Ok. This is "great". Here's what happened:

• PR Remove most uses of CompareInBounds #13244:
  • this code:

        if (!bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true))
        {
            THROW_HR(E_FAIL);
        }

• is replaced with this code:

        THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));

• Due to this specific change, Accessibility: NVDA emits errors in WT1.15 preview #13866 occurs. Turns out, the rest of the change is mostly fine. But since we were prepping for a release, it was much easier to revert the change (that's definitely on me).
• This PR:
  • This time around, I'm not making that mistake of removing the true. I've manually verified that Accessibility: NVDA emits errors in WT1.15 preview #13866 doesn't occur.
  • Also, Remove most uses of CompareInBounds #13244 included a change to remove the second param from IsInBounds(), which is what specifically lead to the change above.

[carlos-zamora]

We could still do the other CompareInBounds() replacements throughout the codebase (as in #13244). Figured specifically focusing on UIA would be nicer though.

[lhecker]

FYI: The final boolean parameter in CompareInBounds is ignored in Release mode builds anyways, so from a perspective of "could this potentially crash?" it doesn't matter if the old code used to have true, false, or no boolean argument at all.

[lhecker]

To make sure that the _start/_end range is properly degenerate here we'd have to use something like this right?

    _end = std::min(_end, documentEnd);
    _start = std::min(_start, _end);

Otherwise _start > _end is possible. I mean technically this function doesn't even touch these two members at this point, but this made me wonder if we should change IsDegenerate() from _start == _end to _start >= _end just in case?

[carlos-zamora]

I'm conflicted. On one hand, IsDegenerate() would be fixed, but on the other hand, we clearly messed up if _start > _end at any point. These two lines help alleviate that by clamping down to documentEnd. Maybe adding an assert(_start <= _end) in IsDegenerate() would be good to detect if we ever run into that? idk, if that would be useful though. idk.

[lhecker]

I'm not sure what to do either, but I personally would make the change to IsDegenerate() regardless. Simply because bugs will exist in our code regardless of how hard we try, so we might as well build our code around that assumption. But I could also be completely wrong about it here, since I'm not completely familiar with how IsDegenerate() is used and whether such a change would make any tangible safety improvements at all in the first place.
assert()s are IMO a great idea in either case, and the more we use them the better, because it's like test driven programming, but at runtime and it's more productive (arguably). 😅

[DHowett]

Way cleaner! Thanks!

[ghost]

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.