Checkable list by derekriemer · Pull Request #7491 · nvaccess/nvda

derekriemer · 2017-08-13T20:37:09Z

Edited and updated by @LeonarddeR

Link to issue number:

closes #7325
closes #4357

Summary of the issue:

Create an accessible check list box, so that each item is a checkbox. This list box only supports one column
Created CheckableAutoWidthColumnListCtrl, which is a multi column supporting list control with accessible check boxes. From an accessibility perspective, this control behaves similar to the list control in Windows disk cleanup (cleanmgr).
Also, create an abstract AccPropServer we can make other servers from. This helps with fixing accessibility, but wraps the callback you provide with exception handling code (Since comtypes seems to squelch errors).

Description of how this pull request fixes the issue:

hcin gui\nvdaControls.py, add three classes.

class ListCtrlAccPropServer(IAccPropServer_Impl): to implement the checkable list's AccpropServer, to apply the accessibility fixes.
class CustomCheckListBox(wx.CheckListBox): to implement the checkable list, and apply the requisite fixes to the control. Also, notify WinEvent whenever an item changes state, so NVDA receives a stateChange.
class AutoWidthColumnCheckListCtrl(AutoWidthColumnListCtrl, listmix.CheckListCtrlMixin): to implement a checkable list control, and apply the requisite fixes to the control. Also, notify WinEvent whenever an item changes state, so NVDA receives a stateChange. This new class also adds a checkedItems property which is similar to the CheckedItems property on CheckListBox, i.e. it allows you to set multiple checked items upon initialisation of the control. It also allows event handlers to be registered for checked/unchecked events.

This code implements server annotation.

Drawbacks:

Server annotation is heavier than direct annotation. It requires a server to be registered for OBJID_CLIENT and CHILDID_SELF with scope of container.

Reasons I used it over direct annotation.

Direct annotation requires to implement hooks for every major function within our custom list, I.E. when a list item is inserted, appended ... we must update the new ones role. This is ugly, and unclean. Notifying state requires hooking every method of a state change, and this too is ugly. It's simple and clean to implement a server.

Resources used:

https://msdn.microsoft.com/en-us/library/windows/desktop/dd373681(v=vs.85).aspx

Testing performed:

Used the attached addon, rename from txt. Selected tools, the checkable list, checked a few items, then pressed okay.

Known issues with pull request:

The checkable list box doesn't support position info.
The checkable list control only changes the state of the check boxes when you physically click the check boxes. Therefore, it is not possible to check or uncheck the boxes with NVDA's route mouse to object command. Note that the disk cleanup manager suffers from the same issue.

Change log entry:

New features for developers

There is now an accessible CustomCheckListBox in gui.nvdaControls. Please use this over wx.CheckListBox because the latter is not accessible.
If you need to make a wx widget accessible which isn't already, it is possible to do so by using an instance of gui.accPropServer.IAccPropServer_impl. Feel use this rather than implementing your own AccPropServer, because it will gracefully handle errors. See the implementation of the checkable list box in gui/nvdaControls.py for more info.
checklist-0.1.0-dev.nvda-addon.zip

…nherit from to implement accpropServers on wx widgets.

josephsl · 2017-08-13T21:04:27Z

Hi, also applicable to #3208 provided that info such as checked state can be safely extracted from items. Thanks. Edit: Removed old email replies.

derekriemer · 2017-08-13T21:39:41Z

Oh, should be stated. I modified some stuff in winUser to remove extra tabs. The methods were like

def blah
\t\tblah
instead of 
def blah
\tblah

LeonarddeR

Some little comments on coding style :)

LeonarddeR · 2017-08-23T15:03:55Z

+PROPID_ACC_NAV_LASTCHILD = GUID("{302ecaa5-48d5-4f8d-b671-1a8d20a77832}")
+
+class IAccPropServer_Impl(COMObject):
+	"""Base class for implementing a COM interace for AccPropServer.


Please fix typo: interace

LeonarddeR · 2017-08-23T15:04:39Z

+
+class IAccPropServer_Impl(COMObject):
+	"""Base class for implementing a COM interace for AccPropServer.
+	Please override the _GetPropValue method, not GetPropValue, because GetPropValue wraps _getPropValue to catch and log exceptions (Which for some reason NVDA's logger misses when they occur in GetPropValue).


Please split this in two lines

LeonarddeR · 2017-08-23T15:06:29Z

+		self.control = control
+		super(IAccPropServer_Impl, self).__init__(*args, **kwargs)
+
+	def _getPropValue(self, pIDString, dwIDStringLen, idProp):


Could you provide explanations for the arguments in your doc string?

They are the same args used in the documentation I link to on msdn.

derekriemer · 2017-09-26T18:02:35Z

Any chance this will be be put in a specific timeframe for review? There's a lot of issues hanging off of it.

LeonarddeR · 2018-01-16T08:57:14Z

Having thought a bit more about this... I actually don't think it makes much sense to have a checkable list in core without anything using it. To clarify, I don't think incubating this in next doesn't make any sense as there is simply no way to test it.

How about implementing #4357 as part of this pr to start with?

derekriemer · 2018-01-25T05:50:11Z

That's a doable chunk of work. I simply was trying to keep implementations separate from the accessibility patch code, for commit cleanlyness, but having some usage of it in core would be useful. There are merge conflicts, I'll try to get to cleaning these up this weekend now that I have vs2017.

LeonarddeR · 2018-01-25T14:36:50Z

Thanks.

…opyright year.

LeonarddeR · 2018-02-17T14:05:21Z

@derekriemer: I wonder whether the checkable list has the possibility to disable checkboxes for certain items. Assuming it does, will that be reflected in your implementation?

derekriemer · 2018-03-07T02:11:59Z

I don't know. If it is possible, it probably doesn't reflect. Should this hold it back?

LeonarddeR

Some small things

LeonarddeR · 2018-05-24T05:08:44Z

+
+#These are the GUIDS for IAccessible properties we can override.
+#Use these to look up the GUID needed when implementing a server.
+#These are taken from oleacc.h (oleacc.idl has them too).


Have you considered storing these in the oleacc module?

LeonarddeR · 2018-05-24T05:59:44Z

+		#Translators: This is the label for a list of checkboxes
+		# controlling which keys are NVDA modifier keys.
+		modifierBoxLabel = _("&Select NVDA Modifier Keys")
+		self.modifierChoices = [


Personally, I"d do this slightly different.

Store the possible modifiers in a list on keyboardHandler, just the key names

Rather than making the names translatable, use keyLabels.localizedKeyLabels in the gui

Of course you could also store the possible modifiers around this line. Then, when saving, use modifierslist.index("insert") in self.kbdList.Checked.

You could also consider changing the config spec into a string list. Though it would require a config spec upgrade, it would create a solid reference from which people could start implementing another checkable list.

LeonarddeR · 2018-07-03T18:58:33Z

@derekriemer: do you expect any incompatibilities with WX Python 4?

it would be great if we could have this working as soon as possible after #7104 is merged, since the proposed UX of #8006 depends on this.

derekriemer · 2018-07-05T21:48:43Z

I need to (check) pun intended that once it merges.

josephsl · 2018-07-06T03:24:27Z

Hi,

@derekriemer: would you like this to be blocked until #7104 becomes reality within master?

Thanks.

LeonarddeR · 2018-07-18T07:13:03Z

I found an additional issue, it seems that only state changes for checked are reported, the not checked state is ignored, at least since WX Python 4.

Note the IA states for an item in a default CheckListBox

IAccessible accState: STATE_SYSTEM_SELECTABLE, STATE_SYSTEM_SELECTED, STATE_SYSTEM_FOCUSED, STATE_SYSTEM_FOCUSABLE, STATE_SYSTEM_VALID (3145734)

And the states for the custom checkable list:

IAccessible accState: STATE_SYSTEM_SELECTED, STATE_SYSTEM_CHECKED, STATE_SYSTEM_VALID (18)

The STATE_SYSTEM_SELECTABLE and STATE_SYSTEM_FOCUSABLE states have to be added, while STATE_SYSTEM_FOCUSED state has to be added for focused items.

Rather than returning them all by hand, I wonder whether we could get the original, unmodified state and than add our own checkable and checked states by means of a bitwise or. That would be the most elegant solution, though it might be a bit complex.

LeonarddeR · 2018-07-18T14:09:39Z

@derekriemer indicated that he's currently quite busy, so I"m taking this over for now.

… in a CheckListCtrl

…nstead. Also, for AutoWidthColumnCheckListCtrl, follow the wx behavior more strictly

LeonarddeR · 2018-07-27T10:49:48Z

@feerrenrut: Note that the code that I added and edited has been tested as follows:

The CheckListCtrl. behaves equivalent to the list control in cleanmgr. Custom WX.EVT_CHECKLISTBOX events are properly fired when checking and unchecking items. The CheckedItems property behaves as expected.
Made sure that the checkable list in keyboard settings still behaves properly. When updating the configuration, the IsChecked method is now used instead of the Checked method. See the source of the wx.core module, CheckedItems was using IsCheckd internally.

feerrenrut

The tick in the checkbox doesn't look the same as the standard checkbox. This one looks wobbly and "hand drawn"

feerrenrut · 2018-07-30T03:11:22Z

+		from IAccessibleHandler import accPropServices
+		# Register object with COM to fix accessibility bugs in wx.
+		server = ListCtrlAccPropServer(self)
+		accPropServices.SetHwndPropServer(self.Handle, winUser.OBJID_CLIENT, 0, (comtypes.GUID * 2)(*[oleacc.PROPID_ACC_ROLE,oleacc.PROPID_ACC_STATE]), c_int(2), server, 1)


Could you split these args up onto a new line each and specify the keyword. This will help people reading this to guess what the args might be for. EG 0 (third arg). Could you also define the GUID array before calling this function and ideally describe why these GUIDs / and what format they are in

feerrenrut · 2018-07-30T04:42:15Z

+		However, this makes it impossible to process toggle events other than by subclassing this class.
+		Therefore, we manually fire a wx.EVT_CHECKLISTBOX event to bind to.
+		Note that in contrast with wx.CheckListBox, for this class wx.EVT_CHECKLISTBOX is also fired
+		when checking or unchecking an item programatically (i.e. when L{Checked} or L{CheckItem} is used).


Is there a reason to do this? It seems like a likely way for logic errors to occur if the behaviour differs from built in controls EG wx.CheckListBox

I agree. The implementation of the mixin is a bit odd anyway. The only way we can create our own CHECKLISTBOX events is within the callback that the mixin defines, and that callback is also triggered when changing states programmatically. We could limit this if there is a way to find out the source of the event that calls the callback, though. I will investigate the source of the mixin.

Could we override programmatic methods to set the checkbox state (checked & checkitem), set a flag, call the parent method, then reset the flag? I suppose that doesn't work if the onCheckItem call does not happen immediately

The problem is that the mouse event handler calls CheckItem. Overriding the mouse event handler method is probably tricky because of the fact that it starts with double underscore, so its name will be mangled.

An ugly but effective way would be overriding CheckItem and then using something like inspect.currentframe().f_back.f_code.co_name to find out what method called it. If it is one of the event handlers, than call the callback, else ignore it.

josephsl · 2018-07-30T05:49:55Z

Hi, I think the oddly looking checkbox could be explained by the fact that wxPython requires CheckListBox to use owner drawn on Windows. Thanks.

…g/disabling a checkbox with space

…CKLISTBOX

LeonarddeR · 2018-07-30T08:08:47Z

@feerrenrut: I think I've been able to come up with a proper solution now.

feerrenrut

The checkmarks in the boxes are still wobbly. I will take a look at that tomorrow.

feerrenrut · 2018-08-06T08:40:09Z

+#Use these to look up the GUID needed when implementing a server.
+#Number of digits Format: "{8-4-4-4-12}"
+PROPID_ACC_NAME = GUID("{608d3df8-8128-4aa7-a428-f55e49267291}")
+PROPID_ACC_VALUE             = GUID("{123fe443-211a-4615-9527-c45a7e93717a}")


There are a lot of extra spaces before the equals sign here.

…changelog

feerrenrut · 2018-08-08T08:05:20Z

Ok, so I'm quite sure that the wobbly check mark is caused by ctypes.windll.user32.SetProcessDPIAware() in core.py

I found that the wxPython 3 demo for the checkable list did not have this issue.
I tested the same on the wxPython 4 demo, normal check marks
I found that the AutoWidthColumnCheckListCtrl does not have this issue.
I found that running the demo using our repo copy (rather than my installed version) of wxPython does not have the issue.
On a hunch I added the SetProcessDPIAware call to the start of the wx checkable list demo, and noticed the wobbly check marks.

I'm not sure what to do about it yet.

LeonarddeR · 2018-08-08T08:09:22Z

@feerrenrut commented on 8 aug. 2018 10:05 CEST:

On a hunch I added the SetProcessDPIAware call to the start of the wx checkable list demo, and noticed the wobbly check marks.

For reference, could you elaborate on what you mean with wobbly exactly? For the non-sighted among us :)

feerrenrut · 2018-08-08T08:26:59Z

For reference, could you elaborate on what you mean with wobbly exactly? For the non-sighted among us :)

Sorry! Now I know that it is related to DPI awareness, I would assume it's a scaling issue. The tick is normally a short diagonal line from the left (about a 3rd of the way from the bottom) of the box meeting the middle bottom of the box, where another longer diagonal line starts and goes towards the top right of the box.

When this tick is "wobbly" these lines are no longer smooth and straight. Now that I know its related to the DPI awareness, I expect that is the result of aliasing after being scaled (and then smoothed). The end effect is like a hand drawn tick, drawn by someone with a not so steady hand.

I hope this helps!

All other check marks in NVDA are smooth. There is still something different about the implementation of this one. I'll dig into the source for wx / wxPython next.

josephsl · 2018-08-08T14:20:29Z

Hi, should we raise this issue on wxPython mailing list? Hopefully @RobinD42 might be aware as to how to fix this from our side and/or from wxPython side of the coin. Thanks.

feerrenrut · 2018-08-14T06:14:59Z

I have filled an issue at wxWidgets/Phoenix#963.

For now I think we should go ahead with this PR. While the checkbox is pretty ugly on scaled screens, it doesn't affect functionality. The functionality this PR is blocking is much more important.

derek riemer added 2 commits August 13, 2017 14:10

Implement an accPropServer for NVDA. This is a base class users can i…

9e9ddff

…nherit from to implement accpropServers on wx widgets.

Add the checkable control

aeb7774

No need to queryInterface

87c89b5

Remove dependency on win32con

5bea2fd

derekriemer mentioned this pull request Aug 14, 2017

combining three modifier key selection check boxes into one combo box #4357

Closed

LeonarddeR suggested changes Aug 23, 2017

View reviewed changes

Review actions 1

65c1e95

derek riemer added 2 commits January 26, 2018 22:38

merge master, fix conflicts between head and master, and update the c…

87b6fb5

…opyright year.

Add initial GUI implementation with no state

5b92494

derek riemer added 2 commits May 23, 2018 15:15

Merge branch 'master' into checkableList

4d3724a

Fix merge conflicts and add state to the checklist

3f8a731

LeonarddeR suggested changes May 24, 2018

View reviewed changes

feerrenrut mentioned this pull request Jun 4, 2018

Allow add-ons to supply version compatibility information in manifests, and use this information to disable incompatible add-ons #8006

Merged

derekriemer added the blocked label Jul 6, 2018

derekriemer self-assigned this Jul 6, 2018

LeonarddeR removed the blocked label Jul 18, 2018

Merge remote-tracking branch 'origin/master' into checkableList

b6ef239

LeonarddeR requested a review from feerrenrut July 25, 2018 14:10

Leonard de Ruijter added 4 commits July 27, 2018 11:50

Fire a wx.EVT_CHECKLISTBOX event when an item is checked or unchecked…

d3cce18

… in a CheckListCtrl

Add additional comment

cf56cd5

Merge remote-tracking branch 'origin/master' into checkableList

bc27347

Checked property on CheckListBOx is deprecated, so use CheckedItems i…

04c29b9

…nstead. Also, for AutoWidthColumnCheckListCtrl, follow the wx behavior more strictly

feerrenrut suggested changes Jul 30, 2018

View reviewed changes

Leonard de Ruijter added 3 commits July 30, 2018 08:21

Review action

1a58269

Send CheckListBOx event when the mouse clicks an icon of when enablin…

71f5a23

…g/disabling a checkbox with space

Expand the AutoWidthColumnCheckListCtrl doc string to mention EVT_CHE…

90c5b7b

…CKLISTBOX

feerrenrut self-requested a review July 30, 2018 08:33

Leonard de Ruijter added 2 commits July 31, 2018 10:44

Merge remote-tracking branch 'origin/master' into checkableList

89ad69c

Removed obsolete import of inspect in nvdaControls

9c605c1

josephsl mentioned this pull request Aug 1, 2018

Use check list boxes for metadata streaming and column inclusion check lists ChrisDuffley/stationPlaylist#76

Closed

feerrenrut suggested changes Aug 6, 2018

View reviewed changes

Leonard de Ruijter added 2 commits August 6, 2018 11:16

Review action to oleacc.py

656f310

Merge remote-tracking branch 'origin/master' into checkableList, fix …

0838d37

…changelog

Merge branch 'master' into checkableList

dd788ea

feerrenrut approved these changes Aug 14, 2018

View reviewed changes

feerrenrut merged commit 32351be into nvaccess:master Aug 14, 2018

nvaccessAuto added this to the 2018.4 milestone Aug 14, 2018

feerrenrut mentioned this pull request Aug 15, 2018

Checkable list checkmarks have scaling issues. #8635

Closed

derekriemer mentioned this pull request Aug 31, 2018

Turn document formatting into a multi-category panel with lists of checkboxes #8699

Closed

Uh oh!

Conversation

derekriemer commented Aug 13, 2017 • edited by LeonarddeR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Drawbacks:

Reasons I used it over direct annotation.

Resources used:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

josephsl commented Aug 13, 2017 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekriemer commented Aug 13, 2017

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekriemer commented Sep 26, 2017

Uh oh!

LeonarddeR commented Jan 16, 2018

Uh oh!

derekriemer commented Jan 25, 2018

Uh oh!

LeonarddeR commented Jan 25, 2018

Uh oh!

LeonarddeR commented Feb 17, 2018

Uh oh!

derekriemer commented Mar 7, 2018

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Jul 3, 2018

Uh oh!

derekriemer commented Jul 5, 2018

Uh oh!

josephsl commented Jul 6, 2018

Uh oh!

LeonarddeR commented Jul 18, 2018

Uh oh!

LeonarddeR commented Jul 18, 2018

Uh oh!

LeonarddeR commented Jul 27, 2018

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsl commented Jul 30, 2018 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

derekriemer commented Aug 13, 2017 •

edited by LeonarddeR

Loading

josephsl commented Aug 13, 2017 via email •

edited by feerrenrut

Loading

LeonarddeR May 24, 2018 •

edited

Loading

josephsl commented Jul 30, 2018 via email •

edited by feerrenrut

Loading

josephsl commented Aug 8, 2018 via email •

edited by feerrenrut

Loading