Skip to content
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

fix(core): toolbar, fix accessibility issues #7373

Closed
wants to merge 2 commits into from
Closed

fix(core): toolbar, fix accessibility issues #7373

wants to merge 2 commits into from

Conversation

Lukakatche
Copy link
Contributor

@Lukakatche Lukakatche commented Dec 15, 2021

Related Issue(s)

closes #5806

Description

Issue 1: added placeholder for date picker
Issue 2: retested with JAWS and it reads selected month
Issue 3: retested with JAWS and each day is read fully
Issue 4: retested with JAWS and time fields are accessible.
Issue 5: retested with JAWS and you can go in between items with arrow keys
Issue 6: added aria-expanded attribute to the button based on the isOpenChange output.
Issue 7: all of the arrows have a title with the corresponding icon description
Issue 8: dual focus is gone
Issue 9: Issue gone
Issue 10: retested and focus works fine
Issue 11: added an input to the spacer for max width
Issue 12: retested and it is already fixed

  1. Visual Testing:
  • [ n/a] visual misalignments/updates
  • [ n/a] check Light/Dark/HCB/HCW themes
  • [ n/a] RTL/LTR - proper rendering and labeling
  • [ n/a] responsiveness(resize)
  • [ n/a] Content Density (Cozy/Compact/(Condensed))
  • [ n/a] States - hover/disabled/focused/active/on click/selected/selected hover/press state
  • [ n/a] Interaction/Animation - open/close, expand/collapse, add/remove, check/uncheck
  • [ n/a] Mouse vs. Keyboard support
  • [ n/a] Text Truncation
  1. API and functional correctness
  • [ n/a] check for console logs (warnings, errors)
  • [ n/a] API boundary values
  • [ n/a] different combinations of components - free style
  • [ n/a] change the API values during testing
  1. Documentation and Example validations
  • [ n/a] missing API documentation or it is not understandable
  • [ n/a] poor examples
  • [ n/a] Stackblitz works for all examples
  1. Accessibility testing
  2. Browser Testing - Edge, Safari, Chrome, Firefox
PR Quality

@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for fundamental-ngx ready!

🔨 Explore the source changes: d64e32b

🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-ngx/deploys/61b9a41970c3ee0008274aee

😎 Browse the preview: https://deploy-preview-7373--fundamental-ngx.netlify.app

Comment on lines +18 to +20
@HostBinding('style.maxWidth')
@Input()
maxWidth: string;
Copy link
Contributor

@dimamarksman dimamarksman Dec 16, 2021

Choose a reason for hiding this comment

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

Do we really need this input?
I don't see the value of such inputs when user can apply the max-width or any other css property directly to the host element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dimamarksman if we have an input for width, I don't see a problem with having an input for max-width

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also do not understand purpose of this.

If user wants fixed spacer - he can set its width in px, if he wants responsive one - should use %.

Instead of max-width i would set flex-shrink: 1 for spacer to handle width: 9999px or any other wrong values.

@Betrozov Betrozov changed the title Fix accessibility issues 5806 fix(core): toolbar, fix accessibility issues Dec 16, 2021
@Betrozov Betrozov added the core Core library specific issues label Dec 16, 2021
@Betrozov Betrozov added this to the Sprint 77 - Ariba milestone Dec 16, 2021
@github-actions
Copy link

This pull request is stale because it has been open 2 days with no activity. Remove stale label or comment or this will be closed in 3 days

@Veerakarthick
Copy link

Hello Colleague,
Retest has been taken below are the details.

issue1: fixed
issue2:fixed
issue3:fixed
issue4:fixed
Issue5: Observed Behavior: Within ‘toolbar overflow’ header, on activating ‘more’ menu, the items available within the menu cannot be accessed using keyboard. Therefore, the list is not accessible using keyboard. Expected Behavior: Ideally, all the UI elements should be accessible using keyboard. i.e. the user should be able to navigate within the ‘more’ menu list using keyboard. screenshot:

issue6: Observed Behavior: Within ‘toolbar overflow’ header, on activating ‘more’ menu, the state ‘expanded’ is not read. Therefore, the information that the menu has been expanded is not read by the screen reader. The speech output is: More button menu Press space to activate the menu, then navigate with arrow keys Expected Behavior: Ideally, on activating ‘more’ menu, the information ‘expanded’ should be read. i.e. the state regarding the UI element on activating the menu should be read by the screen reader. screenshot:

issue7: Fixed
Issue8: Fixed
issue9: Fixed
Issue10:
Observed Behavior: Focus is not clear and not visible in the ‘Hours’, ‘Minutes’ & ‘Period’ input fields present within calendar field under ‘Toolbar Overflow’ section. Expected Behavior: Ideally, a strong four-sided visible focus should be available on all the UI elements present within screen. screenshot:

issue11:Fixed.
issue12:Fixed.

Regards,
Karthikeyan.c

@Karthickc1994
Copy link

Hello Colleague,
Retest has been taken below are the details.
issue1: fixed
issue2:fixed
issue3:fixed
issue4:fixed
Issue5: Observed Behavior: Within ‘toolbar overflow’ header, on activating ‘more’ menu, the items available within the menu cannot be accessed using keyboard. Therefore, the list is not accessible using keyboard. Expected Behavior: Ideally, all the UI elements should be accessible using keyboard. i.e. the user should be able to navigate within the ‘more’ menu list using keyboard.
screenshot:

image
image

issue6: Fixed
issue7: Fixed
Issue8: Fixed
issue9: Fixed
Issue10:
Observed Behavior: Focus is not clear and not visible in the ‘Hours’, ‘Minutes’ & ‘Period’ input fields present within calendar field under ‘Toolbar Overflow’ section. Expected Behavior: Ideally, a strong four-sided visible focus should be available on all the UI elements present within screen. screenshot:

image

issue11:Fixed.
issue12:Fixed.

Retest Environment:
URL: https://deploy-preview-7373--fundamental-ngx.netlify.app/
Browser: Version 101.0.4951.54 (Official Build) (64-bit)
screen Reader; JAWS 2022.2204.20ILM

Regards,
Karthikeyan.c

@Karthickc1994 Karthickc1994 added the rework required Validation failed label May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ATL to verify core Core library specific issues rework required Validation failed stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility Testing_Core_Toolbar
8 participants