Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/angular/src/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -826,11 +826,12 @@ export declare interface IxDrawer extends Components.IxDrawer {
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: ['anchor', 'closeBehavior', 'header', 'placement', 'positioningStrategy', 'show', 'suppressAutomaticPlacement', 'trigger'],
outputs: ['showChanged'],
outputs: ['showChange', 'showChanged'],
standalone: false
})
export class IxDropdown {
protected el: HTMLIxDropdownElement;
@Output() showChange = new EventEmitter<CustomEvent<boolean>>();
@Output() showChanged = new EventEmitter<CustomEvent<boolean>>();
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone) {
c.detach();
Expand All @@ -841,7 +842,11 @@ export class IxDropdown {

export declare interface IxDropdown extends Components.IxDropdown {
/**
* Fire event after visibility of dropdown has changed
* Fire event before visibility of dropdown has changed, preventing event will cancel showing dropdown
*/
showChange: EventEmitter<CustomEvent<boolean>>;
/**
* Fire event after visibility of dropdown has changed, preventing event will prevent nothing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The phrase "preventing event will prevent nothing" is a bit confusing. It could be rephrased for better clarity, for example: "This event is not preventable." or "Calling preventDefault() on this event has no effect." This comment also applies to other generated files where this documentation appears.

Suggested change
* Fire event after visibility of dropdown has changed, preventing event will prevent nothing
* Fire event after visibility of dropdown has changed. This event is not preventable.

*/
showChanged: EventEmitter<CustomEvent<boolean>>;
}
Expand Down
9 changes: 7 additions & 2 deletions packages/angular/standalone/src/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -927,10 +927,11 @@ export declare interface IxDrawer extends Components.IxDrawer {
template: '<ng-content></ng-content>',
// eslint-disable-next-line @angular-eslint/no-inputs-metadata-property
inputs: ['anchor', 'closeBehavior', 'header', 'placement', 'positioningStrategy', 'show', 'suppressAutomaticPlacement', 'trigger'],
outputs: ['showChanged'],
outputs: ['showChange', 'showChanged'],
})
export class IxDropdown {
protected el: HTMLIxDropdownElement;
@Output() showChange = new EventEmitter<CustomEvent<boolean>>();
@Output() showChanged = new EventEmitter<CustomEvent<boolean>>();
constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone) {
c.detach();
Expand All @@ -941,7 +942,11 @@ export class IxDropdown {

export declare interface IxDropdown extends Components.IxDropdown {
/**
* Fire event after visibility of dropdown has changed
* Fire event before visibility of dropdown has changed, preventing event will cancel showing dropdown
*/
showChange: EventEmitter<CustomEvent<boolean>>;
/**
* Fire event after visibility of dropdown has changed, preventing event will prevent nothing
*/
showChanged: EventEmitter<CustomEvent<boolean>>;
}
Expand Down
12 changes: 12 additions & 0 deletions packages/core/scss/mixins/shadow-dom/_focus-visible.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* SPDX-FileCopyrightText: 2025 Siemens AG
*
* SPDX-License-Identifier: MIT
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
@mixin ix-focus-visible() {
outline: 1px solid var(--theme-color-focus-bdr) !important;
outline-offset: var(--theme-btn--focus--outline-offset) !important;
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of !important can make styles difficult to override and debug. Is it possible to achieve the desired focus styling by increasing specificity or reordering CSS rules to avoid using !important?

}
7 changes: 6 additions & 1 deletion packages/core/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4720,6 +4720,7 @@ declare global {
new (): HTMLIxDrawerElement;
};
interface HTMLIxDropdownElementEventMap {
"showChange": boolean;
"showChanged": boolean;
}
interface HTMLIxDropdownElement extends Components.IxDropdown, HTMLStencilElement {
Expand Down Expand Up @@ -7168,7 +7169,11 @@ declare namespace LocalJSX {
alignmentAxis?: number;
};
/**
* Fire event after visibility of dropdown has changed
* Fire event before visibility of dropdown has changed, preventing event will cancel showing dropdown
*/
"onShowChange"?: (event: IxDropdownCustomEvent<boolean>) => void;
/**
* Fire event after visibility of dropdown has changed, preventing event will prevent nothing
*/
"onShowChanged"?: (event: IxDropdownCustomEvent<boolean>) => void;
"overwriteDropdownStyle"?: (delegate: {
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/components/avatar/avatar.scss
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
@use 'mixins/text-truncation';
@use 'mixins/hover';
@use 'mixins/scrollbar';
@use 'mixins/shadow-dom/focus-visible' as focus-visible;

:host {
display: flex;
Expand Down Expand Up @@ -119,3 +120,9 @@
transform: scale(0.8);
}
}

:host(.avatar-button.ix-focused) {
button {
@include focus-visible.ix-focus-visible();
}
}
5 changes: 4 additions & 1 deletion packages/core/src/components/avatar/avatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,10 @@ export class Avatar {

if (this.isClosestApplicationHeader) {
return (
<Host slot="ix-application-header-avatar" class={'avatar-button'}>
<Host
slot="ix-application-header-avatar"
class={'avatar-button ix-focusable'}
>
<BaseButton
disabled={false}
iconOval={false}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/components/button/base-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export const BaseButton: FunctionalComponent<BaseButtonProps> = (

const commonAttributes = {
...ariaAttributes,
tabindex: props.disabled ? -1 : (props.tabIndex ?? 0),
// tabindex: props.disabled ? -1 : (props.tabIndex ?? 0),
class: {
...getButtonClasses(
props.variant,
Expand Down
27 changes: 20 additions & 7 deletions packages/core/src/components/button/button-mixin.scss
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@
border-top-right-radius: var(--ix-button-border-radius-right);
border-bottom-right-radius: var(--ix-button-border-radius-right);

&,
&.focus,
&:focus-visible {
& {
background-color: var(--theme-btn-#{$name}--background);
color: var(--theme-btn-#{$name}--color);

Expand All @@ -32,10 +30,10 @@
border-style: solid;
}

@include hover.focus-visible {
outline: 1px solid var(--theme-color-focus-bdr);
outline-offset: var(--theme-btn--focus--outline-offset);
}
// @include hover.focus-visible {
// outline: 1px solid var(--theme-color-focus-bdr);
// outline-offset: var(--theme-btn--focus--outline-offset);
// }

&.selected {
background-color: var(--theme-btn-#{$name}--background--pressed);
Expand Down Expand Up @@ -160,6 +158,21 @@
:host(.disabled) {
cursor: default;
}

:host(.ix-focused) {
.btn {
outline: 1px solid var(--theme-color-focus-bdr) !important;
outline-offset: var(--theme-btn--focus--outline-offset) !important;
Comment on lines +164 to +165
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of !important here can make these styles hard to override. Could this be avoided by using higher specificity selectors or a different approach?

}
}

:host(:focus-visible) {
outline: none;

.btn {
outline: none;
}
}
}

@mixin all-btn-variants {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/components/button/button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ export class Button implements IxButtonComponent {
onClick: () => this.dispatchFormEvents(),
type: this.type,
alignment: this.alignment,
tabIndex: 0,
ariaAttributes: {
'aria-label': this.ariaLabelButton,
},
Expand All @@ -203,6 +202,7 @@ export class Button implements IxButtonComponent {
<Host
class={{
disabled: this.disabled || this.loading,
'ix-focusable': true,
}}
>
<BaseButton {...baseButtonProps}>
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/components/dropdown-item/dropdown-item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,10 @@ export class DropdownItem implements DropdownItemWrapper {
'icon-only': this.isIconOnly(),
disabled: this.disabled,
submenu: this.isSubMenu,
'ix-focusable': true,
}}
role="listitem"
tabIndex={-1}
>
<button
type="button"
Expand Down
10 changes: 10 additions & 0 deletions packages/core/src/components/dropdown/dropdown-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,16 @@ class DropdownController {
return;
}

getParentDropdownId(dropdownId: string) {
for (const ruleKey of Object.keys(this.submenuIds)) {
if (this.submenuIds[ruleKey].includes(dropdownId)) {
return ruleKey;
}
}

return;
}

private pathIncludesDropdown(eventTargets: EventTarget[]) {
return !!eventTargets.find(
(element: EventTarget) =>
Expand Down
159 changes: 159 additions & 0 deletions packages/core/src/components/dropdown/dropdown-focus.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/*
* SPDX-FileCopyrightText: 2025 Siemens AG
*
* SPDX-License-Identifier: MIT
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import { requestAnimationFrameNoNgZone } from '../utils/requestAnimationFrame';

export const getIndexOfDropdownItem = (
items: HTMLElement[],
item: HTMLElement | null
) => {
if (!item || item.tagName !== 'IX-DROPDOWN-ITEM') {
return -1;
}

return items.findIndex((el) => el === item);
};

export const getNextFocusableDropdownItem = (
items: HTMLElement[],
currentItem: HTMLElement | null
) => {
const currentItemIndex = getIndexOfDropdownItem(items, currentItem);
return items[currentItemIndex + 1];
};

export const getPreviousFocusableItem = (
items: HTMLElement[],
currentItem: HTMLElement | null
) => {
const currentItemIndex = getIndexOfDropdownItem(items, currentItem);
return items[currentItemIndex - 1];
};

const focusItem = (item: HTMLElement) => {
requestAnimationFrameNoNgZone(() =>
item.shadowRoot!.querySelector('button')!.focus()
);
};

export const isTriggerElement = (element: HTMLElement) =>
element.hasAttribute('data-ix-dropdown-trigger');

export const configureKeyboardInteraction = (dropdownElement: HTMLElement) => {
const callback = async (ev: KeyboardEvent) => {
const activeElement = document.activeElement as HTMLElement | null;
let items: HTMLElement[] = [];

const targetTagName = (ev.target as HTMLElement)?.tagName;
if (
targetTagName !== 'IX-DROPDOWN' &&
targetTagName !== 'IX-DROPDOWN-ITEM'
) {
return;
}

try {
const query =
'ix-dropdown-item:not(ix-dropdown ix-dropdown *):not([disabled])';

// Collect items from slots if they exist
if (dropdownElement.querySelectorAll('slot').length > 0) {
const slotElements = Array.from(
dropdownElement.querySelectorAll('slot')
);
items = slotElements.flatMap((slot) =>
Array.from(
slot.assignedElements({ flatten: true }) as HTMLElement[]
).flatMap((el) => {
// Check if the assigned element itself matches the query
if (el.matches && el.matches(query)) {
return [el];
}
// Otherwise, query its children
return Array.from(
el.querySelectorAll(query) as NodeListOf<HTMLElement>
);
})
);
} else {
// No slots, query directly on popoverEl
items = Array.from(
dropdownElement.querySelectorAll(query) as NodeListOf<HTMLElement>
);
}

/* eslint-disable-next-line */
} catch {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The catch block is empty. This will swallow any errors that occur within the try block, making debugging difficult. It's better to at least log the error to the console, especially during development.

Suggested change
} catch {}
} catch (e) { console.error(e); }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist I do not agree, i do not want that the user see any error message in the console

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern about not wanting end-users to see error messages in the console. However, an empty catch block can make debugging very challenging, as it completely hides any issues that might occur during the DOM querying process.

To address this, I recommend logging errors conditionally. This approach ensures that errors are visible to developers during testing and development, which is crucial for identifying and fixing issues, without exposing potentially sensitive or confusing error messages to end-users in a production environment.

This can be achieved by checking the NODE_ENV environment variable:

/* eslint-disable-next-line */
} catch (e) {
  if (process.env.NODE_ENV === 'development') {
    console.error('Error during dropdown item collection:', e);
  }
  // Optionally, you could send errors to an internal logging service in production here.
}

Silently swallowing errors can lead to unexpected behavior in the UI that is very difficult to trace back to its origin. Even if not visible to the end-user, having a record of these errors is crucial for maintaining the stability and reliability of the component.

Suggested change
} catch {}
/* eslint-disable-next-line */
} catch (e) {
if (process.env.NODE_ENV === 'development') {
console.error('Error during dropdown item collection:', e);
}
}


switch (ev.key) {
case 'ArrowLeft':
dropdownElement.dispatchEvent(
new CustomEvent('ix-close-submenu', {
bubbles: true,
cancelable: true,
})
);
break;

case 'ArrowDown':
// Disable movement/scroll with keyboard
ev.preventDefault();
const nextItem = getNextFocusableDropdownItem(items, activeElement);
if (nextItem !== undefined) {
focusItem(nextItem);
}
break;

case 'ArrowUp':
// Disable movement/scroll with keyboard
ev.preventDefault();
const prevItem = getPreviousFocusableItem(items, activeElement);
if (prevItem !== undefined) {
focusItem(prevItem);
}
break;

case 'Home':
ev.preventDefault();
const firstItem = items[0];
if (firstItem !== undefined) {
focusItem(firstItem);
}
break;

case 'End':
ev.preventDefault();
const lastItem = items[items.length - 1];
if (lastItem !== undefined) {
focusItem(lastItem);
}
break;

case 'ArrowRight':
case ' ':
case 'Enter':
if (activeElement && isTriggerElement(activeElement)) {
const triggerEvent = new CustomEvent('ix-open-submenu', {
bubbles: true,
cancelable: true,
detail: {
activeElement: activeElement,
},
});
activeElement.dispatchEvent(triggerEvent);
}
break;
default:
break;
}
};

dropdownElement.addEventListener('keydown', callback);
return () => dropdownElement.removeEventListener('keydown', callback);
};
Loading
Loading