Opened 4 years ago

Closed 4 months ago

Last modified 3 months ago

#1987 closed enhancement (fixed)

Add the ability for NVDA to read charts in Excel

Reported by: elliott94 Owned by: mdcurran
Milestone: Component: Microsoft Office support
Keywords: Cc: lecky_lists@nextra.sk, ondrej.rosik@gmail.com
Blocked by: #4838 Blocking:

Description

When working with data in worksheets, it sometimes becomes necessary to create charts out of several cells in a worksheet.

Unfortunately, NVDA doesn't currently support these. The attached example shows a Bar Chart, with the labeled cells A1, B1, and C1. It would be great to add support for these, if at all possible.

Attachments (4)

Test_Chart_1.xls (16.0 KB) - added by elliott94 4 years ago.
A test Bar Chart.
nvda.log (2.5 KB) - added by dineshkaushal 8 months ago.
For comment 17
a.xlsx (20.4 KB) - added by peter 4 months ago.
attachment-comment-45.7z (2.0 KB) - added by ondrosik 4 months ago.
Comment 45

Download all attachments as: .zip

Change History (51)

Changed 4 years ago by elliott94

A test Bar Chart.

comment:1 Changed 17 months ago by deependra230

Hello,
I am working on this ticket. I understand the basic functioning of 'excel.py' but i couldn't find anything related to charts' interface on the web. Moreover, there is no default way to navigate inside the chart/graph. Can someone please suggest where to start and how to proceed ?
I know its quite a vague question but i am stuck at the start.

comment:2 Changed 9 months ago by dineshkaushal

Please review the branch in_t1987_f7

https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

The problem of excel chart elements being announced multiple times after every alt tab is solved. Thanks to Jamie, the problem is solved using weakref.

comment:3 Changed 9 months ago by jteh

  • Component changed from Core to Microsoft Office support
  • Keywords needsCodeReview added
  • Owner set to mdcurran

comment:4 Changed 9 months ago by jteh

  • Status changed from new to assigned

comment:5 Changed 9 months ago by mdcurran

Great work again so far.

Some things to change:

  1. When object navigating next or previous from a chartElement, you end up on another window outside the chart. At a minimum you should set previous and next properties to None on ExcelchartElementBase. If possible though, it would be cool to implement next and previous properties that actually fetched the next and previous chart element. However this may be impossible to do.
  2. I don't think there is a need to provide all those redirect scripts on ExcelChartElementBase. Instead, on the scripts of the chart, set canPropagate to True. As the chart is the parent, NVDA will automatically allow matching on these. For example:
    script_reportTitle.canPropagate=True
    
  3. Provide translator comments for all your translation strings.
  4. the _select method on ExcelChartElementBase and subclasses should be renamed to something like _getLabel. _select really suggests that it will do something, rather than get something.
  5. We have a role_chart in controlTypes. This can be used for the chart's role.

comment:6 Changed 9 months ago by mdcurran

Also, this branch is still based on the work for the NVDA+f7 list in Excel. We should have a discussion soon around how that code can be rewritten to make use of the browse mode abstraction work in #2975, so that Excel can have quick nav to comments and formulas and such, and how that elements list can also list charts. But for now, only the points in my last comment are important.

comment:7 follow-up: Changed 9 months ago by dineshkaushal

Thank you Mike.

I have made the changes as per your recommendation.

  1. previous and next properties for ExcelChartElement are now set to None. Since these elements get focus from excel events, I don't know how to set actual previous and next element.
  2. removed the redirect scripts and added canPropogate for those scripts.
  3. added translators comments
  4. _select renamed to _getChartElementText
  5. changed role to ROLE_CHART for ExcelChart
  6. added classes for the objects that do some processing. ExcelChartElementBase handles default prompts for other elements.

I need your help for reporting color for the series. I have provided the similar implementation that is found in excel for color reporting, but NVDA announces rgb(red=x,green = y , blue = z).
Please check def script_reportCurrentChartElementColor(self,gesture) in ExcelChartElementBase

the branch is at in_t1987_f7
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

comment:8 follow-up: Changed 9 months ago by dineshkaushal

Forgot to ask:

where can I find the code for element list for browse mode? I tried, but I didn't find it in excel.py in branch next and t2975.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 9 months ago by mdcurran

Replying to dineshkaushal:
Great.
For colors, you need to use the RGB object's name property.

ui.message ( _( "Series color: {} ").format(colors.RGB.fromCOLORREF(int( self.excelChartObject.SeriesCollection( self.arg1 ).Interior.Color ) ).name  ) )

comment:10 in reply to: ↑ 8 ; follow-up: Changed 9 months ago by mdcurran

Replying to dineshkaushal:
New abstracted browse mode and elements list is part of #2975. Branch t2975.
However, its only been added to Microsoft Word so far. No work has been done on Excel.
Most of the abstraction is in browseMode.py, but you can see a real implementation in NVDAObjects/window/winword.py.

comment:11 in reply to: ↑ 9 Changed 9 months ago by dineshkaushal

Fixed the reporting of colors using the name property.
Thank you very much

comment:12 in reply to: ↑ 10 Changed 9 months ago by dineshkaushal

I have started working on integrating browseMode. I am seeking a clarification.

Do we need to implement TextInfo? as per the following code, it looks that we need to implement TextInfo.

class TextInfoQuickNavItem(QuickNavItem):

def activate(self):

self.textInfo.activate()

Should we discuss these issues on this ticket or on #2975?
Replying to mdcurran:

Replying to dineshkaushal:
New abstracted browse mode and elements list is part of #2975. Branch t2975.
However, its only been added to Microsoft Word so far. No work has been done on Excel.
Most of the abstraction is in browseMode.py, but you can see a real implementation in NVDAObjects/window/winword.py.

comment:13 follow-up: Changed 9 months ago by jteh

No, you shouldn't implement a TextInfo. TextInfoQuickNavItem provides a base implementation of several QuickNavItem methods to avoid duplication if you do have a TextInfo. If you don't, you inherit from QuickNavItem directly and implement those methods (such as {{{lt_}} (less than comparison), label, activate, isChild, moveTo, etc.) yourself. See the QuickNavItem class for details.

comment:14 in reply to: ↑ 13 Changed 9 months ago by dineshkaushal

Then will I also implement

class BrowseModeTreeInterceptor(treeInterceptorHandler.TreeInterceptor):

Because script BrowseModeTreeInterceptor.activatePosition calls self._activatePosition(info)

comment:15 follow-up: Changed 9 months ago by jteh

Hmm. We'll need to discuss that. For now, just subclass BrowseModeTreeInterceptor. If you want enter to work, just override script_activatePosition. If we change anything related to this, it won't have a significant impact on your code.

comment:16 in reply to: ↑ 15 Changed 8 months ago by dineshkaushal

I have done first implementation, but I am unable to make the code work. After implementing the code, no excel keys are working.
branch in_t1987_bm
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

Changed 8 months ago by dineshkaushal

For comment 17

comment:17 Changed 8 months ago by dineshkaushal

The current implementation works somewhat, but there is an error on selecting current chart. The following error is there.

AttributeError: 'ExcelBrowseModeTreeInterceptor' object has no attribute 'makeTextInfo'

See the attached log file for more details.

branch in_t1987_bm

​​https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

comment:18 Changed 8 months ago by mdcurran

Just a heads up that QuicknavItem has been slitely changed in master, changeset:e7c90a3 to fix #4767. This will affect this ticket, so you'll want to merge master and made the necessary change.

  • QuickNavItem.moveTo now only should do the move, but not any reporting. It also therefore no longer takes any keyword arguments.
  • QuicknavItem.report method has been introduced, this method should handle reporting of the item. It takes a readUnit keyword argument like moveTo did, which you may or may not use.

comment:19 follow-up: Changed 7 months ago by dineshkaushal

I merged master with in_t1987_bm, but I could not find the change.

I then checked out master, and I can still not find the changed definition for QuickNavItem in browseMode.py file.

Which branch should I checkout?

comment:20 in reply to: ↑ 19 Changed 7 months ago by mdcurran

Replying to dineshkaushal:
A current checkout of master for me, source/browsemode.py: the new report method starts at line 103.
The moveTo method is at line 111.

comment:21 follow-up: Changed 7 months ago by dineshkaushal

I got the changes from master, but I am still getting the error for MakeTextInfo

comment:22 in reply to: ↑ 21 ; follow-up: Changed 7 months ago by mdcurran

Replying to dineshkaushal:
The makeTextInfo error is not related to the other changes. However, I'll look at that makeTextInfo bug today. I meant to do this a while ago.

comment:23 Changed 7 months ago by mdcurran

  • Blocked by 4838 added

comment:24 in reply to: ↑ 22 Changed 7 months ago by mdcurran

Replying to mdcurran:
Please merge t4838 into your branch. This will stop the makeTextInfo error.
Also a few other things:

  • In my copy of in_t1987_bm, it seems excelChart is not imported in source/NVDAObjects/window/excel.py. You won't be able to import excelChart at the top as you'll get a circular import error as excelChart imports excel and tries to use excelBase. But instead you can import excelChart in _getSelection just before you use excelChart.ExcelChart.

comment:25 Changed 7 months ago by dineshkaushal

Added chart, comment and formula listing options in the browse mode.

branch in_t1987_bm
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

comment:26 Changed 7 months ago by mdcurran

Some final code review:

  • As you've initially copied some of the QuicknavItem stuff from winword.py, some of the docstrings and comments are still mentioning Microsoft Word. Please rewrite these to refer to Excel.
  • It may be an idea to rename collectionFromRange to collectionFromWorksheet as that is what it now deals with in Excel.
  • Would it be an idea to create a base class for ExcelCommentQuickNavItem, ExcelFormulaQuickNavItem etc, that implements __lt__ moveTo, and isAfterSelection, so that the same code isn't duplicated for all those classes? Though, please excuse me if the code in each is slightly different. If so then don't worry about this. But from a quick glance it looked pretty much the same.

comment:27 Changed 7 months ago by dineshkaushal

Thanks Mike,
I have fixed all the doc strings refering to word, created ExcelRangeBasedQuickNavItem class as base class for ExcelCommentQuickNavItem and ExcelFormulaQuickNavItem, and renamed collectionFromRange to collectionFromWorksheet.

comment:28 Changed 7 months ago by mdcurran

I've now branched in_t1987_bm to t1987 on NV Access.
I've made the following changes:

  • Fixed broken submodules. When pulling or merging from this branch, do a git submodule update after to ensure you have the correct submodules and that you don't accidentally commit old ones.
  • Excel's treeInterceptor now is created always, though is set to passThrough by default. Also the elementsList script has been overridden on the treeInterceptor to allow it to work in passthrough mode. In short, now when in Excel, you can press NVDZA+f7 to get the elemts list with out having to press NVDA+space first.
  • I rewrote ExcelQuickNavIterator.iterate as the code was picking up some incorrect cells due to Excel's rante object's not handling item indexing in a good way. It seems indexing simply accesses adjacent cells in the sheet, rather than in the actual range. Therefore, for now we just iterate over the range itself. This works fine for the elements list, but this would have to be rethought if we wanted to start supporting quick navigation keys from arbitrary locations in a sheet.
  • Excel's treeInterceptor now only stays alive while this worksheet is the active sheet. This means that when switching sheets, elements list will show the correct data for the sheet.
    • NVDA now fires a focus event in moveTo for comments/formulars/charts, as Excel's activate method does not do this itself. Therefore, now NVDA announces hat you land on after moving to it from the elements list.

Take a look at my work and provide any feedback. If everyone is happy, then I can prepare to merge this into next.

comment:29 Changed 7 months ago by dineshkaushal

Testing is done, you can now integrate this with next.

Thanks for your help.

comment:30 Changed 7 months ago by Michael Curran <mick@nvaccess.org>

  • Status changed from assigned to incubating

In f372f6ef6f974a6509fddfd0592af1f99f718faa:

Merge branch 't1987' into next. Incubates #1987

comment:31 Changed 6 months ago by Michael Curran <mick@nvaccess.org>

In 99683fbd296aa0dfc11cba0d125ca6b8f45d78b8:

Merge branch 't1987' into next. Incubates #1987

comment:32 Changed 6 months ago by jteh

  • Keywords needsCodeReview removed

comment:33 Changed 5 months ago by mdcurran

were there any outstanding bugs on this implementation? I thought one was mentioned on a call but can't quite remember.

comment:34 Changed 5 months ago by dineshkaushal

Modified the code, changed shortcut for comments from c to o.

get the code from in_t1987_temp

comment:35 Changed 5 months ago by Michael Curran <mick@nvaccess.org>

In dc92f310f686836a18fa961964096e50b078bfac:

Merge branch 't1987' into next. Incubates #1987

comment:36 Changed 4 months ago by JamaicanUser

Will this functionality be available for the next release? I find it very handy.

comment:37 Changed 4 months ago by jteh

Barring any major issues, yes. See ReleaseProcess.

comment:38 Changed 4 months ago by Michael Curran <mick@nvaccess.org>

  • Resolution set to fixed
  • Status changed from incubating to closed

In 410b90b49177265c4c7641b07e59b55e37df6fc0:

Merge branch 't1987'. Fixes #1987

comment:39 Changed 4 months ago by JamaicanUser

Don't know where to report this, but when I enter Input Help mode in Excel and press the Elements List dialog command (which I think should be renamed in the User guide to Microsoft Excel Elements List to avoid confusion), the description states headings, links and landmarks. This should say charts, cells with comments and cells with formulas.

comment:40 Changed 4 months ago by James Teh <jamie@nvaccess.org>

In 3625417aa166a97d7853b032ec815d0c1d4fddc9:

Fix the command description for the Elements List command in Microsoft Excel, which previously incorrectly mentioned links, headings and landmarks.

Re #1987.

comment:41 Changed 4 months ago by JamaicanUser

Is there a special reason this is not listed under the 2015.2 milestone fixes?

comment:42 Changed 4 months ago by ondrosik

does this work for excel 2003? When I look at the example chart, i see one chart but for some reason it says it is placed on e13 m 38. When I press enter, It says the name, type, and testcel1, testcel 2 but I can not navigate. I tryed also another random chart from the internet with same result e. g. the navigation by arrow keys doesn't work. but maybe I am missing something important, I newer used charts in ms excel at all. I am testing this for translation purposes.

comment:43 Changed 4 months ago by ondrosik

OK, I werified it, it works with office 2007. So the question is: Will we support 2003 or will we state that this is supported in 2007 and newer? I know that office 2003 is really old piece of software, I use it because I hate rybbons so it is mostly my lazyness.

Changed 4 months ago by peter

comment:44 Changed 4 months ago by peter

Hello,
I have one particular document where i canot see list of formulas. Steps to reproduce:

  1. open attachment attached above
  2. press nvda+f7 and choose formula
  3. nvda will now freeze for few seconds so wait a moment and then check the list of formulas, it's empty.

I have latest master branch and office 2010.

comment:45 Changed 4 months ago by ondrosik

I can reproduce this with Excel 2007 but it seems that sometimes I see the formulas in the list, sometimes not. It seems that it is somehow related with the cursor position. When i move the cursor around in some cases i get the list with formulas. I thought that it depends on particular column but it doesn't when i am on A4 i sometimes get the formulas, sometimes don't.
I am providing log for both cases but be avare that it is too long, see attached file.

Changed 4 months ago by ondrosik

Comment 45

comment:46 Changed 4 months ago by peter

  • Cc lecky_lists@nextra.sk added

comment:47 Changed 3 months ago by ondrosik

  • Cc ondrej.rosik@gmail.com added
Note: See TracTickets for help on using tickets.