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

Remove skip-link-focus-fix.js #1653

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Remove skip-link-focus-fix.js #1653

merged 2 commits into from
Jul 27, 2021

Conversation

Ismail-elkorchi
Copy link
Contributor

@Ismail-elkorchi Ismail-elkorchi commented Apr 20, 2021

skip-link-focus-fix is only applicable in IE11, but it gets loaded even on the +95% of browsers which will never use it.

Now that Matt is supportive of dropping IE11 support in WP Core, I think it is the best time to remove it.

How to test the changes in this Pull Request:

  1. Open any StoreFront page in Safari (which is the only major browser using webkit).
  2. Tab to skip link.
  3. Press Enter to activate it.
  4. Make sure keyboard focus has moved to the first focusable area.
  5. Repeat this process for Opera and the other major browsers.

Changelog

Enhancement – Remove skip-link-focus-fix.js. #1653

@Ismail-elkorchi Ismail-elkorchi requested a review from a team as a code owner April 20, 2021 19:50
@Ismail-elkorchi Ismail-elkorchi requested review from senadir and removed request for a team April 20, 2021 19:50
@nerrad
Copy link
Contributor

nerrad commented Apr 30, 2021

We're definitely open to (and eventually will) remove IE11 only fixes, however, it's too premature for us to do so at this time. The current IE11 phase out plan starts with WordPress 5.8 release and with a focus on admin side support initially. Since our products still support earlier versions of WordPress we can't completely drop IE11 support until we bump up our minimum requirements.

I definitely appreciate the initiative here (and am looking forward to the day we can completely drop these IE11 workarounds!) but it'll be a while before we can merge pull requests like this.

@nerrad nerrad closed this Apr 30, 2021
@Ismail-elkorchi
Copy link
Contributor Author

WordPress 5.8 was already released, moreover, the WooCommerce developper blog state that IE11 support will be fully removed by WooCommerce 5.6, which will be released on August 10th.

In light of recent events, can this pull request be reopened and merged?

@nerrad
Copy link
Contributor

nerrad commented Jul 25, 2021

I've re-opened this for reconsideration. However, I'd like to see some testing instructions in the pull request description please - especially since the previous conditionals in the code also included webkit and opera user agent checks, so on the surface, the existing code doesn't look like it's addressing only IE11.

@nerrad nerrad reopened this Jul 25, 2021
@nerrad
Copy link
Contributor

nerrad commented Jul 25, 2021

It also appears the branch will need rebased on trunk.

@Ismail-elkorchi
Copy link
Contributor Author

Thank you for opening this PR for reconsideration.

I'd like to see some testing instructions in the pull request description please

Of course, I can do that.

the previous conditionals in the code also included webkit and opera user agent checks, so on the surface, the existing code doesn't look like it's addressing only IE11.

The skip link focus fix used in storefront is an old implementation that targeted old browsers and was first used in 2013 by _s in Automattic/_s#139. Then, it was updated in Automattic/_s#1053 to reflect a fix introduced in Twenty Seventeen by this ticket after discovering that this code doesn't work in IE11. In this ticket, the code targeting older versions of webkit and Opera was removed, and only IE11 was checked. In short, all the browsers have fixed the bug that this code is targeting, and was only needed for IE.

@frontdevde frontdevde added the type: technical debt This issue/PR represents/solves the technical debt of the project. label Jul 26, 2021
Copy link
Member

@mikejolley mikejolley left a comment

Choose a reason for hiding this comment

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

Safari is working. I think it makes sense to remove this. If a site really wants support for older, unsupported browsers, I think it would be trivial to add a script back. Let's make sure to explain this in the release notes.

@mikejolley mikejolley added this to the 3.8.0 milestone Jul 27, 2021
@mikejolley mikejolley merged commit 2246206 into woocommerce:trunk Jul 27, 2021
@Ismail-elkorchi Ismail-elkorchi deleted the remove-skip-link-fix branch July 28, 2021 01:00
@Ismail-elkorchi Ismail-elkorchi restored the remove-skip-link-fix branch July 28, 2021 13:47
@Aljullu Aljullu mentioned this pull request Aug 9, 2021
1 task
@Ismail-elkorchi Ismail-elkorchi deleted the remove-skip-link-fix branch August 10, 2021 23:25
@opr
Copy link
Contributor

opr commented Aug 17, 2021

If you need to support older browsers, and want to include skip-link-focus-fix.js on your site you may download the file from the above link and add it to a directory accessible by your website. You may then follow the instructions in the Theme Developer Handbook to enqueue the script on the front-end of your site.

@opr opr added type: enhancement The issue is a request for an enhancement. type: compatibility labels Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: compatibility type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants