Add accessibility PRESUBMIT check for element attribute getters
This check will help stop a pattern that should not be used in the accessibility code because it could bypass ElementInternals and give incorrect results on custom elements. AX-Relnotes: N/A Bug: 372169467 Change-Id: I008c9433a22c9b6723da9e91b640390b86f4dade Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6036605 Reviewed-by: Andrew Grieve <agrieve@chromium.org> Reviewed-by: Aaron Leventhal <aleventhal@chromium.org> Commit-Queue: Aaron Leventhal <aleventhal@chromium.org> Commit-Queue: Mark Schillaci <mschillaci@google.com> Cr-Commit-Position: refs/heads/main@{#1387038}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
56fc621687
commit
44c90b4456
46
PRESUBMIT.py
46
PRESUBMIT.py
@@ -5873,6 +5873,52 @@ def CheckAccessibilityRelnotesField(input_api, output_api):
|
|||||||
|
|
||||||
return [output_api.PresubmitNotifyResult(message)]
|
return [output_api.PresubmitNotifyResult(message)]
|
||||||
|
|
||||||
|
|
||||||
|
_ACCESSIBILITY_ARIA_METHOD_CANDIDATES_PATTERNS = r'(\-\>|\.)(get|has|FastGet|FastHas)Attribute\('
|
||||||
|
|
||||||
|
_ACCESSIBILITY_ARIA_BAD_PARAMS_PATTERNS = (
|
||||||
|
r"\(html_names::kAria(.*)Attr\)",
|
||||||
|
r"\(html_names::kRoleAttr\)"
|
||||||
|
)
|
||||||
|
|
||||||
|
_ACCESSIBILITY_ARIA_FILE_CANDIDATES_PATTERNS = (
|
||||||
|
r".*/accessibility/.*.(cc|h)",
|
||||||
|
r".*/ax_.*.(cc|h)"
|
||||||
|
)
|
||||||
|
|
||||||
|
def CheckAccessibilityAriaElementAttributeGetters(input_api, output_api):
|
||||||
|
"""Checks that the blink accessibility code follows the defined patterns
|
||||||
|
for checking aria attributes, so that ElementInternals is not bypassed."""
|
||||||
|
|
||||||
|
# Limit to accessibility-related files.
|
||||||
|
def FileFilter(affected_file):
|
||||||
|
paths = _ACCESSIBILITY_ARIA_FILE_CANDIDATES_PATTERNS
|
||||||
|
return input_api.FilterSourceFile(affected_file, files_to_check=paths)
|
||||||
|
|
||||||
|
aria_method_regex = input_api.re.compile(_ACCESSIBILITY_ARIA_METHOD_CANDIDATES_PATTERNS)
|
||||||
|
aria_bad_params_regex = input_api.re.compile(
|
||||||
|
"|".join(_ACCESSIBILITY_ARIA_BAD_PARAMS_PATTERNS)
|
||||||
|
)
|
||||||
|
problems = []
|
||||||
|
|
||||||
|
for f in input_api.AffectedSourceFiles(FileFilter):
|
||||||
|
for line_num, line in f.ChangedContents():
|
||||||
|
if aria_method_regex.search(line) and aria_bad_params_regex.search(line):
|
||||||
|
problems.append(f"{f.LocalPath()}:{line_num}\n {line.strip()}")
|
||||||
|
|
||||||
|
if problems:
|
||||||
|
return [
|
||||||
|
output_api.PresubmitPromptWarning(
|
||||||
|
"Accessibility code should not use element methods to get or check"
|
||||||
|
"\nthe presence of aria attributes"
|
||||||
|
"\nPlease use ARIA-specific attribute access, e.g. HasAriaAttribute(),"
|
||||||
|
"\nAriaTokenAttribute(), AriaBoolAttribute(), AriaBooleanAttribute(),"
|
||||||
|
"\nAriaFloatAttribute().",
|
||||||
|
problems,
|
||||||
|
)
|
||||||
|
]
|
||||||
|
return []
|
||||||
|
|
||||||
# string pattern, sequence of strings to show when pattern matches,
|
# string pattern, sequence of strings to show when pattern matches,
|
||||||
# error flag. True if match is a presubmit error, otherwise it's a warning.
|
# error flag. True if match is a presubmit error, otherwise it's a warning.
|
||||||
_NON_INCLUSIVE_TERMS = (
|
_NON_INCLUSIVE_TERMS = (
|
||||||
|
@@ -1289,6 +1289,95 @@ class AccessibilityRelnotesFieldTest(unittest.TestCase):
|
|||||||
'Expected %d messages, found %d: %s' % (0, len(msgs), msgs))
|
'Expected %d messages, found %d: %s' % (0, len(msgs), msgs))
|
||||||
|
|
||||||
|
|
||||||
|
class AccessibilityAriaElementAttributeGettersTest(unittest.TestCase):
|
||||||
|
|
||||||
|
# Test warning is surfaced for various possible uses of bad methods.
|
||||||
|
def testMatchingLines(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
MockFile(
|
||||||
|
"third_party/blink/renderer/core/accessibility/ax_object.h",
|
||||||
|
[
|
||||||
|
"->getAttribute(html_names::kAriaCheckedAttr)",
|
||||||
|
"node->hasAttribute(html_names::kRoleAttr)",
|
||||||
|
"->FastHasAttribute(html_names::kAriaLabelAttr)",
|
||||||
|
" .FastGetAttribute(html_names::kAriaCurrentAttr);",
|
||||||
|
|
||||||
|
],
|
||||||
|
action='M'
|
||||||
|
),
|
||||||
|
MockFile(
|
||||||
|
"third_party/blink/renderer/core/accessibility/ax_table.cc",
|
||||||
|
[
|
||||||
|
"bool result = node->hasAttribute(html_names::kFooAttr);",
|
||||||
|
"foo->getAttribute(html_names::kAriaInvalidValueAttr)",
|
||||||
|
"foo->GetAriaCurrentState(html_names::kAriaCurrentStateAttr)",
|
||||||
|
],
|
||||||
|
action='M'
|
||||||
|
),
|
||||||
|
]
|
||||||
|
|
||||||
|
results = PRESUBMIT.CheckAccessibilityAriaElementAttributeGetters(mock_input_api, MockOutputApi())
|
||||||
|
self.assertEqual(1, len(results))
|
||||||
|
self.assertEqual(5, len(results[0].items))
|
||||||
|
self.assertIn("ax_object.h:1", results[0].items[0])
|
||||||
|
self.assertIn("ax_object.h:2", results[0].items[1])
|
||||||
|
self.assertIn("ax_object.h:3", results[0].items[2])
|
||||||
|
self.assertIn("ax_object.h:4", results[0].items[3])
|
||||||
|
self.assertIn("ax_table.cc:2", results[0].items[4])
|
||||||
|
self.assertIn("Please use ARIA-specific attribute access", results[0].message)
|
||||||
|
|
||||||
|
# Test no warnings for files that are not accessibility related.
|
||||||
|
def testNonMatchingFiles(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
MockFile(
|
||||||
|
"content/browser/foobar/foo.cc",
|
||||||
|
["->getAttribute(html_names::kAriaCheckedAttr)"],
|
||||||
|
action='M'),
|
||||||
|
MockFile(
|
||||||
|
"third_party/blink/renderer/core/foo.cc",
|
||||||
|
["node->hasAttribute(html_names::kRoleAttr)"],
|
||||||
|
action='M'),
|
||||||
|
]
|
||||||
|
results = PRESUBMIT.CheckAccessibilityAriaElementAttributeGetters(mock_input_api, MockOutputApi())
|
||||||
|
self.assertEqual(0, len(results))
|
||||||
|
|
||||||
|
# Test no warning when methods are used with different attribute params.
|
||||||
|
def testNoBadParam(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
MockFile(
|
||||||
|
"third_party/blink/renderer/core/accessibility/ax_object.h",
|
||||||
|
[
|
||||||
|
"->getAttribute(html_names::kCheckedAttr)",
|
||||||
|
"->hasAttribute(html_names::kIdAttr)",
|
||||||
|
],
|
||||||
|
action='M'
|
||||||
|
)
|
||||||
|
]
|
||||||
|
|
||||||
|
results = PRESUBMIT.CheckAccessibilityAriaElementAttributeGetters(mock_input_api, MockOutputApi())
|
||||||
|
self.assertEqual(0, len(results))
|
||||||
|
|
||||||
|
# Test no warning when attribute params are used for different methods.
|
||||||
|
def testNoMethod(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
MockFile(
|
||||||
|
"third_party/blink/renderer/core/accessibility/ax_object.cc",
|
||||||
|
[
|
||||||
|
"foo(html_names::kAriaCheckedAttr)",
|
||||||
|
"bar(html_names::kRoleAttr)"
|
||||||
|
],
|
||||||
|
action='M'
|
||||||
|
)
|
||||||
|
]
|
||||||
|
|
||||||
|
results = PRESUBMIT.CheckAccessibilityAriaElementAttributeGetters(mock_input_api, MockOutputApi())
|
||||||
|
self.assertEqual(0, len(results))
|
||||||
|
|
||||||
|
|
||||||
class AndroidDeprecatedTestAnnotationTest(unittest.TestCase):
|
class AndroidDeprecatedTestAnnotationTest(unittest.TestCase):
|
||||||
|
|
||||||
def testCheckAndroidTestAnnotationUsage(self):
|
def testCheckAndroidTestAnnotationUsage(self):
|
||||||
|
Reference in New Issue
Block a user