-
-
Notifications
You must be signed in to change notification settings - Fork 747
Handle aria-current=false as False instead of True #7901
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
abdd6dc
Handle aria-current=false as False instead of True
bramd 604861c
Fix docstring for NVDAObject._get_isCurrent
bramd f3fcbda
Merge remote-tracking branch 'upstream/master' into i7892
bramd 5c11b86
Address review comments
bramd 31d90a2
Improve comment about aria-current regex in Edge
bramd 6276d9b
Merge remote-tracking branch 'upstream/master' into i7892
bramd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -457,18 +457,19 @@ def _get_ariaProperties(self): | |
| # RegEx to get the value for the aria-current property. This will be looking for a the value of 'current' | ||
| # in a list of strings like "something=true;current=date;". We want to capture one group, after the '=' | ||
| # character and before the ';' character. | ||
| # This could be one of: True, "page", "step", "location", "date", "time" | ||
| RE_ARIA_CURRENT_PROP_VALUE = re.compile("current=(\w+);") | ||
| # This could be one of: "false", "true", "page", "step", "location", "date", "time" | ||
| # "false" is ignored, so it falls back to the default implementation, which is false | ||
| RE_ARIA_CURRENT_PROP_VALUE = re.compile("current=(?!false)(\w+);") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regex seems a bit broken:
May be we need a unit test for this. cc @feerrenrut |
||
|
|
||
| def _get_isCurrent(self): | ||
| ariaProperties=self._getUIACacheablePropertyValue(UIAHandler.UIA_AriaPropertiesPropertyId) | ||
| match = self.RE_ARIA_CURRENT_PROP_VALUE.match(ariaProperties) | ||
| match = self.RE_ARIA_CURRENT_PROP_VALUE.search(ariaProperties) | ||
| log.debug("aria props = %s" % ariaProperties) | ||
| if match: | ||
| valueOfAriaCurrent = match.group(1) | ||
| log.debug("aria current value = %s" % valueOfAriaCurrent) | ||
| return valueOfAriaCurrent | ||
| return False | ||
| return None | ||
|
|
||
| def _get_placeholder(self): | ||
| ariaPlaceholder = self.ariaProperties.get('placeholder', None) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the latter false be none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a comment about the regular expression, which is outside of the isCurrent method and the false here refers to the false as value of the aria-current attribute. That's also the reason why it is written without a capital F. So, I think this comment is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on Line 461 confuses me a little too. It sounds like it is talking about the default implementation of the
isCurrentmethod, perhaps you could clear this up? I think it would be clearer to say something like:# "false" is ignored by the regEx and will not produce a matchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I just commited the improved comment