-
Notifications
You must be signed in to change notification settings - Fork 115
Internal view obstructions #1228
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
base: master
Are you sure you want to change the base?
Conversation
YuriyVelichkoPI
left a comment
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.
Here is the PR review made by Claude code. As for me most of items make sense and should be addressed.
I have doubts about the section Incomplete System UI Coverage. Please review it carefully and add other system views that should be choked. But only if they actually can be transparent and overlap the ad view, no need to follow the claude's recommendation mindlessly.
In this scope of of this change we shouldn't consider custom transparent views.
Code Review: Feature Branch feature/1227-internal-view-obstructions
Overview
This branch addresses issue #1227 where iOS system UI components (navigation bars and tab bars) were incorrectly flagged as obstructions during ad viewability checks, causing false negative reports even when ads were fully visible.
Summary of Changes
- Files Modified: 1 primary file (
PBMViewExposureChecker.m) - Lines Added: 66
- Lines Removed: 2
- Commits: 1 main commit + 3 inherited from master
Positive Aspects ✓
1. Clear Problem Identification
The fix directly addresses the root cause: system UI overlays (particularly iOS 26's "glass" UI design) were being treated as obstructions when they shouldn't be.
2. Comprehensive Detection Logic
The implementation checks for system components in multiple ways:
- Direct class type checking (
isKindOfClass:) - Ancestor traversal (
isViewDescendantOfClass:) - Responder chain traversal (
nextResponder)
This multi-pronged approach increases the likelihood of catching system views regardless of their position in the hierarchy.
3. Clean Code Structure
The refactoring improves code organization:
- Extracted filtering logic into dedicated methods
- Single Responsibility Principle: each method has one clear purpose
- The previous
view.isHiddencheck was properly incorporated into the newshouldIgnoreView:method
4. Minimal Invasiveness
The changes don't alter the core exposure calculation algorithm, just the filtering of what should be considered an obstruction.
Issues and Concerns ⚠️
1. CRITICAL: Missing Test Coverage
The existing ViewExposureTest.swift was NOT modified, meaning there are no automated tests for this new functionality.
Risk: Future changes could break this fix without detection.
Recommendation: Add tests that:
func testIgnoresNavigationBar() {
// Setup with UINavigationController
// Verify ad view is not obstructed by navigation bar
}
func testIgnoresTabBar() {
// Setup with UITabBarController
// Verify ad view is not obstructed by tab bar
}2. Potential Performance Impact
The isViewPartOfTabBar: and isViewPartOfNavigationBar: methods walk the responder chain on every view checked during exposure calculation.
Concerns:
- Responder chain traversal with
while (responder != nil)could be expensive in deep hierarchies - These methods are called twice per view (in
collectObstructionsFrom:andtestForObstructing:) - The second call in
testForObstructing:is redundant sincecollectObstructionsFrom:already callsshouldIgnoreView:
Code Issue (PBMViewExposureChecker.m:214-217):
- (void)testForObstructing:(UIView *)view {
if ([self shouldIgnoreView:view]) { // <-- Redundant check
return;
}This check is unnecessary because testForObstructing: is only called from collectObstructionsFrom:, which has already verified the view shouldn't be ignored.
Recommendation: Remove the redundant check in testForObstructing: to improve performance.
3. Incomplete System UI Coverage
The implementation only handles UINavigationBar and UITabBar, but iOS has other system overlays:
- Status bar (may not be an issue if it's outside the view bounds)
- Toolbar (
UIToolbar) - Search bar (
UISearchBar) - iOS 16+ FloatingBarContainerView (mentioned in the issue but not explicitly handled)
- Keyboard (if visible over an ad)
Question: Are these intentionally excluded, or were they overlooked?
4. Logic Duplication
The methods isViewPartOfTabBar: and isViewPartOfNavigationBar: have nearly identical implementations. This violates DRY (Don't Repeat Yourself).
Suggestion: Refactor to a generic method:
- (BOOL)isViewPartOfSystemUIType:(NSArray<Class> *)classes view:(UIView *)view {
// Check if view is descendant of any class
for (Class cls in classes) {
if ([self isViewDescendantOfClass:cls view:view]) {
return YES;
}
}
// Check responder chain
UIResponder *responder = view.nextResponder;
while (responder != nil) {
for (Class cls in classes) {
if ([responder isKindOfClass:cls]) {
return YES;
}
}
responder = responder.nextResponder;
}
return NO;
}Then simplify shouldIgnoreView::
- (BOOL)shouldIgnoreView:(UIView *)view {
if (view.isHidden) {
return YES;
}
NSArray<Class> *systemUIClasses = @[
[UITabBar class],
[UITabBarController class],
[UINavigationBar class],
[UINavigationController class]
];
return [self isViewPartOfSystemUIType:systemUIClasses view:view];
}5. Missing Documentation
None of the new methods have header comments explaining:
- Their purpose
- Why this filtering is necessary
- What types of views are filtered
Recommendation: Add method documentation, especially since this is a non-obvious business rule.
6. Edge Case: Custom Transparent Overlays
The current approach only handles Apple's system UI. Apps might have their own transparent overlay views (e.g., custom navigation bars, floating buttons) that shouldn't be treated as obstructions.
Question: Should there be a way for SDK users to register custom view classes to ignore? Or is this scope creep?
7. iOS Version Compatibility
The issue mentions this is specifically a problem on "iOS 26" (likely meant iOS 16+). The code doesn't include version checks, which could mean:
- ✓ The fix works across all iOS versions (good)
⚠️ It might have unintended side effects on older iOS versions (needs testing)
Testing Recommendations
Manual Testing Needed:
- ✓ Test with
UINavigationControllerwith translucent navigation bar - ✓ Test with
UITabBarControllerwith translucent tab bar - ✓ Test with both navigation + tab bar (e.g., nested controllers)
- Test on iOS 13, 14, 15, 16, 17+ to ensure no regressions
- Test with opaque (non-translucent) bars - should still be ignored?
- Test with custom UINavigationBar/UITabBar subclasses
- Performance test with deeply nested view hierarchies
Automated Testing Needed:
- Unit tests for
shouldIgnoreView:,isViewPartOfTabBar:,isViewPartOfNavigationBar: - Integration tests simulating the reported issue
- Regression tests ensuring existing exposure calculations still work
Security & Privacy Considerations
✓ No security concerns identified
✓ No privacy data is accessed
✓ Changes are purely computational (geometry calculations)
Final Recommendation
Decision: Approve with Required Changes
The fix addresses the reported issue effectively, but needs improvements before merging:
Must Fix (Blocking):
- ✅ Add automated tests for the new functionality
- ✅ Remove redundant
shouldIgnoreView:check intestForObstructing:
Should Fix (Recommended):
- Consider refactoring to reduce code duplication
- Add method documentation
- Consider handling
UIToolbaras well - Performance test with complex hierarchies
Nice to Have:
- Document iOS version compatibility explicitly
- Consider extensibility for custom overlay types
Overall Assessment: The implementation is functionally sound and addresses the issue, but lacks test coverage and has minor efficiency concerns. With the required changes, this would be production-ready.
#1227
On recent iOS versions with the new
glassUI, system UI components like tab bar, navigation bar are rendered as overlay views that sit above the application content. During viewability polling inPBMViewExposureChecker, these system views were detected as obstructions.The exposure checker now filters internal UI components out.
Alongside the MRAID examples in the demo app were fixed.