Make WordPress Core

Opened 2 years ago

Closed 11 months ago

Last modified 9 months ago

#54421 closed enhancement (fixed)

Deprecate skip link focus fix

Reported by: westonruter's profile westonruter Owned by: sabernhardt's profile sabernhardt
Milestone: 6.3 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: has-patch has-dev-note
Focuses: accessibility, javascript, performance Cc:

Description (last modified by westonruter)

Six of the core themes have the skip link focus fix which was needed specifically for IE11. However, now that IE11 market share is at ~0.5%, it seems a waste to include the script on every single page even when barely any visitor will benefit from it.

Note that the WordPress announcement post for Dropping support for Internet Explorer 11 does say that no changes to bundled themes would be done as part of the plan, including:

No code related to IE11 support (or any other browser that may have been supported when each theme was released) will be removed from default themes.

Nevertheless, I think an exception can be made here given only a fraction of IE11 users (which is already a very small fraction of users) would also need this focus fix.

So I suggest we:

  1. Remove the add_action() call to print the script at wp_print_footer_scripts.
  2. We add a @deprecated tag to the *_skip_link_focus_fix functions.
  3. Discontinue enqueueing skip-link-focus-fix scripts in older themes, but continue to register them.

Attachments (1)

child-themes-restoring-script.zip (886.4 KB) - added by sabernhardt 11 months ago.
collection of child themes to test restoring the script, with variations for the external scripts

Download all attachments as: .zip

Change History (36)

#1 follow-up: @joedolson
2 years ago

In the cases where this is needed, it may be significant (e.g. company intranet environments where IE11 is potentially still required & employees with disabilities need access to intranet resources). So while in general I support the idea of removing these scripts for most sites, I think it should be fairly easy to re-implement them.

Possibly use feature detection to load these only in IE?

#2 in reply to: ↑ 1 @westonruter
2 years ago

Replying to joedolson:

So while in general I support the idea of removing these scripts for most sites, I think it should be fairly easy to re-implement them.

Yes, and it should still be so. In Twenty Twenty, for example, the change I'd suggest is:

  • src/wp-content/themes/twentytwenty/functions.php

    a b add_action( 'wp_enqueue_scripts', 'twentytwenty_register_scripts' ); 
    232232 * thus it does not warrant having an entire dedicated blocking script being loaded.
    233233 *
    234234 * @since Twenty Twenty 1.0
     235 * @deprecated IE11 global usage is at 0.5% so it is no longer warranted for all visitors.
    235236 *
    236237 * @link https://git.io/vWdr2
    237238 */
    function twentytwenty_skip_link_focus_fix() { 
    243244        </script>
    244245        <?php
    245246}
    246 add_action( 'wp_print_footer_scripts', 'twentytwenty_skip_link_focus_fix' );
    247247
    248248/**
    249249 * Enqueue non-latin language styles.

So if a specific site wants to re-implement the skip link focus fix, all they have to do is create a child theme or add a plugin with a single line:

<?php
add_action( 'wp_print_footer_scripts', 'twentytwenty_skip_link_focus_fix' );

Possibly use feature detection to load these only in IE?

Unfortunately page caching will often break user agent detection, so this isn't really feasible.

#3 @joedolson
2 years ago

I guess my concern is that this is going to be a largely invisible change to sites that might need it, so the discoverability that somebody would need to make this change is very low.

It is going to be a very low percentage of sites where it's relevant, however, so it's probably reasonable.

This ticket was mentioned in PR #1904 on WordPress/wordpress-develop by westonruter.


2 years ago
#4

  • Keywords has-patch added
  • Discontinue printing the skip link focus fix inline script by default in Twenty Nineteen, Twenty Twenty, and Twenty Twenty-One.
  • Discontinue enqueueing the skip link focus script (but continue to register it) in Twenty Fifteen, Twenty Sixteen, and Twenty Seventeen.
  • Add deprecation note to skip link focus fix in the functions.js file in Twenty Thirteen and Twenty Fourteen.
  • There is no skip link focus fix in Twenty Ten, Twenty Eleven, or Twenty Twelve.

Trac ticket: https://core.trac.wordpress.org/ticket/54421

#5 @westonruter
2 years ago

Just to confirm, note that IE11 is still the only browser needing this according to Is skip-link-focus-fix.js still necessary? in the _s repo.

#6 @westonruter
2 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

#7 @westonruter
2 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Version set to 3.6

#8 @westonruter
2 years ago

  • Description modified (diff)
  • Severity changed from trivial to minor

When working on the patch I realized that Twenty Fifteen, Twenty Sixteen, and Twenty Seventeen enqueue an external script for the skip link focus fix, so the severity is higher for those themes.

#9 @westonruter
2 years ago

  • Type changed from enhancement to defect (bug)

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#11 @sabernhardt
2 years ago

  • Milestone changed from 5.9 to 6.0
  • Type changed from defect (bug) to enhancement

This does not need to be changed immediately. Besides, I think removing the (legacy) IE conditional code could give more of a performance improvement for the older themes.

Twenty Twenty-One uses the $is_IE variable to determine whether to enqueue the IE stylesheet instead of the one with custom CSS properties. So if page caching breaks the browser detection, that theme would have a bigger problem than printing a 350-byte script for only a small number of people. And if someone needs the script but caching would remove it, asking the site owner to disable page cache should be easier than asking them to create a child theme that reinstates the script (or even to install another plugin for it).

Twenty Thirteen and Twenty Fourteen might be better to keep as they are. Including a deprecation notice is just adding more to the script file—and changing its modified date. Plus, I don't know if moving it outside of functions.js could help at all.

#12 @sabernhardt
2 years ago

Twenty Seventeen has console errors when removing/dequeuing the skip link JS because the twentyseventeenScreenReaderText variable used in navigation.js and global.js is localized on twentyseventeen-skip-link-focus-fix. That part might be a bug (on a separate ticket?). If the navigation script can follow global, the localization could go on twentyseventeen-global instead.

If site owners know they do not need to support IE users, they could remove the script with just a single line of code today for other themes:

// Remove from Twenty Fifteen (similar with twentysixteen).
add_action( 'wp_enqueue_scripts', function() { wp_dequeue_script( 'twentyfifteen-skip-link-focus-fix' ); }, 11 );

// Remove from Twenty Twenty-One (similar with twentynineteen and twentytwenty).
add_action( 'wp', function() { remove_action( 'wp_print_footer_scripts', 'twenty_twenty_one_skip_link_focus_fix' ); } );

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#14 @ryokuhi
2 years ago

  • Milestone 6.0 deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

We reviewed again this ticket today during the accessibility team's bug-scrub.

While we understand that there might be some performance improvements in dropping this hack, we agreed that the improvement is probably going to be marginal.

On the other side, that would require to make an exception to the policy about not making changes because of dropping support.

As such, we agreed to close this ticket as wontfix: in case, this ticket can always be reopened and re-reviewed.

#15 @sabernhardt
11 months ago

  • Milestone set to 6.3
  • Priority changed from low to normal
  • Resolution wontfix deleted
  • Severity changed from minor to normal
  • Status changed from closed to reopened
  • Type changed from enhancement to defect (bug)

As noted in ticket:57031#comment:7, this is worth reconsidering and I think it's enough to reclassify as a bug for some of the themes.

#16 @sabernhardt
11 months ago

  • Owner changed from westonruter to sabernhardt
  • Status changed from reopened to accepted

You could see my untested draft now, though I probably will continue to edit it (at least some of the documentation/comments).

#17 @sabernhardt
11 months ago

#57031 was marked as a duplicate.

This ticket was mentioned in PR #4487 on WordPress/wordpress-develop by @sabernhardt.


11 months ago
#18

  • Removes scripts from the wp_print_footer_scripts action in Twenty Nineteen, Twenty Twenty and Twenty Twenty-One.
  • Switches enqueue functions to register to deactivate the scripts in Twenty Fifteen, Twenty Sixteen and Twenty Seventeen.
  • Rearranges Twenty Seventeen's scripts to connect twentyseventeenScreenReaderText with the global script instead of the unused skip link fix.
  • Updates scripts in Twenty Fifteen and Twenty Sixteen with code from Twenty Seventeen. Twenty Sixteen needed to keep an adjustment that offsets the toolbar and border.
  • Removes the script from JS files in Twenty Thirteen and Twenty Fourteen and edits their modified dates.

Trac 54421

#19 @sabernhardt
11 months ago

I restarted with a fresh branch for PR 4487. It includes the recommendations from @westonruter and @joedolson, but I hesitated to add the code quality improvement suggested by @mukesh27.

Also, Twenty Sixteen's offset technically should have been -67 for widths from 700 to 782 pixels and -46 for narrower widths. However, encountering a problem with the 53-pixel offset would involve:

  1. Accessing a site that continues to enqueue this script,
  2. Using Internet Explorer,
  3. Sizing the window to the 700-782 pixel range (or zoom equivalent),
  4. Logging in to the site, and
  5. Activating the skip link.

To consider increasing it for everyone, see the offset comparison between 53 and 67 pixels.

@sabernhardt
11 months ago

collection of child themes to test restoring the script, with variations for the external scripts

#20 @sabernhardt
11 months ago

To test enqueuing the external scripts as a dependency or enqueuing the file by name, you could switch the active action in the functions.php file for the Twenty Fifteen, Twenty Sixteen and Twenty Seventeen child themes.

@sabernhardt commented on PR #4487:


11 months ago
#22

The modified dates are set to 20230627 (for now) in the four oldest themes, which should change if the PR is committed before the day of Beta 1.

#23 @westonruter
11 months ago

  • Keywords commit added

@flixos90 commented on PR #4487:


11 months ago
#24

@sabernhardt

The modified dates are set to 20230627 (for now) in the four oldest themes, which should change if the PR is committed before the day of Beta 1.

Do you mean whenever it's committed prior to Beta 1, the date of the commit should be used?

@sabernhardt commented on PR #4487:


11 months ago
#25

Yes, using the date of the commit would be good. Probably most of the external resources use that as the version, though it is not consistent throughout the themes (some styles and scripts use the planned release date).

#26 @sabernhardt
11 months ago

  • Type changed from defect (bug) to enhancement

Apparently the patch does not solve the iPhone issue on #38338, so I'm switching this back to an enhancement.

#28 @westonruter
11 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55861:

Bundled Themes: Remove/disable obsolete IE-specific skip-link-focus-fix.

  • Removes script from the wp_print_footer_scripts action in Twenty Nineteen, Twenty Twenty, and Twenty Twenty-One.
  • Switches enqueue functions to just register the scripts in Twenty Fifteen, Twenty Sixteen, and Twenty Seventeen.
  • Rearranges Twenty Seventeen's scripts to connect twentyseventeenScreenReaderText with the global script instead of the unused skip link fix.
  • Updates scripts in Twenty Fifteen and Twenty Sixteen with code from Twenty Seventeen _to run on Internet Explorer only_. Twenty Sixteen needed to keep an adjustment that offsets the toolbar and border.
  • Removes the script from JS files in Twenty Thirteen and Twenty Fourteen and edits their modified dates.

Props sabernhardt, westonruter, joedolson, flixos90, mukesh27.
Fixes #54421.

@westonruter commented on PR #4487:


11 months ago
#29

I noticed in fd6d5db that I put the wrong date, but I fixed that in what I committed to trunk: https://github.com/WordPress/wordpress-develop/commit/404fd63386493dd96dd0af8a494ee30fb76362de

This ticket was mentioned in Slack in #core by westonruter. View the logs.


10 months ago

This ticket was mentioned in Slack in #core by sabernhardt. View the logs.


9 months ago

#33 @stevenlinx
9 months ago

  • Keywords needs-dev-note added

#34 @sabernhardt
9 months ago

  • Keywords commit removed

If anyone would like to review the dev note, I started a post draft (also in a Google doc if you prefer that).

Note: See TracTickets for help on using tickets.