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

Add additional element types to the elements list in browse mode #588 #6131

Merged
merged 15 commits into from
Aug 31, 2017
Merged

Add additional element types to the elements list in browse mode #588 #6131

merged 15 commits into from
Aug 31, 2017

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jul 2, 2016

From ticket #588:

If made in a future release, have controls on web pages be apart of the elements list, via the Alt-C command, listing all control items that are shown on a web page, which could also be filtered with the alt-f command.
For example, names of buttons, check boxes, edit boxes, combo boxes, etc. Then press enter or Alt-A to activate, focused control within element's list, or Alt-M to move to, focused control, while in element's list.

I agree with this proposal where it proposes adding additional elements to the elements list. This pull request does the following:

  • Add additional elements to the elements list, namely buttons, form fields, edit boxes, radio buttons, combo boxes, check boxes and graphics. Their shortcut keys correspond with the quicknav keys in browse mode
  • Rewrite virtualBuffers.VirtualBufferQuickNavItem.label to accomplish correct labelling of the different element types
  • Add REASON_ELEMENTSLIST to controlTypes in order to not show expanded and collapsed states in the elements list, as these states conflict with expanding/collapsing items in the the tree view. State filtering is done with controlTypes.getPositiveStates and controlTypes.getNegativeStates

Fixes #588

@feerrenrut
Copy link
Contributor

With so many radio button options a dropdown menu would make more sense, since it is a lot more compact. However some logic will need to be added to ensure that the keyboard shortcuts still work.

@jcsteh
Copy link
Contributor

jcsteh commented Dec 9, 2016

@michaelDCurran, I think we discussed this a while ago, but clearly didn't document our conclusions. :( I'm not convinced we need to have all of these element types; the use case is sketchy at best. I can probably see the point in form fields, graphics and maybe buttons, but I don't see any use in listing separate form field types.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Dec 9, 2016 via email

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Dec 9, 2016 via email

@Brian1Gaff
Copy link

Brian1Gaff commented Dec 11, 2016 via email

@derekriemer
Copy link
Collaborator

derekriemer commented Dec 11, 2016 via email

@LeonarddeR
Copy link
Collaborator Author

Have been thinking about this a little longer, and I wonder, is it clear for everyone that a button is a form field? Any how, if we remove buttons and only have form fields, it won't be possible to activate buttons from the elements list.

@feerrenrut: I assume four radio buttons isn't a problem from a visual perspective? Excel elements list has five of them.

@jcsteh
Copy link
Contributor

jcsteh commented Dec 15, 2016

I understand your arguments regarding check boxes, edit fields, etc. Nevertheless, I'd prefer to leave these out for now. We can always add them later if there's high demand, but it's much harder to remove them later. :) So, please go with form fields and buttons for now.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Dec 15, 2016 via email

@jcsteh
Copy link
Contributor

jcsteh commented Dec 15, 2016 via email

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Dec 15, 2016 via email

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Dec 15, 2016 via email

@feerrenrut
Copy link
Contributor

@LeonarddeR It's hard to say. It will really depend on the length of the text and the size of the dialog. There are different recommendations depending on where you look. The following two links from ux.stackexchange are interesting (radio-buttons-or-dropdown-for-same-list-of-options and dropdown-vs-radio-button). Things may need to be shuffled around so that the options can be laid out vertically. I can take a look at the PR and we can discuss it further if you like. Please mention me on it when you are ready.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut, thanks for your info. The current state of the request is actually ready for review. :)

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

I've had a look at this. I think the number over radio buttons is ok. I would not want to add more though.

I terms of the implementation, I'm not very familiar with this area of code. It looks ok to me, but I'm unlikely to spot issues that may crop up in code that depends on the changed areas. Perhaps @jcsteh will have a quick look over it?

@@ -109,14 +109,30 @@ def obj(self):

@property
def label(self):
if self.itemType == "landmark":
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see some documentation added here. Specifically talking about what label is and how its expected to be used. I realise that you didn't introduce this, but since you are in the area and have taken the time to understand this code it would be beneficial if this was documented better.

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation for this property belongs in browseMode.TextInfoQuickNavItem.label, as that's where the base property is defined. Only because this touches a completely different file, I'd suggest we leave this for this PR.

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Thanks! The essentials look great, though I do have some concerns regarding the states stuff.

value = super(VirtualBufferQuickNavItem,self).label
# Translators: Reported label in the elements list for an element which which has no name and value
unlabeled = _("Unlabeled")
if self.itemType is not "heading":
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be easier to follow if you move the heading code first, since there's only a tiny bit of code for headings and a lot for other stuff. In fact, you can just return early for headings and then you can get rid of a whole extra level of indentation for the rest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point there.

name += " "
return name + aria.landmarkRoles[attrs["landmark"]]
realStates=attrs["states"]
positiveStates = " ".join(controlTypes.stateLabels[st] for st in controlTypes.processPositiveStates(roleRaw, realStates, controlTypes.REASON_ELEMENTSLIST, realStates))
Copy link
Contributor

Choose a reason for hiding this comment

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

This states stuff seems a bit arbitrary at the moment.

  • Do we really need states at all here? Sure, I guess this means you get to hear "check box not checked" or the like, but I'm not convinced we need that in the Elements List anyway. If the user is using the list, they are trying to find a specific control. If they want an overview of the form's values, they should be using quick navigation, no?
  • I'm a bit bothered by introducing REASON_ELEMENTSLIST when it's really only used for states calculation. Furthermore, we just use it to kill the collapsed state... but why the collapsed state? If we really do want specific states, I feel we should perhaps be explicit about that. Otherwise, we could just be continually adding more and more exceptions in controlTypes.
  • Perhaps we can answer both of these questions if you can give me some specific use cases you had in mind here.
  • At the very least, we'd need some comments here explaining why these choices were made.

Copy link
Collaborator Author

@LeonarddeR LeonarddeR Jan 10, 2017

Choose a reason for hiding this comment

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

  • Do we really need states at all here? Sure, I guess this means you get to hear "check box not checked" or the like, but I'm not convinced we need that in the Elements List anyway. If the user is using the list, they are trying to find a specific control. If they want an overview of the form's values, they should be using quick navigation, no?

One reason to do it like this, is that the other screen readers with elements lists (SuperNova and JAWS) also show the element states. Note that one who uses the elements list doesn't necessarily use it to find a particular element, but can also use it to get an overview of elements as well as review input in a form. I agree that quick navigation has preference in most, but not all cases. Imagine the case where you have a form full of check boxes, including an "i agree" one. An elements list with states would allow a user to quickly jump to that particular control with first letter navigation, thereby hearing the current state.

  • I'm a bit bothered by introducing REASON_ELEMENTSLIST when it's really only used for states calculation. Furthermore, we just use it to kill the collapsed state... but why the collapsed state? If we really do want specific states, I feel we should perhaps be explicit about that. Otherwise, we could just be continually adding more and more exceptions in controlTypes.

I added this because reporting the collapsed state doesn't make any sense in the elements list, as a collapsible item like a combo box isn't usually expanded when you are in the elements list. It is one of the states you wouldn't bother about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jcsteh: Do the explanationss above sound sensible to you?
I've been thinking about it a bit more and I agree that REASON_ELEMENTSLIST should be avoided here. We can discard the collapsed state in the elements list code anyway, if we still want that.

@LeonarddeR
Copy link
Collaborator Author

@jcsteh or @feerrenrut, this is ready for another review if you'd like.

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Sorry it's taken me so long to get back to this.

Thanks for the explanation and use case regarding why you're including states. It makes sense. That said:

  1. I would still prefer that we got rid of REASON_ELEMENTSLIST. As you noted, we can put code to discard specific states in QuickNavItem.label.
  2. The decision as to what to include still seems a bit arbitrary to me. For example, we only include positive states for buttons, but both positive and negative for other form fields. Why? There might be a good reason, but if there is, it needs to be commented.
  3. Speaking of commenting, it'd be good if you could include some comments with example output for each case you handle. That might make it easier to see what's going on here.

@LeonarddeR
Copy link
Collaborator Author

Sorry it's taken me so long to get back to this.

No problem

  1. I would still prefer that we got rid of REASON_ELEMENTSLIST. As you noted, we can put code to discard specific states in  QuickNavItem.label .

No problem. The question is though, what reason should I use instead? REASON_QUERY seems not ok, since that will keep states like STATE_READONLY for elements other than ROLE_EDITABLETEXT and ROLE_CHECKBOX. Or shouldn't we bother about that?

  1. The decision as to what to include still seems a bit arbitrary to me. For example, we only include positive states for buttons, but both positive and negative for other form fields. Why? There might be a good reason, but if there is, it needs to be commented.

It has been too long ago I wrote this code. I see no valid reason for why I did this, so Negative states will now be reported for buttons as well. We do have toggle buttons where this is relevant.

  1. Speaking of commenting, it'd be good if you could include some comments with example output for each case you handle. That might make it easier to see what's going on here.

I will do that as soon as the reason issue has been tackled.

@LeonarddeR
Copy link
Collaborator Author

Ah, REASON_QUERY also keeps STATE_LINKED, so we definitely need to discard some states. Alternatively, we can just pass None as the reason, but I do not prefer that...

@LeonarddeR
Copy link
Collaborator Author

Sorry for spamming so much. I got one additional alternative, REASON_QUICKNAV. Since the label property is part of a TextInfoQuickNavItem, this sounds quite reasonable.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jun 29, 2017

This is perhaps a tiny bit ugly and assumes that ControlField key names are the same as NVDAObject attribute names, but that's definitely true for the attributes you need here.

I actually like this approach a lot. In the case of UIA, it will not work for landmark as that is no object property, but we can work around that of course. We can also consider setting a landmark property on the Edge UIA objects by default, which could be added to #7328. But, I'm reluctant to do such a thing.

@accessaces
Copy link

I'd like to see this fully implemented soon as it will really help in my work testing websites for developers. Being able to show them a list of all the elements or separate elements is key. I often have to switch to jaws to get screenshots of what graphic labels are or screenshots of different form controls. I would really like you to add the graphics and individual form controls.it would be key to accessibility testers like myself to be able to show these types of elements separately and together less often together. This would mean one last task I have to do in jaws when it comes to writing reports on improper labeled controls elements and so forth.

@jcsteh
Copy link
Contributor

jcsteh commented Aug 2, 2017

@accessaces commented on 27 Jul 2017, 10:54 GMT+10:

I would really like you to add the graphics and individual form controls.it would be key to accessibility testers like myself to be able to show these types of elements separately and together less often together.

While accessibility testing is a use case we obviously want to support as much as we can, we can't do that at the expense of normal usage. Adding all of these options clutters the UI significantly, and as I explained in previous comments, there's no real world use case for this (in normal usage). If we did want to support all of these elements for accessibility testing, perhaps this could be done in an add-on somehow.

@jcsteh
Copy link
Contributor

jcsteh commented Aug 2, 2017

@LeonarddeR, status update? Were you going to make changes based on #6131 (comment)?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Aug 2, 2017

I've actually been waiting for #7328 to be merged in Master, which it is now. I can start working on this again in the near future.

@jcsteh commented on 2 aug. 2017 03:18 CEST:

Were you going to make changes based on #6131 (comment)?

Yes.

@LeonarddeR
Copy link
Collaborator Author

@jcsteh: this is ready for another round of review.

@@ -196,6 +196,44 @@ def isAfterSelection(self):
caret=self.document.makeTextInfo(textInfos.POSITION_CARET)
return self.textInfo.compareEndPoints(caret, "startToStart") > 0

def _getLabelForProperties(self, labelPropertyGetter):
"""
Fetches required properties for this L{TextInfoQuickNavItem} and constructs a label to be shown in an elements list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a line after this which is something like the following:

This can be used by subclasses to implement the L{label} property.

def _getLabelForProperties(self, labelPropertyGetter):
"""
Fetches required properties for this L{TextInfoQuickNavItem} and constructs a label to be shown in an elements list.
@Param labelPropertyGetter: A callable taking 1 argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please clarify what the argument is; e.g. "taking one argument specifying the property to fetch".

"""
Fetches required properties for this L{TextInfoQuickNavItem} and constructs a label to be shown in an elements list.
@Param labelPropertyGetter: A callable taking 1 argument.
For example, if L{itemType} is landmark, the callable must return the landmark type when the landmark argument is provided to it.
Copy link
Contributor

Choose a reason for hiding this comment

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

'when the landmark argument is provided to it': This would be clearer as something like 'when "landmark" is passed as the property argument'

Fetches required properties for this L{TextInfoQuickNavItem} and constructs a label to be shown in an elements list.
@Param labelPropertyGetter: A callable taking 1 argument.
For example, if L{itemType} is landmark, the callable must return the landmark type when the landmark argument is provided to it.
Alternative attributes or dictionary keys might be name or value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps 'Alternative property names might be "name" or "value"'? I understand why you're talking about attributes/keys here, but I'm finding this confusing to read given that this thing talks about property getting.

For example, if L{itemType} is landmark, the callable must return the landmark type when the landmark argument is provided to it.
Alternative attributes or dictionary keys might be name or value.
An expected callable might be the __getattribute__ method on an L{NVDAObject}, or the get method on a L{Dict}.
Note that L{Dict.get} doesn't raise an exception, it returns None instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than talking about dict.get here, it's probably best to say that the callable must return None if the property doesn't exist rather than raising an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there such a callable for objects, instead of getattribute? For objects, we're never fetching attributes that an object does not have, so in theory, getattribute is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just use: lambda prop: getattr(obj, prop, None).

An expected callable might be the __getattribute__ method on an L{NVDAObject}, or the get method on a L{Dict}.
Note that L{Dict.get} doesn't raise an exception, it returns None instead.
"""
value = self.textInfo.text.strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to name this variable text or content rather than value, as value is confusing with respect to buttons, etc.

negativeStates = " ".join(controlTypes.negativeStateLabels[st] for st in controlTypes.processNegativeStates(role, realStates, controlTypes.REASON_FOCUS, realStates))
if self.itemType is "formField":
if role in (controlTypes.ROLE_BUTTON,controlTypes.ROLE_DROPDOWNBUTTON,controlTypes.ROLE_TOGGLEBUTTON,controlTypes.ROLE_SPLITBUTTON,controlTypes.ROLE_MENUBUTTON,controlTypes.ROLE_DROPDOWNBUTTONGRID,controlTypes.ROLE_SPINBUTTON,controlTypes.ROLE_TREEVIEWBUTTON):
labelParts = (value or name or unlabeled, roleText, positiveStates, negativeStates)
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to have a comment with example content that might be produced by each of these conditions:

# For example: "Mute toggle button pressed"

return name + aria.landmarkRoles[attrs["landmark"]]
else:
return super(VirtualBufferQuickNavItem,self).label
value = super(VirtualBufferQuickNavItem,self).label
Copy link
Contributor

Choose a reason for hiding this comment

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

This always gets executed, but it's only used for "heading". But we also fetch the text of the TextInfo in _getLabelForProperties, which results in a wasted call (this property isn't cached). So, I think this call to super should be moved inside the if block.

return super(VirtualBufferQuickNavItem,self).label
value = super(VirtualBufferQuickNavItem,self).label
if self.itemType is "heading":
# Explicitly handle headings here as well, to avoid requesting the Control field attribs
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment says "as well". As well as what? Unless I'm missing something, "as well" should be removed.

return super(VirtualBufferQuickNavItem,self).label
value = super(VirtualBufferQuickNavItem,self).label
if self.itemType is "heading":
# Explicitly handle headings here as well, to avoid requesting the Control field attribs
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way of doing this so you can rely entirely on _getLabelForProperties would be to make a lazy fetcher for control field attributes. This might be overkill, but I'll let you decide. It would go something like this:

attrs = {}
def propertyGetter(prop):
	if not attrs:
		# Lazily fetch the attributes the first time they're needed.
		# We do this because we don't want to do this if they're not needed at all.
		attrs.update(self.textInfo._getControlFieldAttribs(self.vbufFieldIdentifier[0], self.vbufFieldIdentifier[1]))
	return attrs.get(prop)

@LeonarddeR
Copy link
Collaborator Author

@jcsteh: I addressed your comments. Furthermore, I decided that it would be more readable for users to have a semicolon between the label parts ("mute; toggle button; pressed" instead of "mute toggle button pressed"). Especially when there is no role, such as for links and buttons, having the state directly behind the name or content can be misinterpreted.

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Fantastic, except for one tiny tiny nit.

"""
Fetches required properties for this L{TextInfoQuickNavItem} and constructs a label to be shown in an elements list.
This can be used by subclasses to implement the L{label} property.
@Param labelPropertyGetter: A callable taking 1 argument, specifying the property to fetch".
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious '"' character before full stop. :)

Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Oops, I did review this before incubating; just forgot to hit approve. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork Pull requests filed on behalf of Babbage B.V.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Direct Jump To Specific Element, Inside Elements List Dialog, and Elements List Enhanscement
7 participants