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

Checkable list #7491

Merged
merged 36 commits into from Aug 14, 2018
Merged

Checkable list #7491

merged 36 commits into from Aug 14, 2018

Conversation

derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Aug 13, 2017

Edited and updated by @leonardder

Link to issue number:

closes #7325
closes #4357

Summary of the issue:

  1. Create an accessible check list box, so that each item is a checkbox. This list box only supports one column
  2. 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).
  3. 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.

  1. class ListCtrlAccPropServer(IAccPropServer_Impl): to implement the checkable list's AccpropServer, to apply the accessibility fixes.
  2. 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.
  3. 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:

  1. The checkable list box doesn't support position info.
  2. 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

derek riemer added 2 commits Aug 13, 2017
@josephsl
Copy link
Collaborator

@josephsl josephsl commented Aug 13, 2017

@derekriemer
Copy link
Collaborator Author

@derekriemer derekriemer commented Aug 13, 2017

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

Copy link
Collaborator

@leonardder leonardder left a comment

Some little comments on coding style :)

PROPID_ACC_NAV_LASTCHILD = GUID("{302ecaa5-48d5-4f8d-b671-1a8d20a77832}")

class IAccPropServer_Impl(COMObject):
"""Base class for implementing a COM interace for AccPropServer.
Copy link
Collaborator

@leonardder leonardder Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix typo: interace


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).
Copy link
Collaborator

@leonardder leonardder Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this in two lines

Copy link
Collaborator Author

@derekriemer derekriemer Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where?

self.control = control
super(IAccPropServer_Impl, self).__init__(*args, **kwargs)

def _getPropValue(self, pIDString, dwIDStringLen, idProp):
Copy link
Collaborator

@leonardder leonardder Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@derekriemer derekriemer Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@derekriemer
Copy link
Collaborator Author

@derekriemer derekriemer commented Sep 26, 2017

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

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jan 16, 2018

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
Copy link
Collaborator Author

@derekriemer derekriemer commented Jan 25, 2018

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
Copy link
Collaborator

@leonardder leonardder commented Jan 25, 2018

Thanks.

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Feb 17, 2018

@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
Copy link
Collaborator Author

@derekriemer derekriemer commented Mar 7, 2018

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

Copy link
Collaborator

@leonardder leonardder left a comment

Some small things


#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).
Copy link
Collaborator

@leonardder leonardder May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered storing these in the oleacc module?

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

@leonardder leonardder May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I"d do this slightly different.

  1. Store the possible modifiers in a list on keyboardHandler, just the key names
  2. 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
Copy link
Collaborator

@leonardder leonardder commented Jul 3, 2018

@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
Copy link
Collaborator Author

@derekriemer derekriemer commented Jul 5, 2018

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

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Jul 6, 2018

Hi,

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

Thanks.

@derekriemer derekriemer self-assigned this Jul 6, 2018
@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jul 18, 2018

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
Copy link
Collaborator

@leonardder leonardder commented Jul 18, 2018

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

@leonardder leonardder requested a review from feerrenrut Jul 25, 2018
@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jul 27, 2018

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

  1. 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.
  2. 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.

Copy link
Member

@feerrenrut feerrenrut left a comment

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

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)
Copy link
Member

@feerrenrut feerrenrut Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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).
Copy link
Member

@feerrenrut feerrenrut Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@leonardder leonardder Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@feerrenrut feerrenrut Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@leonardder leonardder Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

@josephsl josephsl commented Jul 30, 2018

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jul 30, 2018

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

@feerrenrut feerrenrut self-requested a review Jul 30, 2018
Copy link
Member

@feerrenrut feerrenrut left a comment

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

source/oleacc.py Outdated
#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}")
Copy link
Member

@feerrenrut feerrenrut Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Aug 8, 2018

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
Copy link
Collaborator

@leonardder leonardder commented Aug 8, 2018

@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
Copy link
Member

@feerrenrut feerrenrut commented Aug 8, 2018

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
Copy link
Collaborator

@josephsl josephsl commented Aug 8, 2018

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Aug 14, 2018

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.

@feerrenrut feerrenrut merged commit 32351be into nvaccess:master Aug 14, 2018
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants