Handle aria-current=false as False instead of True by bramd · Pull Request #7901 · nvaccess/nvda

bramd · 2018-01-06T12:38:29Z

Link to issue number:

Summary of the issue:

Aria-current="false' is one of the allowed values according to the ARIA 1.1 spec and should be handled as False. Currently, it is handled as True.

Description of how this pull request fixes the issue:

Don't include the aria-current attribute if its' value is false or None. Previously it was only None.

Testing performed:

Tested in Firefox Nightly, Chrome, Edge and Internet Explorer in a virtual buffer as well as in focus mode.

Known issues with pull request:

None.

Change log entry:

Bug fixes
- Handle aria-current with a value of false as false instead of true (aria-current="false" is read as aria-current="true" #7892).

LeonarddeR

I'm afraid this pr is not complete yet, as it only covers virtual buffers, neither Edge browse mode nor the object level. It also seems that the doc string of the iscurrent object property is in error, as it says that a value of "true" should return the True boolean value, but I belief it never does and always returns "true" as a string.

feerrenrut · 2018-01-08T05:20:23Z

For the sake of reference aria-current support was implemented in PR #6860

bramd · 2018-01-08T20:20:20Z

The first attempt at this was incomplete. Now this PR should cover all cases and the docstring for NVDAObject._get_isCurrent has been fixed as suggested by @LeonarddeR.

LeonarddeR

This looks good, though I'm still a bit worried about the isCurrent property returning either a boolean or a string. Personally I'd make it return None instead of False, but that's unrelated. A real issue though seems to be the regex, even though that's not a regression in your code, it might be good to address that here.

LeonarddeR · 2018-01-09T14:24:29Z

@@ -458,7 +458,7 @@ def _get_ariaProperties(self):
 	# 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"


Could you update this comment to add "false" as a valid value?

I also double checked in edge. True should be listed as "true"

LeonarddeR · 2018-01-09T14:31:00Z

 	# 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+);")
+	RE_ARIA_CURRENT_PROP_VALUE = re.compile("current=(?!false)(\w+);")


This regex seems a bit broken:

RE_ARIA_CURRENT_PROP_VALUE.match("current=true;dummy=test;") properly matches

RE_ARIA_CURRENT_PROP_VALUE.match("var=val;current=true;dummy=test;") does not match

May be we need a unit test for this. cc @feerrenrut

feerrenrut · 2018-01-10T00:16:51Z

@@ -458,7 +458,7 @@ def _get_ariaProperties(self):
 	# 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"


I also double checked in edge. True should be listed as "true"

feerrenrut · 2018-01-10T00:53:35Z

-		it will return one of the following values: True, "page", "step", "location", "date", "time"
+		it will return one of the following values: "true", "page", "step", "location", "date", "time"
 		"""
 		return False


Given this returns boolean False I think it would be best if we converted "true" to boolean True. Or perhaps better would be to return one off: None, 'true', 'page', etc

* isCurrent defaults to None instead of False, so now it returns either None or one of the allowed strings ("true", "page", etc) * Use the search method on the regular expression to match the aria-current attribute in Edge, so it is also found if it's not the first attribute in the string. Also, document that false is a valid value.

LeonarddeR · 2018-01-15T19:52:39Z

-	# 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


Shouldn't the latter false be none?

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.

The comment on Line 461 confuses me a little too. It sounds like it is talking about the default implementation of the isCurrent method, 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 match

Thanks for the suggestion, I just commited the improved comment

LeonarddeR · 2018-01-26T20:08:10Z

Looks good.

Merge remote-tracking branch 'origin/pr/7901' into next

LeonarddeR suggested changes Jan 6, 2018

View reviewed changes

Handle aria-current=false as False instead of True

abdd6dc

bramd force-pushed the i7892 branch from ddb6c7d to abdd6dc Compare January 8, 2018 20:03

Fix docstring for NVDAObject._get_isCurrent

604861c

LeonarddeR suggested changes Jan 9, 2018

View reviewed changes

LeonarddeR added the feature/browse-mode label Jan 9, 2018

feerrenrut suggested changes Jan 10, 2018

View reviewed changes

bramd added 2 commits January 13, 2018 22:24

Merge remote-tracking branch 'upstream/master' into i7892

f3fcbda

LeonarddeR approved these changes Jan 15, 2018

View reviewed changes

bramd added 2 commits January 26, 2018 20:45

Improve comment about aria-current regex in Edge

31d90a2

Merge remote-tracking branch 'upstream/master' into i7892

6276d9b

feerrenrut approved these changes Jan 29, 2018

View reviewed changes

feerrenrut added a commit that referenced this pull request Jan 29, 2018

Incubates #7901

b40508d

Merge remote-tracking branch 'origin/pr/7901' into next

nvaccessAuto added the incubating label Jan 29, 2018

nvaccessAuto assigned feerrenrut Jan 29, 2018

michaelDCurran merged commit d338d58 into nvaccess:master Mar 6, 2018

nvaccessAuto removed the incubating label Mar 6, 2018

nvaccessAuto added this to the 2018.1 milestone Mar 6, 2018

michaelDCurran modified the milestones: 2018.1, 2018.2 Mar 13, 2018

Adriani90 mentioned this pull request Jan 23, 2019

aria-current="false" is read as aria-current="true" #7892

Closed

Uh oh!

Uh oh!

Conversation

bramd commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Jan 8, 2018

Uh oh!

bramd commented Jan 8, 2018

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Jan 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bramd commented Jan 6, 2018 •

edited

Loading

LeonarddeR Jan 9, 2018 •

edited

Loading