Defect fixes of Mobile Accessibility Analyzer#4235
Conversation
WalkthroughThe changes refactor accessibility rule filtering by switching from Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant ActAccessibilityTesting
participant MobileAccessibilityAnalyzer
participant ReportGenerator
UI->>ActAccessibilityTesting: Trigger AnalyzerMobileAccessibility(driver, context, element)
ActAccessibilityTesting->>MobileAccessibilityAnalyzer: AnalyzerMobileAccessibility(driver, context, element, screenshot)
MobileAccessibilityAnalyzer->>MobileAccessibilityAnalyzer: Analyze rules, filter by severity/tags
MobileAccessibilityAnalyzer->>ReportGenerator: CreateMobileAccessibilityHtmlReport(issues, screenshot)
ReportGenerator-->>MobileAccessibilityAnalyzer: HTML Report
MobileAccessibilityAnalyzer->>ActAccessibilityTesting: SetMobileAccessibilityResultToAction(issues, currentAct)
ActAccessibilityTesting-->>UI: Status, error message, report
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityAnalyzer.cs(8 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityRuleDataExtensions.cs(1 hunks)
🔇 Additional comments (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityRuleDataExtensions.cs (2)
16-32: LGTM! Efficient implementation with proper case handling.The use of
HashSetfor O(1) lookups and proper handling of comma-separated tags with trimming is well implemented.
34-45: LGTM! Clean implementation with proper case-insensitive handling.Good use of
StringComparer.OrdinalIgnoreCasein the HashSet constructor.Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs (1)
887-901: LGTM! Good handling of wrapped elements and screenshot extraction.The unwrapping of
IWrapsElementand extraction of base64 screenshot data is properly implemented.Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityAnalyzer.cs (1)
39-43: LGTM! Good defensive programming with null handling.The constructor properly handles null input by initializing to an empty collection.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityAnalyzer.cs (2)
628-668: Update parameter name to reflect its content.The parameter
screenShotPathis misleading as it contains base64-encoded image data, not a file path.-public void AnalyzerMobileAccessibility(IWebDriver Driver, IWebElement element, Act currentAct, MobileAccessibilityAnalyzer mobileAnalyzer, string screenShotPath = null) +public void AnalyzerMobileAccessibility(IWebDriver Driver, IWebElement element, Act currentAct, MobileAccessibilityAnalyzer mobileAnalyzer, string screenShotBase64 = null)And update the usage:
-CreateMobileAccessibilityHtmlReport(mobileAxeResult, path, screenShotPath); +CreateMobileAccessibilityHtmlReport(mobileAxeResult, path, screenShotBase64);
799-859: Update parameter name for clarity.The parameter
screenshotPathshould be renamed to better reflect that it contains base64-encoded image data.-private void CreateMobileAccessibilityHtmlReport(List<AccessibilityIssue> issues, string outputPath, string screenshotPath = null) +private void CreateMobileAccessibilityHtmlReport(List<AccessibilityIssue> issues, string outputPath, string screenshotBase64 = null)And update the usage:
-if (!string.IsNullOrWhiteSpace(screenshotPath)) +if (!string.IsNullOrWhiteSpace(screenshotBase64)) { html.Append(" <h2>Page Screenshot</h2>"); html.Append(" <div class='screenshot-container'>"); - html.Append(" <img src='data:image/png;base64," + screenshotPath + "' alt='Screenshot of the mobile page' />"); + html.Append(" <img src='data:image/png;base64," + screenshotBase64 + "' alt='Screenshot of the mobile page' />"); html.Append(" </div>"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityAnalyzer.cs(8 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityRuleDataExtensions.cs(1 hunks)
🔇 Additional comments (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityRuleDataExtensions.cs (2)
16-32: LGTM! Well-implemented filtering logic.The
WithTagsmethod efficiently filters rules using a HashSet for O(1) lookups and properly handles edge cases. Good use of collection expressions for creating the result.
34-49: Great extraction of duplicate normalization logic!The
NormalizeTagNamesmethod successfully addresses the DRY principle by extracting the duplicate tag normalization logic fromActAccessibilityTesting. The implementation is clean and maintains proper null handling.Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs (1)
440-507: Well-refactored filtering logic with proper error handling!The
GetFilteredRuleListmethod has been successfully refactored to:
- Use
ObservableCollectionconsistently- Leverage the new
WithTagsextension method for cleaner code- Provide clear error messages when required lists are empty
- Properly handle both ByStandard and BySeverity analyzer types
The implementation is clean and maintainable.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/MobileAccessibilityAnalyzer.cs (1)
697-792: Excellent implementation of result processing logic!The
SetMobileAccessibilityResultToActionmethod provides comprehensive result processing:
- Correctly handles both ByStandard and BySeverity analyzer types
- Sets appropriate pass/fail status based on configured criteria
- Provides detailed return parameters including violation counts, lists, and severities
- Includes clear error messages that explain why the action passed or failed
The implementation is well-structured and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs(7 hunks)
🔇 Additional comments (10)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs (6)
45-45: LGTM! Necessary import addition.The import is required to support the
eRunStatusenum used in the accessibility testing handler.
606-609: LGTM! Clear error handling for unsupported functionality.The immediate failure with a descriptive error message provides good user guidance about using 'Page' target instead of element-level analysis for mobile accessibility testing.
1303-1328: LGTM! Good refactoring for consistency.The consolidation of the switch statement with proper scoping improves code structure and readability while maintaining the same functionality.
1390-1399: LGTM! Consistent scoping improvement.Adding explicit braces for switch cases improves code consistency and maintainability.
1450-1474: LGTM! Consistent scoping pattern.The explicit braces for performance data switch cases maintain consistency with the codebase style improvements.
2172-2180: LGTM! Performance improvement and consistency.The Android unlock logic now efficiently checks the device lock state before attempting unlock, preventing unnecessary operations. The iOS unlock case maintains consistent scoping with the rest of the codebase.
Also applies to: 2216-2219
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs (4)
41-41: LGTM - Required using statement for ObservableCollection.The addition of
System.Collections.ObjectModelusing statement is necessary to support the transition fromObservableListtoObservableCollection.
428-428: LGTM - Improved type consistency with .NET standards.The change from
ObservableList<AccessibilityRuleData>toObservableCollection<AccessibilityRuleData>aligns with standard .NET collection types and is appropriate for data binding scenarios.
881-901: Excellent improvements to method signature and variable naming.The updated method signature with
ISearchContext contextparameter provides better flexibility, and the screenshot extraction logic is clean and well-implemented. The variable renaming fromscreenShotPathtoscreenShotBase64correctly addresses the past review feedback about misleading naming.The wrapped element handling and error logging are also well-implemented.
972-972: Perfect implementation of DRY principle.The use of
MobileAccessibilityRuleDataExtensions.NormalizeTagNamessuccessfully addresses the previous review feedback about extracting duplicate tag normalization logic. This improves code maintainability and follows the DRY principle.
| /// <summary> | ||
| /// Filters the accessibility rule list based on selected standards and/or severity levels. | ||
| /// </summary> | ||
| /// <returns>An ObservableCollection of AccessibilityRuleData for analysis.</returns> | ||
| public ObservableCollection<AccessibilityRuleData> GetFilteredRuleList() | ||
| { | ||
| ObservableList<AccessibilityRuleData> allRules = GetRuleList(); | ||
| //all rules that are marked as Active in configuration | ||
| ObservableCollection<AccessibilityRuleData> rulesForAnalysis = | ||
| [.. GetRuleList().Where(x => x.Active)]; | ||
|
|
||
| // Start with the rules excluded by the user's configuration ('Active' field being false) | ||
| HashSet<string> finalExcludedRuleIds = new HashSet<string>( | ||
| allRules.Where(x => !x.Active).Select(x => x.RuleID), | ||
| StringComparer.OrdinalIgnoreCase | ||
| ); | ||
| // Get the selected analyzer type | ||
| string analyzerType = GetInputParamValue(ActAccessibilityTesting.Fields.Analyzer); | ||
|
|
||
| // Apply filtering based on Analyzer mode | ||
| if (GetInputParamValue(ActAccessibilityTesting.Fields.Analyzer) == nameof(eAnalyzer.ByStandard)) | ||
| // lists for selected tags and severities | ||
| List<string>? selectedStandardTags = null; | ||
| if (StandardList != null && StandardList.Any()) | ||
| { | ||
| if (StandardList == null || !StandardList.Any()) | ||
| { | ||
|
|
||
| Reporter.ToLog(eLogLevel.ERROR, "Error: 'ByStandard' analyzer selected, but no standards are provided."); | ||
|
|
||
| return new ObservableList<AccessibilityRuleData>(); | ||
| } | ||
| selectedStandardTags = StandardList.Select(i => i.Value.ToString().ToLower()).ToList(); | ||
|
|
||
| selectedStandardTags = MobileAccessibilityRuleDataExtensions.NormalizeTagNames(selectedStandardTags.ToArray()).ToList(); | ||
| } | ||
|
|
||
| HashSet<string> selectedStandardTags = new HashSet<string>( | ||
| StandardList.Select(item => item.Value.ToString().Equals("bestpractice", StringComparison.OrdinalIgnoreCase) ? "best-practice" : item.Value.ToString()), | ||
| StringComparer.OrdinalIgnoreCase | ||
| ); | ||
| List<string>? selectedSeverities = null; | ||
| if (SeverityList != null && SeverityList.Count != 0) | ||
| { | ||
| selectedSeverities = [.. SeverityList.Select(x => x.Value.ToLower())]; | ||
| } | ||
|
|
||
| // Assuming Tag is a string or list of strings that can be matched. | ||
| allRules = new ObservableList<AccessibilityRuleData>( | ||
| allRules.Where(r => | ||
| { | ||
| if (string.IsNullOrEmpty(r.Tags)) // Handle cases where Tags might be null or empty | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Split the comma-separated tags string into a collection of individual tags | ||
| // .Select(s => s.Trim()) to remove leading/trailing whitespace from each tag | ||
| // .ToList() to make it a List<string> if needed, or keep as IEnumerable<string> | ||
| IEnumerable<string> ruleIndividualTags = r.Tags.Split(',') | ||
| .Select(s => s.Trim()) | ||
| .Where(s => !string.IsNullOrWhiteSpace(s)); // Filter out empty strings from splitting | ||
|
|
||
| // Check if any of the individual tags for the rule are in the selectedStandardTags | ||
| return ruleIndividualTags.Any(ruleTag => selectedStandardTags.Contains(ruleTag)); | ||
| }) | ||
| ); | ||
| if (SeverityList != null && SeverityList.Any()) | ||
| // Apply filtering based on Analyzer type | ||
| if (analyzerType == nameof(ActAccessibilityTesting.eAnalyzer.ByStandard)) | ||
| { | ||
| if (selectedStandardTags == null || selectedStandardTags.Count == 0) | ||
| { | ||
| List<string> selectedSeverities = SeverityList.Select(x => x.Value.ToLower()).ToList(); | ||
|
|
||
| Status = eRunStatus.Failed; | ||
| Error = "Standard list is empty or not set when 'By Standard' analyzer is selected."; | ||
| return []; | ||
| } | ||
|
|
||
| // Rules to exclude based on severity: if a rule's impact is NOT in the selected severities | ||
| var severityExcludedRuleIds = allRules | ||
| .Where(r => !selectedSeverities.Contains(r.Impact.ToLower())) // Exclude if its Impact is NOT among the selected | ||
| .Select(r => r.RuleID); | ||
| // Filter rules to only include those matching selected standards/tags | ||
| rulesForAnalysis = rulesForAnalysis.WithTags([.. selectedStandardTags]); | ||
|
|
||
| foreach (var ruleId in severityExcludedRuleIds) | ||
| { | ||
| finalExcludedRuleIds.Add(ruleId); | ||
| } | ||
| if (selectedSeverities != null && selectedSeverities.Count != 0) | ||
| { | ||
| rulesForAnalysis = new ObservableCollection<AccessibilityRuleData>( | ||
| rulesForAnalysis.Where(rule => !selectedSeverities.Contains(rule.Impact.ToLower())) | ||
| ); | ||
| } | ||
| } | ||
| else if (GetInputParamValue(ActAccessibilityTesting.Fields.Analyzer) == nameof(eAnalyzer.BySeverity)) | ||
| else if (analyzerType == nameof(ActAccessibilityTesting.eAnalyzer.BySeverity)) | ||
| { | ||
| if (SeverityList == null || !SeverityList.Any()) | ||
| if (selectedSeverities == null || selectedSeverities.Count == 0) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, "Error: 'BySeverity' analyzer selected, but no severities are provided."); | ||
| // Optionally, set currentAct.Status = Failed and currentAct.Error | ||
| return new ObservableList<AccessibilityRuleData>(); | ||
| Status = eRunStatus.Failed; | ||
| Error = "Severity list is empty or not set when 'By Severity' analyzer is selected."; | ||
| return []; | ||
| } | ||
|
|
||
| List<string> selectedSeverities = SeverityList.Select(x => x.Value.ToLower()).ToList(); | ||
|
|
||
| // Filter rules: only include rules whose Impact matches a selected severity | ||
| allRules = new ObservableList<AccessibilityRuleData>( | ||
| allRules.Where(r => selectedSeverities.Contains(r.Impact.ToLower())) | ||
| // Filter rules to only include those matching selected severities | ||
| rulesForAnalysis = new ObservableCollection<AccessibilityRuleData>( | ||
| rulesForAnalysis.Where(rule => selectedSeverities.Contains(rule.Impact.ToLower())) | ||
| ); | ||
| } | ||
|
|
||
| // Final step: Apply the combined exclusions to the initially loaded rules | ||
| // Create the final list of rules that are truly active for the analyzer | ||
| ObservableList<AccessibilityRuleData> finalActiveRules = new ObservableList<AccessibilityRuleData>(); | ||
| foreach (AccessibilityRuleData ruleData in allRules) | ||
| } | ||
| else | ||
| { | ||
| if (!finalExcludedRuleIds.Contains(ruleData.RuleID)) | ||
| { | ||
| ruleData.Active = true; // Mark as active for consistency, though not strictly needed here | ||
| finalActiveRules.Add(ruleData); | ||
| } | ||
| else | ||
| { | ||
| ruleData.Active = false; // Explicitly mark as inactive if excluded | ||
| } | ||
|
|
||
| Status = eRunStatus.Passed; | ||
| Error = string.Empty; | ||
| } | ||
|
|
||
| return finalActiveRules; | ||
| return rulesForAnalysis; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-structured method with clear separation of concerns.
The refactored GetFilteredRuleList method provides excellent error handling and uses modern C# syntax. The logic correctly handles different analyzer types and validates input parameters.
However, the severity exclusion logic in the "By Standard" branch (lines 477-482) might be counterintuitive:
// In "By Standard" mode, why are we excluding selected severities?
rulesForAnalysis = new ObservableCollection<AccessibilityRuleData>(
rulesForAnalysis.Where(rule => !selectedSeverities.Contains(rule.Impact.ToLower()))
);Consider adding a comment to clarify the intent, or verify if this should be inclusion rather than exclusion of severities when using standard-based analysis.
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/ActionsLib/UI/Web/ActAccessibilityTesting.cs between
lines 436 and 507, the filtering logic in the "By Standard" analyzer branch
excludes rules matching the selected severities, which may be confusing. Review
whether this exclusion is intentional or if it should instead include only rules
with the selected severities. To fix, either add a clear comment explaining why
severities are excluded in this context or change the filtering condition to
include rules with severities in the selectedSeverities list, aligning it with
expected behavior.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes