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

Fix misleading log message for aria-current="true" #11782

Merged
merged 15 commits into from Feb 3, 2021

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Oct 23, 2020

Link to issue number:

None

Summary of the issue:

There is a misleading log message for aria-current="true". It was a non-handled value, an exception was raised, a message added to the log, and the value True (bool rather than str) was used.

When trying to follow this code it is hard to know where the values are strings, None, or bool. In some cases the naming is overly specific.

Description of how this pull request fixes the issue:

Types and naming for isCurrent were confusing, so normalize the types
using an enum and only name as ariaCurrent when the data is specific to aria-current,
otherwise use 'isCurrent'. Include comments to link aria-current as the
potential (likely) source for 'isCurrent'

Testing performed:

Used Chrome, Firefox, IE, and Edge with https://a11ysupport.io/tests/html/aria/aria-current.html

Known issues with pull request:

This is technically an API breaking change. If there are any concerns about this, we may have to wait for the 2021.1 release.

Change log entry:

changes for developers:
- 'NVDAObject' (and derivatives) property 'isCurrent' now strictly returns 'controlTypes.IsCurrent'. It is no longer Optional, and thus will not return None. When an object is not current 'controlTypes.IsCurrent.NO' is returned.
- The 'controlTypes.isCurrentLabels' has been removed, instead use the 'displayString' property on a 'controlTypes.IsCurrent' enum value. EG 'controlTypes.IsCurrent.YES.displayString'

Types and naming for isCurrent were confusing, so normalize the types
using an enum and only name as ariaCurrent when the data is specific to aria-current,
otherwise use 'isCurrent'. Include comments to link aria-current as the
potential (likely) source for 'isCurrent'
@AppVeyorBot

This comment has been minimized.

@feerrenrut feerrenrut added this to the 2021.1 milestone Oct 26, 2020
@feerrenrut feerrenrut added the deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release label Oct 26, 2020
LeonarddeR
LeonarddeR previously approved these changes Nov 20, 2020
source/controlTypes.py Outdated Show resolved Hide resolved
source/controlTypes.py Outdated Show resolved Hide resolved
source/speech/__init__.py Outdated Show resolved Hide resolved
LeonarddeR
LeonarddeR previously approved these changes Nov 24, 2020
source/controlTypes.py Outdated Show resolved Hide resolved
- Use a property
- shorten name
@feerrenrut
Copy link
Contributor Author

@LeonarddeR I have addressed your suggestions. I think making this a property is a good idea. I decided against translation because it is hard to search for usages in the code base. Instead I have gone with displayString which communicates that it is for displaying to the user. This also leaves room for other transformations (other than translation) to happen to this string prior to being presented to a user.

source/controlTypes.py Outdated Show resolved Hide resolved
LeonarddeR
LeonarddeR previously approved these changes Dec 31, 2020
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

Great @feerrenrut

@feerrenrut
Copy link
Contributor Author

I have just noticed inconsistent case for IsCurrent.Yes all the rest of the values are uppercase. I'll fix this.

@feerrenrut
Copy link
Contributor Author

Python docs for enum use upper case for members of the enum, so I'll match that style.

@AppVeyorBot
Copy link

See test results for failed build of commit afe27d9a62

michaelDCurran
michaelDCurran previously approved these changes Jan 25, 2021
@AppVeyorBot
Copy link

See test results for failed build of commit 66cf02e9be

@feerrenrut feerrenrut merged commit 94f60a8 into master Feb 3, 2021
@feerrenrut feerrenrut deleted the fixMisleadingAriaCurrentLogWarning branch February 3, 2021 07:59
aaclause added a commit to aaclause/BrailleExtender that referenced this pull request Feb 7, 2021
@seanbudd seanbudd mentioned this pull request Mar 5, 2021
18 tasks
XLTechie added a commit to XLTechie/xlnvda that referenced this pull request May 14, 2021
…fferently. Picked the best lines from both.
XLTechie added a commit to XLTechie/xlnvda that referenced this pull request May 20, 2021
…fferently. Picked the best lines from both.
feerrenrut pushed a commit that referenced this pull request May 21, 2021
Changes from #11782 were listed twice, though slightly differently. Picked the best lines from both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants