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

Make skip link for Full Site Editing themes AMP compatible #6115

Closed
pierlon opened this issue Apr 24, 2021 · 3 comments · Fixed by #6823
Closed

Make skip link for Full Site Editing themes AMP compatible #6115

pierlon opened this issue Apr 24, 2021 · 3 comments · Fixed by #6823
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Integration: Gutenberg P1 Medium priority WS:Core Work stream for Plugin core
Milestone

Comments

@pierlon
Copy link
Contributor

pierlon commented Apr 24, 2021

Bug Description

Gutenberg 10.5 adds a skip link that points to the main element when a FSE theme is active. This is accomplished with a bit of CSS and JS (see WordPress/gutenberg#30336) but that solution isn't AMP compatible. This now causes a validation error to be raised for the invalid script:

image

Expected Behaviour

A validation error should not be raised for the skip link functionality when using a FSE theme.

Steps to reproduce

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Screenshots

Additional context

  • WordPress version:
  • Plugin version:
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode:
  • PHP version:
  • OS:
  • Browser: [e.g. chrome, safari]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@pierlon pierlon added Bug Something isn't working WS:Core Work stream for Plugin core labels Apr 24, 2021
@westonruter westonruter added this to the v2.1.1 milestone Apr 24, 2021
@westonruter westonruter modified the milestones: v2.1.1, v2.1 Apr 28, 2021
@westonruter westonruter modified the milestones: v2.1, v2.1.1 Apr 28, 2021
@westonruter
Copy link
Member

I've added a workaround to omit the script in #6115 as it was needed to make the tests pass. This issue remains needed to actually implement the functionality.

As for how to implement support, I think gutenberg_the_skip_link() is somewhat of a missed opportunity. If full-site editing is active, then this means all of the blocks can be manipulated as data. Therefore PHP should rather be used to insert the a.skip-link.screen-reader-text rather than using JS to inject it. This will also ensure the skip link works when JS is turned off. I suggest we contribute that upstream to Gutenberg.

@dhaval-parekh
Copy link
Collaborator

Therefore PHP should rather be used to insert the a.skip-link.screen-reader-text rather than using JS to inject it. This will also ensure the skip link works when JS is turned off. I suggest we contribute that upstream to Gutenberg.

We can use PHP but it needs to be with PHP DOM since while adding skip link we need to find the ID of the target element.

@maitreyie-chavan maitreyie-chavan removed this from the v2.3 milestone Jan 13, 2022
@maitreyie-chavan maitreyie-chavan added this to the v2.2.1 milestone Jan 13, 2022
@maitreyie-chavan maitreyie-chavan added P1 Medium priority and removed P0 High priority labels Jan 13, 2022
@fellyph
Copy link
Collaborator

fellyph commented Jan 28, 2022

Tested at 2.2.x and QA Passed

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Integration: Gutenberg P1 Medium priority WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants