-
Notifications
You must be signed in to change notification settings - Fork 26
Fix button issues #774
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: main
Are you sure you want to change the base?
Fix button issues #774
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,9 @@ | |
| *ngIf="isBusy" | ||
| [ngStyle]="getRippleContainerStyle()" | ||
| class="nui-button-ripple-container" | ||
| aria-live="polite" | ||
| aria-atomic="true" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aria-atomic true I wouldn't use that because it force screen readers to read it contents |
||
| aria-label="Loading" | ||
| > | ||
| <div class="ripple ripple-1"></div> | ||
| <div class="ripple ripple-2"></div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,12 @@ export class ButtonComponent implements OnInit, OnDestroy, AfterContentChecked { | |
| /** Sets aria-label for the component */ | ||
| @Input() public ariaLabel: string = ""; | ||
|
|
||
| /** Sets aria-disabled for disabled state programmatic indication */ | ||
| @HostBinding("attr.aria-disabled") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be on the control of the clients but not handle form the nova side this is not correct because screen readers already rely on the native https://ripple.watermarkinsights.com/blog/a-few-remarks-on-using-aria/ only valid case is next: but as you can see it is under control of client code not nova. |
||
| public get ariaDisabled(): string | null { | ||
| return this.getHostElement().disabled ? "true" : null; | ||
| } | ||
|
|
||
| /** | ||
| * Optionally, set whether to fire a "click" event repeatedly while the button is pressed. | ||
| */ | ||
|
|
@@ -188,6 +194,9 @@ should be set explicitly: `, | |
| el.nativeElement | ||
| ); | ||
| } | ||
|
|
||
| // Validate accessibility for icon-only buttons | ||
| this.validateIconOnlyButtonAccessibility(); | ||
| } | ||
|
|
||
| public ngOnInit(): void { | ||
|
|
@@ -251,6 +260,12 @@ should be set explicitly: `, | |
| const mouseLeave$ = fromEvent(hostElement, "mouseleave").pipe( | ||
| takeUntil(this.ngUnsubscribe) | ||
| ); | ||
| const keyUp$ = fromEvent(hostElement, "keyup").pipe( | ||
| takeUntil(this.ngUnsubscribe), | ||
| filter((event: KeyboardEvent) => event.key === " " || event.key === "Enter") | ||
| ); | ||
|
|
||
| // Handle mouse-based repeat events | ||
| fromEvent(hostElement, "mousedown") | ||
| .pipe( | ||
| takeUntil(this.ngUnsubscribe), | ||
|
|
@@ -274,9 +289,49 @@ should be set explicitly: `, | |
| } | ||
| }); | ||
| }); | ||
|
|
||
| // Handle keyboard-based repeat events for accessibility | ||
| fromEvent(hostElement, "keydown") | ||
| .pipe( | ||
| takeUntil(this.ngUnsubscribe), | ||
| filter((event: KeyboardEvent) => { | ||
| return this.isRepeat && (event.key === " " || event.key === "Enter"); | ||
| }) | ||
| ) | ||
| .subscribe((event: KeyboardEvent) => { | ||
| event.preventDefault(); // Prevent default space/enter behavior | ||
| const repeatSubscription = timer( | ||
| buttonConstants.repeatDelay, | ||
| buttonConstants.repeatInterval | ||
| ) | ||
| .pipe( | ||
| takeUntil( | ||
| merge(keyUp$, this.ngUnsubscribe) | ||
| ) | ||
| ) | ||
| .subscribe(() => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. subscribe in subscribe wow that is awesome code ! great ... kiding this may lead to memory leaks and the code overcomplicated which makes it un readable |
||
| if (hostElement.disabled) { | ||
| repeatSubscription.unsubscribe(); | ||
| } else { | ||
| hostElement.click(); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private getHostElement() { | ||
| return this.el.nativeElement; | ||
| } | ||
|
|
||
| private validateIconOnlyButtonAccessibility(): void { | ||
| // Check if button will be icon-only and validate accessibility | ||
| setTimeout(() => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why set timeout? |
||
| if (this._isContentEmpty && this.icon && !this.ariaLabel) { | ||
| this.logger.warn( | ||
| "Icon-only button detected without aria-label. Consider providing a meaningful aria-label for screen readers: ", | ||
| this.el.nativeElement | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
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.
aria-busy="true"
aria-live="polite"
Don't use aria-label or aria-labelledby on a span or div unless its given a role.
aria-label=Loading is not ok in this case since this element do not have content but only visual indicator of busy