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

Inconsistent reporting of checkable list items #7076

Closed
dkager opened this issue Apr 14, 2017 · 17 comments · Fixed by #7279
Closed

Inconsistent reporting of checkable list items #7076

dkager opened this issue Apr 14, 2017 · 17 comments · Fixed by #7279
Labels
p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority
Milestone

Comments

@dkager
Copy link
Collaborator

dkager commented Apr 14, 2017

Can be reproduced in many lists with checkable items in Windows 10, e.g. the disk clean-up utility.
Uncecked items are brailled as Foo sel ( ) chk, checked ones as Foo (x) sel chk.

@dkager
Copy link
Collaborator Author

dkager commented May 26, 2017

According to @LeonarddeR the cause of this inconsistency is that positive states (checked, selected) are brailled before negative states (unchecked). This makes it non-trivial to correct. The priority isn't very high, since all information is still shown, just not in a consistent order.

@LeonarddeR
Copy link
Collaborator

Note that this issue isn't braille only, it is reproducible in speech as well.
@dkager, could you change the issue title accordingly?

I think we should consider to discard the selected state for these...

@dkager dkager changed the title Inconsistent reporting of checkable list items in braille Inconsistent reporting of checkable list items May 26, 2017
@jcsteh
Copy link
Contributor

jcsteh commented May 26, 2017

Giving this p4, as I think the impact of fixing this is relatively low and the cost to fix is quite high. I think it'd also be quite difficult to come up with good rules here for all possible cases.

@jcsteh jcsteh added the p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority label May 26, 2017
@LeonarddeR
Copy link
Collaborator

@jcsteh: I looked at the code which should be responsible for this if I get it correctly, and I'm probably missing something, because it seems quite easy to me.

Example: In speech.getSpeechTextForProperties, create a new set.

allStates=positiveStates | negativeStates
for st in allStates:
if st in positiveStates:
stateLabels.append(controlTypes.stateLabels[st])
elif st in negativeStates:
stateLabels.append(controlTypes.negativeStateLabels.get(st, _("not %s")%controlTypes.stateLabels[st]))

@jcsteh
Copy link
Contributor

jcsteh commented May 31, 2017

That definitely solves part of the problem; I didn't think of that. Nice thinking. However, you still need to decide on an order for the states. A set is unordered; you can't rely on the order in any way. I guess you could sort allStates numerically just so the order is determinate.

I was originally wondering whether we need some kind of defined priority order for states; i.e. whether some states (regardless of positive or negative) are more relevant than others. However, we don't have that right now anyway (all positive come before negative), so that could be a future piece of work if needed.

@LeonarddeR
Copy link
Collaborator

A set is unordered; you can't rely on the order in any way.

Ah, so that's where I made an incorrect assumption, I thought that a set was always same order based on hash, regardless of the order you'd put them in the set.

So we can do at least two things here:

  1. Just implement it like Inconsistent reporting of checkable list items #7076 (comment). I tested this and this fixes the example brought in by @dkager in Inconsistent reporting of checkable list items #7076 (comment)

    Checked: Foo (x) sel chk
    Not checked: Foo ( ) sel chk

  2. Do a numerical sort. This way, Output will always be the same, but for the example, it will change order

    Checked: Foo sel (x) chk
    Not checked: Foo sel ( ) chk

@LeonarddeR
Copy link
Collaborator

@jcsteh: Having thought some more about this, I think it will be ok if we'd just do a numerical sort of the merged positive and negative states. This might change the order of presentation people are accustomed to in some situations. However, given how python sets work, the consistence of the current behaviour is unreliable anyway.

I'd also like to have your opinion about discarding the selected state for ROLE_CHECKBOX, i.e. does this state make sense in case of check boxes, even when they are part of a list control?

In order to test things, I've enabled showing check boxes in the windows 10 explorer (ribbonn>view tab). I don't belief it is possible to have something selected which is not checked, or something checked which is not selected.

@dkager
Copy link
Collaborator Author

dkager commented Jun 12, 2017

@LeonarddeR Do you mean that you see either "Foo not selected 1 of 10" or "Foo 1of 10", depending on if it is selected? That is, you never see "Foo selected 1 of 10". And you're saying that with a checkable list item that behavior should be the same?

@LeonarddeR
Copy link
Collaborator

@LeonarddeR Do you mean that you see either "Foo not selected 1 of 10" or "Foo 1of 10", depending on if it is selected? That is, you never see "Foo selected 1 of 10". And you're saying that with a checkable list item that behavior should be the same?

Here is a more detailed str:

  1. In Windows Explorer, press alt+v,h, t to enable item check boxes
  2. Go to the desktop
  3. When arrowing around the desktop, NVDA will say "Checkbox checked selected" for every item
  4. When using object navigation, NVDA will say "check box not checked" for unselected items
  5. Holt ctrl and press space multiple times. You Toggle between "checked selected" and "not checked"
  6. Open a folder in Explorer.
  7. When arrowing around, NVDA will say "Checkbox checked" for every item
  8. When using object navigation, NVDA will say "not checked not selected" for unselected items
  9. Holt ctrl and press space multiple times. You Toggle between "checked selected" and "not checked not selected"

In both situations above, I think that the selected state can be discarded.

@dkager
Copy link
Collaborator Author

dkager commented Jun 12, 2017

[...]
When arrowing around the desktop, NVDA will say "Checkbox checked selected" for every item

Interesting: it doesn't say that if you have the checkbox display disabled. That's why I thought you only wanted to remove the "selected" state but keep the "not selected" one.

In both situations above, I think that the selected state can be discarded.

I agree, but I am not sure that it can be generalized. In this case it only holds because the definition of the setting is that you can select using checkboxes. But in other programs like Disk Clean-up you could have a list item that is not selected while it is checked. I guess Disk Clean-up is a bad example. In any case, maybe your suggestion can be applied to the Explorer app module?

@jcsteh
Copy link
Contributor

jcsteh commented Jun 13, 2017

I don't think you can generalise that a check box list item can never be selected. I can't think of usage in the wild, but an example might be a list of things you can enable or disable wherein you can select multiple items and perform a command to "Enable selected" or "Disable selected".

Note that we don't report selected for items which have the selectable state because selected is usually the default from a user perspective. Instead, we report not selected when appropriate.

I haven't tested this, but I'm guessing that when you turn on the Explorer check box selection thingy, items lose the selectable state. That would explain why you hear "selected" in that case.

I agree reporting both doesn't make sense in this Explorer case, but I think that's Explorer specific.

LeonarddeR pushed a commit to BabbageCom/nvda that referenced this issue Jun 13, 2017
…ame order, regardless whether they are positive or negative. Fixes nvaccess#7076
@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Jun 23, 2017

@jcsteh: Before I ask you to review #7279, may be you can give some hints about where to start to provide unit tests for this? Do you want a unit test only for states, or do you want to have it on a broader scale (e.g. for consistent output of speech.getSpeechTextForProperties)?

@LeonarddeR
Copy link
Collaborator

@jcsteh commented:

I was originally wondering whether we need some kind of defined priority order for states; i.e. whether some states (regardless of positive or negative) are more relevant than others.

This is actually very easy to do, and I wonder whether we really should postpone this. For example, I think the invalid entry state has more priority than the required state.

@jcsteh
Copy link
Contributor

jcsteh commented Jun 29, 2017

@leonardder commented on 24 Jun 2017, 03:15 GMT+10:

@jcsteh: Before I ask you to review #7279, may be you can give some hints about where to start to provide unit tests for this? Do you want a unit test only for states, or do you want to have it on a broader scale (e.g. for consistent output of speech.getSpeechTextForProperties)?

The more unit tests, the better, but for this PR, I'm happy for you to just do unit tests for states. At a minimum, you want to verify that you're achieving what you set out to achieve; e.g. test selected + checked and selected + unchecked.

@LeonarddeR
Copy link
Collaborator

Thanks @jcsteh: tests are in b869827

@derekriemer
Copy link
Collaborator

@LeonarddeR now that I'm mostly done with an accessible checkable list, is there likelyhood of this being done?

@LeonarddeR
Copy link
Collaborator

I belief this is ready for another review. I provided tests as @jcsteh requested. Honestly, this would be a nice to have for #588 as well.

@nvaccessAuto nvaccessAuto added this to the 2018.1 milestone Jan 1, 2018
michaelDCurran pushed a commit that referenced this issue Jan 1, 2018
…e or negative (#7279)

* In speech and braille output, states will now always be reported in same order, regardless whether they are positive or negative. Fixes #7076

* Adressed review comment

* Create a new processAndLabelStates function in controlTypes and use that in the speech and braille modules

* Added unit tests for states

* Small fix for negative state unit test

* Make use of assertSetEqual and use de setUp method to set an object to test on

* Added additional controlTypes unit tests, not particularly related to this pr

* Added doc strings

* Review actions

* Switch to processAndLabelStates for browseMode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants