The code (including the initial check that is adapted in this PR) was added in

commit b02ed2de682ec2b323282b8e4f67d60a825dd8e2
Author: Michael Curran <michaelDCurran@users.noreply.github.com>
Date:   Wed Oct 31 16:56:51 2018 +1000

    Fix up of: Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected (#8898)
    
    * Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected.
    
    * Address review comments.
    
    * Address review comments.
    
    * Update what's new
    
    * Limit logic that stops announcing selected for a single selected item, and announces 'not selected' for not selected items, to controls that have the focusable state.
    
    * IAccessible NVDAObject's getSelectedItemsCount_accSelection: handle a childID coming from accSelection, as found in QT tree tables like in Mumble

but I (as a non-expert) see no obvious reason to allow for positive child IDs here only.

Since I'm not familiar with how things work exactly, I've updated the branch/PR to incorporate the suggested change in the original commit, and updated commit message and the PR description.
From other PRs, it seems though, that the commits are finally squashed into a single commit anyway.

Sorry if that was not the right way...

For the future: What's the "right" way to address comments in the review process and what should I update so that the commit message of the final commit is an up to date one? Is that taken from the actual commit(s) or from the pull request description, so it's mostly important to keep that up to date and the single commits of the PR don't necessarily have to be "nice" because they're squashed into a single one anyway?

@feerrenrut May be you can answer this question?
As far as I know, commits are indeed squashed and the pr description is used to create the description of the commit, but I'm not 100% sure.

Yes, in most cases we do a squash merge and copy information from the description (with some editorializing).

The only cases we don't do that:

• Merges between beta and master
• For code refactors, where individual commits helps to track moved AND modified code. In this case the commits for modification of the code should be isolated from the commits that move code.

What does a negative selection ID mean? That doesn't sound like a valid object.

Unless you are sure and can explain why a negative Id would be returned, this should continue to be treated as an error. This may require new code, specific to OpenOffice / LibreOffice specific to handle this error.

This sounds like a bug in LibreOffice, what is the negative value? Could it be an integer overflow?

It would be helpful if @michaelDCurran could confirm please?

Thanks for explaining the workflow!

What does a negative selection ID mean? That doesn't sound like a valid object.

Unless you are sure and can explain why a negative Id would be returned, this should continue to be treated as an error. This may require new code, specific to OpenOffice / LibreOffice specific to handle this error.

This sounds like a bug in LibreOffice, what is the negative value? Could it be an integer overflow?

The central question seems to be whether or not negative child IDs are valid or not. LibreOffice actually assigns negative child IDs (that seem to be unique application-wide) and since I did not find any statement when reading the IAccessible::get_accSelection or the "How Child IDs Are Used in Parameters" doc, I was assuming that was probably valid, and only 0 would be a special value (meaning CHILDID_SELF).

Now, I've found a "How Servers Implement Child IDs" page in the MSAA docs, which says:

How Servers Implement Child IDs

Server developers can assign child IDs to both simple elements and accessible objects. However, the recommended approach is to support the standard Component Object Model (COM) interface IEnumVARIANT in every accessible object that has children.

If you implement IEnumVARIANT, you must:

• Enumerate all children, both simple elements and accessible objects. Provide child IDs for all simple elements and provide the IDispatch to each accessible object.
• For accessible objects, set the vt member of the VARIANT to VT_DISPATCH. The pdispVal member must contain a pointer to the IDispatch interface. Note that the VARIANT is allocated and freed by the client.
• For simple elements, the child ID is any 32-bit positive integer. Note that zero and negative integers are reserved by Microsoft Active Accessibility. Set the VARIANT structure vt member to VT_I4 and the lVal member to the child ID.

If you do not support IEnumVARIANT, you must assign child IDs and number the children in each object sequentially starting with one.

In the LibreOffice Calc case that #13276 is about, the children (table cells) are accessible objects, not just simple elements.
Given that, it currently seems to me like it would probably be reasonable to have get_accSelection return a variant with vt=VT_DISPATCH and the pdispVal member set to the IDispatch interface of the accessible object representing the cell, rather than just the child ID (vt=VT_IA and the lVal member set to the child ID).

In addition to changes on LibreOffice side, this might need handling the case where get_accSelection returns an IDispatch for a single selected child object here on NVDA side. (But given I'm not very experienced with NVDA development, I don't yet know exactly how that would end up here "on the Python side"). From how I read the get_accSelection doc, returning an IEnumVARIANT is primarily meant to be used when multiple children are selected.

What do you think?

If you agree this would be a reasonable approach, I would start working on the LibreOffice part first, then potentially come up with an updated PR (if changes in NVDA are needed in addition).

I think an opinion from either @jcsteh or @michaelDCurran would be very welcome here, since they've been working with IA2 for ages. As far as I know, one way to uniquely identify an IA2 object is the IAccessible2 uniqueID, which can certainly be negative. I assume with Child ID, you'd have to be compliant with the MSAA spec so avoid negative IDs if possible.

The use of negative child ids doesn't fit well into the IAccessible spec, but it has been done by IAccessible2 for a very long time and should be considered standard for all intents and purposes. A negative child id should be treated as a unique id, while a positive child id should be treated as a child index.

That said, as noted in #13277 (comment), IAccessible2 elements are always full IAccessible objects, not "simple elements". Thus, anything that returns an accessible (including accSelection) really should return an object pointer. In the case of accSelection, this means VT_DISPATCH for a single selection or VT_UNKNOWN and iEnumVARIANT (with VT_DISPATCH elements) for multiple selection.

In short, NVDA supporting negative child ids returned from accSelection isn't necessarily "wrong", but ideally, LibreOffice would be fixed to return full objects.

Thanks a lot for the explanation!

I have implemented returning an IDispatch for the selected object in LibreOffice (s. https://gerrit.libreoffice.org/c/core/+/129201 , will merge to master branch later today) and updated this PR to support the IDispatch case on NVDA side.

I'll also update PR #13233 to handle the IDispatch case in addition to the one where child IDs are returned (but that's independent of this PR here).

Strictly speaking, this change is not needed to make the announcement of a single selected cell work in LibreOffice, since that already works with only the LibreOffice change in place: An IDispatch is returned by LO, the previous exception will no longer be triggered, but a COMErrror is triggered when querying for the IEnumVARIANT interface a few lines below.
Since that COMError is handled in getSelectedItemsCount (in source\NVDAObjects\IAccessible\__init__.py) and it falls back to calling the nSelected methods on the IAccessibleTable2 interface, the result is still OK, but I think NVDA should properly handle the IDispatch case.

* FAIL: System tests. See test results for more information.

In the log, I see this test failure:

Robot.chromeTests :: HTML test cases in Chrome                                
==============================================================================
checkbox labelled by inner element :: A checkbox labelled by an in... | FAIL |
Unable to locate 'before sample' marker. Too many attempts looking for 'Before Test Case Marker' See NVDA log for full speech.

That test passes for me locally when I run ./runsystemtests.bat. Could it be that this is some failure unrelated to the PR or is there anything I can do to reproduce that locally?

Sorry about that @michaelweghorn, there is an intermittent problem that affects the chrome tests. It affects the first test to be run, which is currently that one. We plan to investigate it. Until that is fixed we have to manually re-run the build or you can push an empty commit to re-run.

I'll manually re-run this PR build. The status should update soon.

@feerrenrut: Thanks!

@michaelweghorn I see the change to support IDispatch, but there is no change to support negative child IDs... the original line still only checks that the int is positive. Is this change no longer needed? If so then the pr description (including proposed change log line) should be updated before I can merge this.

@michaelweghorn I see the change to support IDispatch, but there is no change to support negative child IDs... the original line still only checks that the int is positive. Is this change no longer needed? If so then the pr description (including proposed change log line) should be updated before I can merge this.

@michaelDCurran:This is no longer needed, since LibreOffice now (from LibreOffice 7.4 on) returns an IDispatch instead of negative child IDs, which conforms better to the MSAA spec. I have updated the PR description accordingly.

@michaelDCurran:This is no longer needed, since LibreOffice now (from LibreOffice 7.4 on) returns an IDispatch instead of negative child IDs, which conforms better to the MSAA spec. I have updated the PR description accordingly.

Since I was mostly focusing on the LibreOffice case, I took the feedback from #13277 (comment) to fix LibreOffice and adapt this PR to handle the IDispatch case as well.
If considered useful, I can also re-add support for negative child IDs given @jcsteh 's comment still mentions

The use of negative child ids doesn't fit well into the IAccessible spec, but it has been done by IAccessible2 for a very long time and should be considered standard for all intents and purposes. A negative child id should be treated as a unique id, while a positive child id should be treated as a child index.

What do you think?

Thanks, I've added a commit doing that on top.