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

LibreOffice Calc: report merged cell coordinates #9310

Closed
surfer0627 opened this issue Feb 22, 2019 · 24 comments · Fixed by #12849
Closed

LibreOffice Calc: report merged cell coordinates #9310

surfer0627 opened this issue Feb 22, 2019 · 24 comments · Fixed by #12849

Comments

@surfer0627
Copy link
Contributor

Steps to reproduce:

  1. Open Calc.
  2. Select a1 through a3.
  3. Choose Format - Merge Cells - Merge Cells.
  4. Move to cell a1.

Actual behavior:

NVDA reports: a1

Expected behavior:

NVDA reports: a1 through a3

System configuration:

NVDA Installed/portable/running from source:

Installed.

NVDA version:

2018.4.1

Windows version:

6.1.7601 service pack 1

Name and version of other software in use when reproducing the issue:

LibreOffice 6.1.3.2

Other questions:

Does the issue still occur after restarting your PC?

Yes.

Have you tried any other versions of NVDA?

NO.

@LeonarddeR
Copy link
Collaborator

LibreOffice Table Cells don't implement IAccessibleTableCell, and therefore there's no way to get the row and column span. LibreOffice itself also does not expose the merged state in the accessible name of the cell.

@Adriani90
Copy link
Collaborator

Bugs against Libre Office can be reported here:
https://bugs.documentfoundation.org/enter_bug.cgi?format=guided
Unfortunately I am not using Libre Office at all, so I guess there are people who can fill a bug with more Details than I could. @LeonarddeR could you fill one please?

@surfer0627
Copy link
Contributor Author

Related issue in LibreOffice #124832.

@Adriani90
Copy link
Collaborator

Still reproducible in Libre Office 6.4 with NVDA 2019.3 RC2

@michaelweghorn
Copy link
Contributor

LibreOffice Table Cells don't implement IAccessibleTableCell, and therefore there's no way to get the row and column span. LibreOffice itself also does not expose the merged state in the accessible name of the cell.

I've just implemented support for IAccessibleTableCell in LibreOffice (commits: https://git.libreoffice.org/core/commit/839dbf9ecf9f8fbec7de983d1a2e16d7de6f868c, https://git.libreoffice.org/core/commit/97a88e30e2e084ab860635ff4e0a03442d8a12af ). Does LibreOffice now provide everything that's necessary so that the NVDA side can be implemented or is anything else needed from LibreOffice side?

Daily build for LibreOffice (containing the changes) can be downloaded here (You'll need one that is from today or newer.): https://dev-builds.libreoffice.org/daily/master/current.html

@surfer0627
Copy link
Contributor Author

cc: @LeonarddeR
Any thoughts?

Now, LibreOffice implement for IAccessibleTableCell.

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Sep 11, 2021

@michaelweghorn Thanks for this, awesome work. Could you tell when IAccessibleTable2 was implemented on the table? I think we'll need to come with a separate implementation in NVDA based on IAccessibleTable2/TableCell availability to make sure older versions won't break.

@LeonarddeR
Copy link
Collaborator

I have the merged cells announcement working here locally, so that's a great thing.

Having the ability to properly name a selected cell range (#6897) is a different thing though. LibreOffice doesn't fire a focus event for newly selected cells with shift+arrows, it seems. The cell the selection started from is also still regarded to as focused. Therefore, nvda doesn't announce selection.

@michaelweghorn
Copy link
Contributor

@michaelweghorn Thanks for this, awesome work. Could you tell when IAccessibleTable2 was implemented on the table? I think we'll need to come with a separate implementation in NVDA based on IAccessibleTable2/TableCell availability to make sure older versions won't break.

@LeonarddeR: I've added this just this week to the current LibreOffice development version, i.e. it will be available from LibreOffice 7.3 on only (whose release is to be expected around the beginning of February 2022). So I agree that having the implementation conditional on the interface being available sounds reasonable.

I have the merged cells announcement working here locally, so that's a great thing.

Nice!

Having the ability to properly name a selected cell range (#6897) is a different thing though. LibreOffice doesn't fire a focus event for newly selected cells with shift+arrows, it seems. The cell the selection started from is also still regarded to as focused. Therefore, nvda doesn't announce selection.

Starting from your comment #6897 (comment) which also mentions that no focus event is fired, I have started looking into this a bit and trying to understand what is happening and what should potentially be happening. I need to admit that I currently do not understand what LibreOffice could/should do differently regarding the announcement of focus for that case, though. As you mention, the cell that the selection started from is still focused, which I think is the intended behavior. When you start typing, that goes to the initially selected cell, and it therefore looks correct to me that no focus is announced for other cells.

From my (limited) understanding, this is also in line with MSAA documentation on selection and focus; quoting from there:

Like many elements in applications running on Microsoft Windows operating systems, accessible objects select and receive keyboard focus. These attributes enable users to interact with application elements, change values, and otherwise manipulate them.

There are some key differences between object selection and object focus:

  • A focused object is the one object in the entire operating system that receives keyboard input. The object with the keyboard focus is either the active window or a child object of the active window.
  • A selected object is marked to participate in some type of group operation.

For example, a user can select several items in a list-view control, but the focus is given only to one object in the system at a time. Note that focused items are from a selection of items.

I think that one way that should generally work and with which I have experimented a bit locally is to override event_selectionAdd, event_selectionRemove, event_selection and event_selectionWithin on SymphonyTableCell/SymphonyTable and announce the current selection if one of those events is received.

What do you think about this approach in general? Or do you have another suggestion what LibreOffice should be doing? (I'm still quite new to the a11y world and rather at the beginning of understanding how exactly things are supposed to work, and very happy on any hints/tips.)

If you think it's useful, I'll be happy to share a test branch to at least demonstrate my idea. (I had planned to do that at some point anyway, but was still at a point where I was experimenting to get a somewhat better understanding myself before I would have started a discussion myself... I have also realized that there's a problem with selectionRemove not being properly received, which looks like a problem on LibreOffice side and some cases where events are also not properly sent from LO side, but I think that shouldn't hurt when demonstrating the general idea.)

@LeonarddeR
Copy link
Collaborator

@LeonarddeR: I've added this just this week to the current LibreOffice development version, i.e. it will be available from LibreOffice 7.3 on only (whose release is to be expected around the beginning of February 2022). So I agree that having the implementation conditional on the interface being available sounds reasonable.

That's exactly what I did now :)

Starting from your comment #6897 (comment) which also mentions that no focus event is fired, I have started looking into this a bit and trying to understand what is happening and what should potentially be happening. I need to admit that I currently do not understand what LibreOffice could/should do differently regarding the announcement of focus for that case, though. As you mention, the cell that the selection started from is still focused, which I think is the intended behavior. When you start typing, that goes to the initially selected cell, and it therefore looks correct to me that no focus is announced for other cells.

That's interesting. So to clarify, behavior in LibreOffice differs from the behavior in Office Excel where focus changes when selecting? I.e. when on A1 and pressing shift+right arrow, B1 is focused in Excel, whereas A1 is still focused in Calc?

@LeonarddeR
Copy link
Collaborator

Ah never mind, In excel the focus also stays on the cell where the selection has started from. However, Excel fires another focus event on the focused cell when the selection changes with the keyboard. I think that makes sense.

Yet I will look at the several selection events for Libre.

@LeonarddeR
Copy link
Collaborator

I think that one way that should generally work and with which I have experimented a bit locally is to override event_selectionAdd, event_selectionRemove, event_selection and event_selectionWithin on SymphonyTableCell/SymphonyTable and announce the current selection if one of those events is received.

What do you think about this approach in general? Or do you have another suggestion what LibreOffice should be doing?

I think it'll work like this. Some findings:

  1. selectionAdd and selectionRemoved are called for all the cells that get (de)selected. This means we somehow have to wait for the last event before announcing the selection. I think that's not very ideal.
  2. selection is called on cells when moving focus, it's not called when changing selection, it seems.
  3. I can't find any caeses where selectionwithin is called on the table. I think that event would be beneficial to have.

(I'm still quite new to the a11y world and rather at the beginning of understanding how exactly things are supposed to work, and very happy on any hints/tips.)

It's really awesome to have you on board!

If you think it's useful, I'll be happy to share a test branch to at least demonstrate my idea.

I'm not sure I understand you correctly. How does your idea differ from current alpha branch?

@michaelweghorn
Copy link
Contributor

That's interesting. So to clarify, behavior in LibreOffice differs from the behavior in Office Excel where focus changes when selecting? I.e. when on A1 and pressing shift+right arrow, B1 is focused in Excel, whereas A1 is still focused in Calc?

Yes, exactly, that's what I am seeing and which sounded reasonable to me in general when reading the documentation mentioned above (but I'm not an expert).

1. selectionAdd and selectionRemoved are called for all the cells that get (de)selected. This means we somehow have to wait for the last event before announcing the selection. I think that's not very ideal.

One way this could potentially be handled is by remembering the selection and only announce it if it has changed, i.e. the new selection would only be announced for event_selectionAdd called on the first cell added to the selection, and ignored for the other ones added at the same time (and whose event_selectionAdd method would be called subsequently). I have experimented with this approach in my demo branch (which for now just remembers the first and last cell in the selection, which could be adapted/extended if necessary).

2. selection is called on cells when moving focus, it's not called when changing selection, it seems.

Indeed, I also see this called only when moving focus from one cell to another one. I think for the other cases where selection changes from multiple cells to a single one, selectionRemove is generally called on the cells that are no longer part of the selection instead.
(However, with an unmodified LibreOffice development version, selectionRemove never gets called on NVDA side, which looks like a problem that needs to be fixed in LibreOffice.)

3. I can't find any caeses where selectionwithin is called on the table. I think that event would be beneficial to have.

From what I can see, this e.g. gets called when more than 10 cells are added to a selection (instead of the single selectionAdd events), e.g. when you have A1 to O1 selected, then press Shift+Down to extend the selection to be A1 to O2.
(This seems to be in line with what https://docs.microsoft.com/en-us/windows/win32/winauto/event-constants says about EVENT_OBJECT_SELECTIONWITHIN).

If you think it's useful, I'll be happy to share a test branch to at least demonstrate my idea.

I'm not sure I understand you correctly. How does your idea differ from current alpha branch?

Do you mean the LibreOffice alpha branch? I'm not sure whether it was clear I was referring to a test branch for NVDA rather than LibreOffice. I have now pushed this branch to my fork of the repo as michaelweghorn/nvda6897_tdf100086_DEMO_announce_selected_cells: michaelweghorn@31dbfaa

This is certainly not the way that an announcement should properly be done, but I think it shows a few ideas about a potential approach.

When experimenting with this, I also realized that some things are not yet properly announced from LO side, e.g. the event_selectionRemove events are missing completely and some events are missing for other cases as well. I plan to further look into this on LO side. However, I think that those remaining LibreOffice problems probably shouldn't be blocking for the work on NVDA side at the moment, since they "just" affect specific scenarios...

@LeonarddeR
Copy link
Collaborator

Thanks for all the clarifications.

(However, with an unmodified LibreOffice development version, selectionRemove never gets called on NVDA side, which looks like a problem that needs to be fixed in LibreOffice.)

That would be great.

  1. I can't find any caeses where selectionwithin is called on the table. I think that event would be beneficial to have.

From what I can see, this e.g. gets called when more than 10 cells are added to a selection (instead of the single selectionAdd events), e.g. when you have A1 to O1 selected, then press Shift+Down to extend the selection to be A1 to O2.
(This seems to be in line with what https://docs.microsoft.com/en-us/windows/win32/winauto/event-constants says about EVENT_OBJECT_SELECTIONWITHIN).

Yes, I can reproduce it now.>

If you think it's useful, I'll be happy to share a test branch to at least demonstrate my idea.

I'm not sure I understand you correctly. How does your idea differ from current alpha branch?

Do you mean the LibreOffice alpha branch? I'm not sure whether it was clear I was referring to a test branch for NVDA rather than LibreOffice.

I was referring to LibreOffice Alpha, haha.

I have now pushed this branch to my fork of the repo as michaelweghorn/nvda6897_tdf100086_DEMO_announce_selected_cells: michaelweghorn@31dbfaa

This is certainly not the way that an announcement should properly be done, but I think it shows a few ideas about a potential approach.

I loked at it and it was certainly promising. I chose a slightly different approach in LeonarddeR@b9f7ee1 that automatically ensures that duplicates are silenced.

When experimenting with this, I also realized that some things are not yet properly announced from LO side, e.g. the event_selectionRemove events are missing completely and some events are missing for other cases as well. I plan to further look into this on LO side. However, I think that those remaining LibreOffice problems probably shouldn't be blocking for the work on NVDA side at the moment, since they "just" affect specific scenarios...

True. Indeed, selectionRemove is missing. Some other things I observed:

  1. The selected state seems never be added to a focused cell, even when it is yet selected.
  2. As mentioned, I think it would help to have an event on the table or focused cell when anything changes selection wise. Currently, we can only announce selection reliably by propagating events from all the different cells to the focused cell. We can also use a separate announcement mechanism, but as said, that requires us to do the caching ourselves instead of relying on what's already in NVDA with regard to object property speech caching. The idea of making the selection announcement happen using the focused cell was borrowed from Excel.

@michaelweghorn
Copy link
Contributor

Thanks for all the clarifications.

(However, with an unmodified LibreOffice development version, selectionRemove never gets called on NVDA side, which looks like a problem that needs to be fixed in LibreOffice.)

That would be great.

I have looked into this and fixed it in LibreOffice:
https://git.libreoffice.org/core/commit/836a226205df9e76e77d26af80f402de7b876d61

I loked at it and it was certainly promising. I chose a slightly different approach in leonardder@b9f7ee1 that automatically ensures that duplicates are silenced.

Thanks a lot! That looks really great.
(The only thing I spotted was a typo, "throuhg" instead of "through for the merged cell case.)

True. Indeed, selectionRemove is missing. Some other things I observed:

1. The selected state seems never be added to a focused cell, even when it is yet selected.

I can reproduce that and have looked a bit into it from LibreOffice side. I'll possibly work on a fix at some point. (I have some different potential approaches in mind. One which would be a rather quick fix by handling that specific case, another more fundamental change that would get rid of some double bookkeeping in the document model and on the Win a11y part of LibreOffice, but the latter might not be feasible or at least requires more work since other scenarios (unrelated to our Calc selection one) rely on the current way of how it is handled.) In case it helps here, I can offer to work on a rather quick fix for that specific case now; otherwise I'd probably keep that in mind as a potential task for "sometime later" (and then reconsider whether or not to do the bigger change).

2. As mentioned, I think it would help to have an event on the table or focused cell when anything changes selection wise. Currently, we can only announce selection reliably by propagating events from all the different cells to the focused cell. We can also use a separate announcement mechanism, but as said, that requires us to do the caching ourselves instead of relying on what's already in NVDA with regard to object property speech caching. The idea of making the selection announcement happen using the focused cell was borrowed from Excel.

Agreed. I still can't really come up with an idea myself that makes me really happy so far, though, but I may be missing something. Announcing focus on cells that are not really focused (which I understood Excel is doing) feels a bit odd to me. I was wondering a bit whether just sending a SELECTIONWITHIN event regardless of how exactly selection was adapted (and no SELECTIONADD or SELECTIONREMOVE events) would be a good idea, but don't really know yet...

https://docs.microsoft.com/en-us/windows/win32/winauto/event-constants says this for EVENT_OBJECT_SELECTIONWITHIN:

Numerous selection changes have occurred within a container object. The system sends this event for list boxes; server applications send it for their accessible objects.
This event is sent when the selected items within a control have changed substantially. The event informs the client that many selection changes have occurred, and it is sent instead of several EVENT_OBJECT_SELECTIONADD or EVENT_OBJECT_SELECTIONREMOVE events. The client queries for the selected items by calling the container object's IAccessible::get_accSelection method and enumerating the selected items.
For this event notification, the hwnd and idObject parameters of the WinEventProc callback function describe the container in which the changes occurred.

(From what I have read so far, using SelectionAdd and SelectionRemove still appears somewhat "cleaner" to me at least in theory, but from what I have seen so far, it doesn't really make things easier on either side.)

@LeonarddeR
Copy link
Collaborator

I have looked into this and fixed it in LibreOffice:
https://git.libreoffice.org/core/commit/836a226205df9e76e77d26af80f402de7b876d61

Great, I'll test with that.

(The only thing I spotted was a typo, "throuhg" instead of "through for the merged cell case.)

Thanks, will fix that.

  1. The selected state seems never be added to a focused cell, even when it is yet selected.

I can reproduce that and have looked a bit into it from LibreOffice side. I'll possibly work on a fix at some point. (I have some different potential approaches in mind. One which would be a rather quick fix by handling that specific case, another more fundamental change that would get rid of some double bookkeeping in the document model and on the Win a11y part of LibreOffice, but the latter might not be feasible or at least requires more work since other scenarios (unrelated to our Calc selection one) rely on the current way of how it is handled.) In case it helps here, I can offer to work on a rather quick fix for that specific case now; otherwise I'd probably keep that in mind as a potential task for "sometime later" (and then reconsider whether or not to do the bigger change).

It's not super important I think.

  1. As mentioned, I think it would help to have an event on the table or focused cell when anything changes selection wise. Currently, we can only announce selection reliably by propagating events from all the different cells to the focused cell. We can also use a separate announcement mechanism, but as said, that requires us to do the caching ourselves instead of relying on what's already in NVDA with regard to object property speech caching. The idea of making the selection announcement happen using the focused cell was borrowed from Excel.

Agreed. I still can't really come up with an idea myself that makes me really happy so far, though, but I may be missing something. Announcing focus on cells that are not really focused (which I understood Excel is doing) feels a bit odd to me.

NO, Excel announces focus on the currently focused cell when the focused cell takes part in a changing selection. SO no focus events are fired on cells that aren't focused, at least that's what I understand from it.

@michaelweghorn
Copy link
Contributor

Thanks for the clarification.

NO, Excel announces focus on the currently focused cell when the focused cell takes part in a changing selection. SO no focus events are fired on cells that aren't focused, at least that's what I understand from it.

Ah, that was a misunderstanding on my side then.

@michaelweghorn
Copy link
Contributor

I have thought over focus/selection a bit more and experimented a bit from a user perspective with both, Excel/NVDA on Windows and LibreOffice/Orca on Linux.
While I am still a bit undecided on the question on whether it makes sense to send an extra focus event for the currently focused cell when selection changes (like Excel seems to do it), I currently tend to think that there may be some value to keep the two separate. (Semantically, there is no change in focus, and thus "no need" to send a corresponding focus event.)

Orca handles the two differently, as shown e.g. by this "opposite" example (where focus changes while selection remains unchanged):

  • select various cells, e.g. A1:D10
  • press Tab several times to switch focus between the different cells in the selection

In that scenario:

  • NVDA always repeats the current selection of cells in Excel
  • Orca only announces the newly focused cell and "ignores" selection

@michaelweghorn
Copy link
Contributor

While I am still a bit undecided on the question on whether it makes sense to send an extra focus event for the currently focused cell when selection changes (like Excel seems to do it), I currently tend to think that there may be some value to keep the two separate. (Semantically, there is no change in focus, and thus "no need" to send a corresponding focus event.)

For clarification, those thoughts refer to the question of whether LibreOffice should send an extra focus event or not (not about whether or not NVDA should apply a separate handling for the case of selected cells vs. focus change in general).

@CyrilleB79
Copy link
Collaborator

* NVDA always repeats the current selection of cells in Excel

* Orca only announces the newly focused cell and "ignores" selection

FYI, this point regarding Excel was discussed in #6959. I had a prototype branch to mimick Orca's behaviour, but people were rather asking to report both focus and selection and I haven't worked further on it.

michaelweghorn added a commit to michaelweghorn/nvda that referenced this issue Sep 17, 2021
@michaelweghorn
Copy link
Contributor

After seeing the "accSelection is broken in LibreOffice" comment in PR #12849, I have taken a look at this. The problem is that the IEnumVARIANT being used in subsequent calls to the IAccessible::get_accSelection method is the same, and it's not reset in between, i.e. another call to IEnumVARIANT::Next will only return those selected items not already considered during previous calls.

Essentially, a call to IAccessible::get_accSelection is needed. I see two ways to do this:

@LeonarddeR: Do you have any opinion here? Does that fix the brokenness you have observed or are there more problems?

@michaelweghorn
Copy link
Contributor

Essentially, a call to IAccessible::get_accSelection is needed. I see two ways to do this [...]

Sorry, that was a copy-paste mistake, should be: "Essentially, a call to IEnumVARIANT::Reset is needed."

@LeonarddeR
Copy link
Collaborator

Wow, this is great, thanks @michaelweghorn. I'd be glad to take michaelweghorn@5b03875 . I think it makes sense to fix this in LibreOffice as well, but this will at least work for other cases where no freshIEnumVARIANT is returned for AccSelection.

LeonarddeR pushed a commit to LeonarddeR/nvda that referenced this issue Sep 18, 2021
@michaelweghorn
Copy link
Contributor

@LeonarddeR: Great, thanks. I've just merged the LibreOffice change, so that will be fixed in LibreOffice 7.3 as well.

seanbudd pushed a commit that referenced this issue Oct 20, 2021
….3 and above (#12849)

Link to issue number:
Fixes #9310
Fixes #6897

Summary of the issue:
In LibreOffice calc, selection is not announced and for merged cells, only the first cell is announced.

Description of how this pull request fixes the issue:
In LibreOffice 7.3 development branch, support for IAccessibleTable2 and IAccessibleTableCell were added. This allows us to implement something that works as expected.
@nvaccessAuto nvaccessAuto modified the milestone: 2021.3 Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants