Communication: Fix dropdown menu behavior for links to allow default browser options
#9832
Conversation
asliayk
added
tests
client
asliayk
changed the title
Communication: Fix dropdown menu behavior for links to allow default browser options
WalkthroughThe changes primarily involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)🔇 Additional comments (4)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (4)
The addition of the OnDestroy interface follows Angular best practices for component lifecycle management and memory leak prevention. Also applies to: 40-40
The implementation correctly determines if the element is a link by checking the cursor style, with proper error handling and fallback. This effectively solves the original issue by allowing default browser context menu for links.
Consider avoiding static property for dropdown state management Using -static activeDropdownPost: AnswerPostComponent | null = null;
+@Input() isActiveDropdown = false;
onRightClick(event: MouseEvent) {
// ... cursor style check ...
if (!isPointerCursor) {
event.preventDefault();
- if (AnswerPostComponent.activeDropdownPost !== this) {
- AnswerPostComponent.cleanupActiveDropdown();
- }
- AnswerPostComponent.activeDropdownPost = this;
+ this.closeOtherDropdowns.emit();
+ this.isActiveDropdown = true;
// ... rest of the method
}
}
+@Output() closeOtherDropdowns = new EventEmitter<void>();
Enhance component cleanup in ngOnDestroy While the dropdown cleanup is good, consider adding comprehensive cleanup:
+private subscriptions = new Subscription();
+private clickListener: () => void;
constructor(
public changeDetector: ChangeDetectorRef,
public renderer: Renderer2,
@Inject(DOCUMENT) private document: Document,
) {
super();
+ this.clickListener = this.renderer.listen('document', 'click', () => this.onClickOutside());
}
ngOnDestroy(): void {
if (AnswerPostComponent.activeDropdownPost === this) {
AnswerPostComponent.cleanupActiveDropdown();
}
+ if (this.clickListener) {
+ this.clickListener();
+ }
+ this.subscriptions.unsubscribe();
}Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (2)
103-104: Consider a more robust link detection approachWhile using cursor style to detect interactive elements works, consider combining it with additional checks for better reliability:
- const isPointerCursor = window.getComputedStyle(targetElement).cursor === 'pointer'; + const computedStyle = window.getComputedStyle(targetElement); + const isLink = targetElement.tagName.toLowerCase() === 'a' || + targetElement.closest('a') !== null; + const isPointerCursor = computedStyle.cursor === 'pointer'; + const isInteractive = isLink || isPointerCursor;
117-120: Consider adding boundary checks for dropdown positioningThe dropdown position calculation should include vertical boundary checks to ensure the dropdown remains within viewport:
this.dropdownPosition = { x: event.clientX, y: event.clientY, }; + + // Ensure dropdown stays within viewport vertically + const dropdownHeight = 200; // Approximate height + const screenHeight = window.innerHeight; + if (this.dropdownPosition.y + dropdownHeight > screenHeight) { + this.dropdownPosition.y = screenHeight - dropdownHeight - 10; + }src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (3)
180-193: Consider adding more cursor style test cases.The test cases cover 'pointer' and 'default' cursors, but should also include other common cursor styles like 'text' to ensure comprehensive coverage of the right-click behavior.
const testCases = [ { cursor: 'pointer', preventDefaultCalled: false, showDropdown: false, dropdownPosition: { x: 0, y: 0 }, }, { cursor: 'default', preventDefaultCalled: true, showDropdown: true, dropdownPosition: { x: 100, y: 200 }, }, + { + cursor: 'text', + preventDefaultCalled: true, + showDropdown: true, + dropdownPosition: { x: 100, y: 200 }, + }, ];
201-203: Use more specific type for mock return value.The current mock implementation uses a partial type. Consider using a more complete mock object to prevent potential type-related issues.
-jest.spyOn(window, 'getComputedStyle').mockReturnValue({ - cursor, -} as CSSStyleDeclaration); +const mockStyle = { + cursor, + getPropertyValue: (prop: string) => prop === 'cursor' ? cursor : '', + setProperty: jest.fn(), +} as unknown as CSSStyleDeclaration; +jest.spyOn(window, 'getComputedStyle').mockReturnValue(mockStyle);
195-208: Consider extracting test setup to a helper function.The test setup code is quite lengthy. Consider extracting it to a helper function to improve readability and maintainability.
+const setupRightClickTest = (cursor: string) => { + const event = new MouseEvent('contextmenu', { clientX: 100, clientY: 200 }); + const targetElement = document.createElement('div'); + Object.defineProperty(event, 'target', { value: targetElement }); + + const mockStyle = { + cursor, + getPropertyValue: (prop: string) => prop === 'cursor' ? cursor : '', + setProperty: jest.fn(), + } as unknown as CSSStyleDeclaration; + jest.spyOn(window, 'getComputedStyle').mockReturnValue(mockStyle); + + const preventDefaultSpy = jest.spyOn(event, 'preventDefault'); + + return { event, preventDefaultSpy }; +}; testCases.forEach(({ cursor, preventDefaultCalled, showDropdown, dropdownPosition }) => { - const event = new MouseEvent('contextmenu', { clientX: 100, clientY: 200 }); - const targetElement = document.createElement('div'); - Object.defineProperty(event, 'target', { value: targetElement }); - - jest.spyOn(window, 'getComputedStyle').mockReturnValue({ - cursor, - } as CSSStyleDeclaration); - - const preventDefaultSpy = jest.spyOn(event, 'preventDefault'); + const { event, preventDefaultSpy } = setupRightClickTest(cursor); component.onRightClick(event);src/main/webapp/app/shared/metis/post/post.component.ts (2)
124-125: Consider using a more robust link detection methodWhile checking the cursor style works, it might be more reliable to directly check if the clicked element is a link or contains a link in its parent chain.
-const isPointerCursor = window.getComputedStyle(targetElement).cursor === 'pointer'; +const isLink = (element: HTMLElement): boolean => { + if (!element) return false; + return element.tagName === 'A' || (element.parentElement ? isLink(element.parentElement) : false); +}; +const shouldDisableDropdown = isLink(targetElement);
124-146: Consider enhancing keyboard accessibilityWhile the right-click handling is improved, consider adding keyboard navigation support for the custom dropdown menu when it is shown.
// Add keyboard event handling @HostListener('keydown', ['$event']) onKeyDown(event: KeyboardEvent) { if (this.showDropdown) { switch(event.key) { case 'Escape': this.showDropdown = false; this.enableBodyScroll(); break; // Add more keyboard navigation as needed } } }src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (2)
315-351: Test implementation looks good but could be more comprehensive.The test case effectively verifies the
onRightClickbehavior for different cursor styles. However, consider these improvements:
- Add specific test cases for link elements (
<a>tags) since they are the main focus of the PR- Include additional cursor values that might appear on links (e.g., 'text', 'help')
- Consider testing the interaction with actual link text and href attributes
Here's a suggested enhancement:
it('should handle onRightClick correctly based on cursor style', () => { const testCases = [ { cursor: 'pointer', + element: 'a', + href: 'https://example.com', preventDefaultCalled: false, showDropdown: false, dropdownPosition: { x: 0, y: 0 }, }, { cursor: 'default', + element: 'div', preventDefaultCalled: true, showDropdown: true, dropdownPosition: { x: 100, y: 200 }, }, + { + cursor: 'text', + element: 'a', + href: 'https://example.com', + preventDefaultCalled: false, + showDropdown: false, + dropdownPosition: { x: 0, y: 0 }, + }, ]; - testCases.forEach(({ cursor, preventDefaultCalled, showDropdown, dropdownPosition }) => { + testCases.forEach(({ cursor, element, href, preventDefaultCalled, showDropdown, dropdownPosition }) => { const event = new MouseEvent('contextmenu', { clientX: 100, clientY: 200 }); - const targetElement = document.createElement('div'); + const targetElement = document.createElement(element); + if (href) { + (targetElement as HTMLAnchorElement).href = href; + } Object.defineProperty(event, 'target', { value: targetElement });
349-349: Consider moving mock restoration to afterEach.Instead of calling
jest.restoreAllMocks()within the test, move it to theafterEachblock where other cleanup is performed. This ensures consistent cleanup across all tests and follows Jest best practices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(1 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(1 hunks)src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (1)
src/main/webapp/app/shared/metis/post/post.component.ts (1)
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (2)
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (1)
209-211: 🛠️ Refactor suggestion
Use more specific Jest matchers.
According to the coding guidelines, use more specific Jest matchers for boolean and object comparisons.
-expect(component.showDropdown).toBe(showDropdown);
-expect(component.dropdownPosition).toEqual(dropdownPosition);
+expect(component.showDropdown).toBeTrue();
+expect(component.dropdownPosition).toContainEntries(Object.entries(dropdownPosition));Likely invalid or redundant comment.
src/main/webapp/app/shared/metis/post/post.component.ts (1)
127-146: LGTM! The implementation handles dropdown state correctly
The code properly:
- Prevents default behavior only when necessary
- Manages dropdown state and positioning
- Handles cleanup of previous dropdowns
- Updates change detection appropriately
This aligns well with Angular's OnPush change detection strategy and maintains proper component state management.
Please verify that:
- The dropdown is disabled for all types of links (text links, button links, etc.)
- The browser's context menu appears correctly for links
- The custom dropdown still works for non-link elements
src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts
Outdated
Show resolved
Hide resolved
asliayk
changed the title
Communication: Fix dropdown menu behavior for links to allow default browser optionsCommunication: Fix dropdown menu behavior for links to allow default browser options
HawKhiem
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
sachmii
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
dismissed stale reviews from PaRangger and coderabbitai[bot]
via
e32c071
PaRangger
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
FelberMartin
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
bugfix/communication/disable-dropdown-when-there-is-link
branch
Checklist
General
Client
Motivation and Context
When a user right-clicks on a link within a message, the dropdown menu opens instead of displaying the browser's default context menu (e.g., option to open the link in a new tab).
(Closes #9776)
Description
The dropdown menu is now disabled when the user right-clicks on a link, allowing the browser's default options to be displayed.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
onRightClickmethod in both the AnswerPost and Post components to validate behavior under different cursor styles.