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

MWPW-142622: [Project PEP] Close button focus #1897

Merged

Conversation

robert-bogos
Copy link
Contributor

Description

This PR fixes the bug where the blue focus outline of the PEP X button does not show, when PEP is initiated after the region picker modal.

Related Issue

Resolves: MWPW-142622

Testing instructions

  1. Go to the test URL;
  2. Sign in;
  3. Wait for the region picker modal to show up and close it;
  4. When PEP is initiated, the X button should be focused and have a blue outline;

Screenshots (if appropriate):

image

Test URLs

Milo:

overmyheadandbody and others added 4 commits February 16, 2024 15:53

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…y loaded (adobecom#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
@robert-bogos robert-bogos added the needs-verification PR requires E2E testing by a reviewer label Feb 19, 2024
@robert-bogos robert-bogos self-assigned this Feb 19, 2024
@robert-bogos robert-bogos requested a review from a team as a code owner February 19, 2024 08:54
Copy link
Contributor

aem-code-sync bot commented Feb 19, 2024

Page Scores Audits Google
/drafts/rbogos/pep-prompt?martech=off&imsClientId=fedsmilo PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (project-pep@6a8efc7). Click here to learn what that means.

Additional details and impacted files
@@              Coverage Diff               @@
##             project-pep    #1897   +/-   ##
==============================================
  Coverage               ?   95.66%           
==============================================
  Files                  ?      159           
  Lines                  ?    41289           
  Branches               ?        0           
==============================================
  Hits                   ?    39499           
  Misses                 ?     1790           
  Partials               ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -138,6 +138,10 @@
left: 12px;
}

.appPrompt-close:focus {
outline: 2px solid #157AF3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me, however I'm wondering whether we should use variables for colours. Maybe @overmyheadandbody has additional feedback on this.
Also, one thing I noticed: the focus order is "cancel" button, then "X", even though the "X" mark is higher than the "cancel" button. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The focus order should be indeed different, but that's addressed in another pending PR

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't set any other focus colors on any of the Gnav elements. This is intentional, as we want all page elements to be consistent in their focused state. Since a lot of other elements don't have an explicit value, it means we ultimately rely on the browser to set this color, through its default :focus-visible value. Setting an explicit value might break this consistency for this particular element. Is there no other way of handling this issue, but maintain the user agent defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@overmyheadandbody I'll investigate further and see if there's a way without setting the focus color 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a solution that preserves the user agent focus color, it's a bit off on safari, but it's better than what we had before

@SilviuLCF SilviuLCF added verified PR has been E2E tested by a reviewer and removed needs-verification PR requires E2E testing by a reviewer labels Feb 21, 2024
@SilviuLCF SilviuLCF self-requested a review February 21, 2024 09:12
Copy link

@SilviuLCF SilviuLCF left a comment

Choose a reason for hiding this comment

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

QA verified , PR can be merged

Copy link
Contributor

@overmyheadandbody overmyheadandbody left a comment

Choose a reason for hiding this comment

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

This comes with the risk of some consistency issues, maybe we could find another way of handling this

@@ -138,6 +138,10 @@
left: 12px;
}

.appPrompt-close:focus {
outline: 2px solid #157AF3;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't set any other focus colors on any of the Gnav elements. This is intentional, as we want all page elements to be consistent in their focused state. Since a lot of other elements don't have an explicit value, it means we ultimately rely on the browser to set this color, through its default :focus-visible value. Setting an explicit value might break this consistency for this particular element. Is there no other way of handling this issue, but maintain the user agent defaults?

…wpw-142622-project-pep
@overmyheadandbody overmyheadandbody merged commit 69b5a5e into adobecom:project-pep Feb 26, 2024
10 checks passed
overmyheadandbody added a commit to overmyheadandbody/milo that referenced this pull request Feb 26, 2024
* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (adobecom#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (adobecom#1889)

* hotfix: blue outline on close button when focused

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>
overmyheadandbody added a commit that referenced this pull request Mar 21, 2024
* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (#1889)

* hotfix: blue outline on close button when focused

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>
overmyheadandbody added a commit that referenced this pull request Apr 8, 2024
* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (#1889)

* hotfix: blue outline on close button when focused

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>
overmyheadandbody added a commit to overmyheadandbody/milo that referenced this pull request Apr 29, 2024
* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (adobecom#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (adobecom#1889)

* hotfix: blue outline on close button when focused

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>
overmyheadandbody added a commit that referenced this pull request Apr 29, 2024
* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (#1889)

* hotfix: blue outline on close button when focused

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>
Blainegunn pushed a commit that referenced this pull request May 3, 2024
* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (#1889)

* MWPW-141021: Project PEP Accessibility Requirements (#1853)

* [MWPW-139990] PEP prompt

* pep accessibility requirements

* hotfix

* hotfixes

* initial focus on close icon

* implementing feedback

* hotfix

* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (#1889)

* implementing feedback

* css variables

* hotfix: moved css variables

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>

* MWPW-142622: [Project PEP] Close button focus  (#1897)

* [MWPW-139990] PEP prompt

* MWPW-142617: Microsoft Edge & Chrome Win - progress bar not completely loaded (#1882)

* hotfix

* refactor: changed loader animation logic

* update

* revert update

* more gpu acceleration for pep loader

* hotfix

* hotfix

* [MWPW-141023] PEP authoring (#1889)

* hotfix: blue outline on close button when focused

* hotfix

---------

Co-authored-by: Rares Munteanu <overmyheadandbody@gmail.com>

* MWPW-141020: [Project PEP] Implement Analytics Enablement (#1951)

* pep analytics

* hotfix

* hotfix

* hotfix

* removed capitalization from analytics label

* [MWPW-141050] Fetch profile for PEP

* [MWPW-141025] Integrate XLG with PEP

* [MWPW-142625] PEP relative to App Switcher

* MWPW-144108: Project PEP unit tests (#2136)

* unit tests for pep

* fixes for gnav unit tests

* optimizing pep unit tests

* MWPW-146582: PEP fixes (#2144)

* added aria-label attribute to pep close button

* force load imslib.min.js

* revert force load imslib.min.js

* changed aria label for close button

* pep final touches

* [MWPW-147145] Allow SVG in PEP Prompt; change redirect method

---------

Co-authored-by: Robert Bogos <146744221+robert-bogos@users.noreply.github.com>
Co-authored-by: Robert Bogos <robert.adobe.github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants