Skip to content

Aria-atomic="true" missing on error container #27386

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

Merged

Conversation

bradleybrecher
Copy link
Contributor

@bradleybrecher bradleybrecher commented Mar 22, 2020

Description (*)

Issue : Aria-atomic="true" is missing on error container

Summary: The page contains an error container made with the WAI-ARIA role="alert" attribute, but it does not contain a WAI-ARIA attribute that will make sure all assistive technology can read the error message after more than one invalid submission.

Fix: To make sure that all assistive technologies will read the error message after more than one invalid submission, added the WAI-ARIA attribute aria-atomic="true" to the error container tag.

Related Pull Requests

N/A

Fixed Issues (if relevant)

N/A

Precondition:

have Cart price rule with discount code;

Manual testing scenario:

  1. Go to Storefront;
  2. Add product to cart;
  3. Go to Shopping Cart;
  4. Expand Apply Discount Code tab and fill correct code

Expected Result: ✔️ The success message is shown on the page

Screenshot from 2020-09-03 11-19-16

  1. Try to fill with incorrect code

Expected Result: ✔️ The error message is shown on the page

Screenshot from 2020-09-03 11-19-06

  1. Proceed to Checkout;
  2. Fill all required fields and go to Review & Payments step;
  3. Expand the Apply Discount Code tab and fill correct code

Expected Result: ✔️ The success message is shown on the page

Screenshot from 2020-09-03 11-13-30

  1. Try to fill with incorrect code

Expected Result: ✔️ The error message is shown on the page

Screenshot from 2020-09-03 11-13-19

Questions or comments

N/A

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Aria-atomic="true" missing on error container #29560: Aria-atomic="true" missing on error container

@m2-assistant
Copy link

m2-assistant bot commented Mar 22, 2020

Hi @bradleybrecher. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@rogyar
Copy link
Contributor

rogyar commented Mar 22, 2020

I can confirm that it's a good idea to wrap all messages into the area-atomic DOM element since we stack messages in this DOM element in case of more than one message.

Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

@bradleybrecher. Thank you for your contribution. According to the new definition of done all changes should be covered by automated tests. I would recommend covering this change by MFTF test that will check that aria-atomic tag is present in case of error/success message on the page. You can use the following example of the selector that shows how to create a proper selector and assert it using seeElement command.

<element name="successMessage" type="text" selector="//*[@data-ui-id='messages-message-success']" timeout="120"/>

Thank you.

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@engcom-Charlie
Copy link
Contributor

@magento run all tests

@engcom-Charlie engcom-Charlie requested a review from rogyar August 6, 2020 17:43
@rogyar rogyar added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Aug 9, 2020
@rogyar
Copy link
Contributor

rogyar commented Aug 9, 2020

Hi @engcom-Charlie. I would still recommend using a separate test case instead of modifying test selectors. My concern is if [aria-atomic=true] will be removed or modified for the DOM elements for some reason, we will have many failing tests instead of one. If we don't change selectors but introduce a new test that ensures that the corresponding selectors have [aria-atomic=true], we will potentially have only one test failing in case of changing the corresponding DOM element.

I hope, I described both cases clear :)

@engcom-Charlie
Copy link
Contributor

Hi @rogyar, do you mean it should be a new mftf test or add a selector check to some already written test?
Thank you.

@rogyar
Copy link
Contributor

rogyar commented Aug 10, 2020

Hi @engcom-Charlie. My recommendation was to introduce a new MFTF test that will go to the corresponding page and check that .message-success[aria-atomic=true] selector exists and visible. Without modifying the existing MFTF elements. In that way, we will preserve backward compatibility and reduce the chance of a couple of failing tests as the result of the DOM elements refactoring in the future.

Thank you!

@magento-engcom-team
Copy link
Contributor

@bradleybrecher thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@engcom-Alfa
Copy link
Contributor

engcom-Alfa commented Sep 3, 2020

✔️ QA Passed

Precondition:

have Cart price rule with discount code;

Manual testing scenario:

  1. Go to Storefront;
  2. Add product to cart;
  3. Go to Shopping Cart;
  4. Expand Apply Discount Code tab and fill correct code

Actual Result: ✔️ The success message is shown on the page

Screenshot from 2020-09-03 11-19-16

  1. Try to fill with incorrect code

Actual Result: ✔️ The error message is shown on the page

Screenshot from 2020-09-03 11-19-06

  1. Proceed to Checkout;
  2. Fill all required fields and go to Review & Payments step;
  3. Expand the Apply Discount Code tab and fill correct code

Actual Result: ✔️ The success message is shown on the page

Screenshot from 2020-09-03 11-13-30

  1. Try to fill with incorrect code

Actual Result: ✔️ The error message is shown on the page

Screenshot from 2020-09-03 11-13-19

@engcom-Alfa engcom-Alfa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Sep 3, 2020
@engcom-Charlie
Copy link
Contributor

@magento run all tests

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-8120 has been created to process this Pull Request

@rogyar
Copy link
Contributor

rogyar commented Sep 6, 2020

Hi @engcom-Charlie. Cold you move this PR to the correct status, please? pull-request-dashboard-manager don't trust me in my decisions :)

Thank you

@m2-assistant
Copy link

m2-assistant bot commented Sep 7, 2020

Hi @bradleybrecher, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: SalesRule Component: Theme Component: Ui Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Issue] Aria-atomic="true" missing on error container
6 participants