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 169999 header obscures content #3898

Open
wants to merge 8 commits into
base: stage
Choose a base branch
from

Conversation

zagi25
Copy link
Contributor

@zagi25 zagi25 commented Apr 3, 2025

Issue: When navigating a page by using Tab or Shift + Tab , focused element that is hidden by gnav, sticky element, cookie banner etc. remains hidden.
Screenshot 2025-04-03 at 15 29 25

  • Element gets scrolled to the middle of the screen when focused by Tab or Shift + Tab
  • Cookie banner get's position sticky, when there is visible focus on the page and focus is not inside cookie banner, so that when page is scrolled to the end using Tab all elements in the footer can be focused and are not covered by the cookie banner ( This was done with the assumption that if the cookie banner is present it is always the last element in the body )

I tried setting scroll-padding-top and scroll-padding-bottom to the exact height of the elements that are hiding other content, but this requires JS and it will bring unnecessary complexity trying to cover all the use cases.

Resolves: MWPW-169999 , MWPW-170165

Test URLs:

Homepage URLs:

CC URLs:

QE NOTE: Please close the MWPW-170165 with the reason fixed by MWPW-169999 if this also resolves the issue described in that ticket.

@zagi25 zagi25 requested a review from a team April 3, 2025 13:58
Copy link
Contributor

aem-code-sync bot commented Apr 3, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Contributor

aem-code-sync bot commented Apr 3, 2025

@aem-code-sync aem-code-sync bot temporarily deployed to MWPW-169999-header-obscures-content April 3, 2025 14:09 Inactive
Copy link
Contributor

github-actions bot commented Apr 4, 2025

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

@aem-code-sync aem-code-sync bot temporarily deployed to MWPW-169999-header-obscures-content April 7, 2025 09:12 Inactive
@zagi25
Copy link
Contributor Author

zagi25 commented Apr 7, 2025

@mokimo I pushed solution using JS.

  • Value of scroll padding is taken from sticky sections / cookie banner height
  • Values are updated on resize, sticky section closing and hiding of the cookie banner

I left some comments for easier review, will remove them later. I added the code to accessibility file, if you think it should be added somewhere else let me know (same goes for calling of the function)

@aem-code-sync aem-code-sync bot temporarily deployed to MWPW-169999-header-obscures-content April 7, 2025 09:41 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to MWPW-169999-header-obscures-content April 8, 2025 12:40 Inactive
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.

I quite like this approach and it has worked well during my testing. Just a few small nits, but looking great otherwise and much more straightforward than the previous implementation 👌

position: sticky;
bottom: 0;
/* #onetrust-banner-sdk z-index value */
z-index: 2147483645;
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 still not getting what this achieves. I've removed it entirely and things seem to continue working. What use-case is this trying to address?

Screenshot 2025-04-09 at 11 42 04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know if this will ever happen, but without z-index if user scrolls while focus is in footer
Screenshot 2025-04-09 at 16 01 34

Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

The experience is a lot better when tabbing, glad this went through a few iterations and we really managed to improve it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants