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

Read-only states for UIA objects #10494

Merged
merged 10 commits into from Jul 23, 2020
Merged

Read-only states for UIA objects #10494

merged 10 commits into from Jul 23, 2020

Conversation

francipvb
Copy link
Contributor

@francipvb francipvb commented Nov 14, 2019

Link to issue number:

None.

Summary of the issue:

For various WPF text controls, read-only state isn't added. This is because the current implementation only tries to get the ValueIsReadOnly property. However WPF text controls don't support it.

Description of how this pull request fixes the issue:

This PR adds checks and looks at the document range from the text pattern, if the control implements it. Then it checks for the Is Read-Only attribute and adds the state according to the returned value.

Testing performed:

Tested with output text view of Visual Studio 2019.

Known issues with pull request:

None known.

Change log entry:

Section: Changes

  • Now NVDA detects more read-only text UIA controls.

@francipvb
Copy link
Contributor Author

francipvb commented Nov 14, 2019

CC @leonardder @josephsl @Adriani90

@leonardder
Copy link
Collaborator

leonardder commented Nov 15, 2019

Thanks for this contribution. I think that the code will be more readable if the STATE_READONLY state is only added once in the code. Something like this:

		except COMError:
			isReadOnly=UIAHandler.handler.reservedNotSupportedValue
		if (
			isReadOnly == UIAHandler.handler.reservedNotSupportedValue
			and self.UIATextPattern
		):
			# Most UIA text controls don't support the "ValueIsReadOnly" property, so we need to
			# 	look at root document "IsReadOnly" attribute.
			document = self.UIATextPattern.documentRange
			isReadOnly = document.GetAttributeValue(UIAHandler.UIA_IsReadOnlyAttributeId)
		if isReadOnly != UIAHandler.handler.reservedNotSupportedValue:
			states.add(controlTypes.STATE_READONLY)

@francipvb francipvb marked this pull request as ready for review Nov 15, 2019
if isReadOnly != UIAHandler.handler.reservedNotSupportedValue:
states.add(controlTypes.STATE_READONLY)

if (

This comment was marked as outdated.

document = self.UIATextPattern.documentRange
isReadOnly = document.GetAttributeValue(UIAHandler.UIA_IsReadOnlyAttributeId)
# We can check "isReadOnly" again
if isReadOnly:
Copy link
Collaborator

@leonardder leonardder Nov 16, 2019

Choose a reason for hiding this comment

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

I think reservedNotSupportedValue evaluates to True. I think we either need to do

Suggested change
if isReadOnly:
if isReadOnly and isReadOnly != UIAHandler.handler.reservedNotSupportedValue:

or

Suggested change
if isReadOnly:
if isReadOnly is True:

francipvb added 3 commits Nov 16, 2019
This is to ensure "isReadOnly" is not "reservedNotSupportedValue" when returning from
document range. Additionally, this value is set if there is a "COMError".
Applied suggested change from @leonardder.
Copy link
Collaborator

@leonardder leonardder left a comment

Thanks for this.

One additional suggestion in which case this new patch improves things.

In the outlook appModule, OutlookUIAWordDocument class, the isReadonlyViewer property can now simply return controlTypes.STATE_READONLY in self.states. SO, we no longer have to rely on the object model.

I leave it up to you whether you want to file this as a separate pr after this one, or do it in this one. I think it makes sense to do it as part of this pr.

Now it reports the read-only flag according to the state if it is added
to the "states" set.
Copy link
Collaborator

@leonardder leonardder left a comment

Thanks @francipvb, confirmed that Outlook still behaves as expected with this.

@leonardder leonardder requested a review from michaelDCurran Nov 18, 2019
@leonardder
Copy link
Collaborator

leonardder commented Jul 16, 2020

@michaelDCurran I think this code looks ok. Could either you or @feerrenrut have a look? This one is pretty small, so shouldn't take longer than a minute or five.

Copy link
Member

@feerrenrut feerrenrut left a comment

Overall this looks good, thank you @francipvb . Please let me know if you won't have time to extract the method.

@@ -1231,12 +1231,28 @@ def _get_states(self):
states.add(controlTypes.STATE_INVALID_ENTRY)
if self._getUIACacheablePropertyValue(UIAHandler.UIA_IsRequiredForFormPropertyId):
states.add(controlTypes.STATE_REQUIRED)

# Read-only state
Copy link
Member

@feerrenrut feerrenrut Jul 22, 2020

Choose a reason for hiding this comment

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

Could you please extract this whole section into a new private method _getIsReadOnlyState()

@francipvb
Copy link
Contributor Author

francipvb commented Jul 22, 2020

@feerrenrut feerrenrut merged commit 124bb02 into nvaccess:master Jul 23, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.3 milestone Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants