WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 3 months ago

#45066 assigned enhancement

Replace wp-a11y.js with @wordpress/a11y package

Reported by: omarreiss Owned by: omarreiss
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-testing has-unit-tests
Focuses: accessibility, javascript Cc:

Description (last modified by afercia)

We've extracted wp-includes/wp-a11y.js to a reusable package wordpress/a11y which is published on NPM. Let's make sure this package is also used in WordPress core. Once all the JavaScript is built using webpack, we can also import this package wherever it is used and configure webpack to load is an external.

Attachments (10)

45066.diff (7.9 KB) - added by herregroen 9 months ago.
45066.2.diff (6.4 KB) - added by herregroen 9 months ago.
45066.3.diff (7.1 KB) - added by desrosj 8 months ago.
45066.4.diff (6.5 KB) - added by herregroen 8 months ago.
45066.5.diff (7.0 KB) - added by herregroen 8 months ago.
45066.6.diff (10.0 KB) - added by herregroen 8 months ago.
45066-indented.diff (65.1 KB) - added by herregroen 8 months ago.
45066.7.diff (6.7 KB) - added by herregroen 8 months ago.
45066.8.diff (6.4 KB) - added by sstoqnov 3 months ago.
Replace wp-a11y.js with @wordpress/a11y & fix failing tests
45066.9.diff (1.4 KB) - added by sstoqnov 3 months ago.
Remove wp-a11y from build process

Download all attachments as: .zip

Change History (34)

@herregroen
9 months ago

#1 @herregroen
9 months ago

Added a patch that fully removes the original registration including the file.

As the wp-a11y handle is already registered no further action is required. Some PHPUnit tests required minor modifications in order to ensure wp-a11y is registered and to account for it's dependencies.

@herregroen
9 months ago

#2 @afercia
9 months ago

  • Description modified (diff)
  • Focuses accessibility added

Seems to me everything works as expected, from an accessibility perspective. Thanks!

#3 @omarreiss
8 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 5.0

Signing off for commit. @gziolo, @herregroen or @atimmer. Feel free.

#4 @ocean90
8 months ago

  • Keywords has-patch needs-refresh added; commit removed

Before it gets committed it'd be good to fix the indentation in update-core.php and update the patch for [43877].

#5 @gziolo
8 months ago

expected  = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5B%5D=jquery-core,jquery-migrate,wp-polyfill-ecmascript,wp-dom-ready,wp-a11y&amp;ver={$ver}'></script>\n";

Should wp-polyfill-ecmascript be updated to wp-polyfill?

@desrosj
8 months ago

#6 @desrosj
8 months ago

  • Keywords needs-testing added; needs-refresh removed

In 45066.3.diff:

  • Fixed indentation in update-core.php.
  • Changed wp-polyfill-ecmascript to wp-polyfill.
  • Fixes failing tests due to wp_get_script_polyfill() now being triggered by wp-polyfill.

Also, wp_default_packages_vendor() and wp_default_packages_scripts() were already being called in the 5.0 branch. Since wp_default_packages() calls these, I removed those calls.

#7 @ocean90
8 months ago

@desrosj Using wp_default_packages() will cause a fatal error. There’s no need to switch to that, see #45271 for background.

@herregroen
8 months ago

@herregroen
8 months ago

#9 @herregroen
8 months ago

@afercia Seems that way.

Just updated the patch to correct that indentation.

I've also changed the expected test output to unminified files. wp-tests-config-sample.php sets the ABSPATH as the src directory in which case unminified JS sources are used.

@herregroen
8 months ago

#10 @herregroen
8 months ago

Or it might make more sense to indent everything as it was. Not sure why the change in the 5.0 branch was made.

#11 @ocean90
8 months ago

Let's keep the indentation as in 45066.4.diff to keep the diff small. Trunk has the code style fixes applied which the 5.0 branch has not since it was branched from 4.9.

@herregroen
8 months ago

#12 @herregroen
8 months ago

@ocean90 Latest patch updated with both the adapted tests and smallest changes in regards to update-core.php.

Testing everything shows no issues so the latest patch should be good for commit.

#13 @omarreiss
8 months ago

  • Milestone changed from 5.0 to 5.0.1

Hmm, I signed off 11 days ago. Now it seems late in the process to commit this nice to have. Punting to 5.0.1. This only removes some duplication after all.

#14 @desrosj
8 months ago

Talked this through with @herregroen and wanted to put this here for visibility. Locally, the tests in the most recent patch fail for me. But it is a .min vs no .min issue. My site was provisioned in VVV using trunk, which has the following conditional:

if ( defined( 'WP_RUN_CORE_TESTS' ) && WP_RUN_CORE_TESTS ) {
	define( 'ABSPATH', dirname( __FILE__ ) . '/build/' );
} else {
	define( 'ABSPATH', dirname( __FILE__ ) . '/src/' );
}

WP_RUN_CORE_TESTS is defined in phpunit.xml.dist as 1 and would cause the tests to return the minified versions.

When this gets ported over to trunk, the tests may need to change to look for the minified versions.

#15 @pento
7 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#16 @pento
7 months ago

  • Milestone changed from 5.0.2 to 5.0.3

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


7 months ago

#18 @desrosj
7 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.0.3 to 5.1

Patch is no longer applying cleanly for me. Going to punt this to 5.1 because it is also still in need of testing.

#19 @desrosj
6 months ago

  • Milestone changed from 5.1 to 5.2

This still needs a refresh. Punting to 5.2.

@herregroen If you able to refresh before 5.1 Beta1 tomorrow we can look at including it.

#20 @herregroen
6 months ago

@desrosj going to need some more time for this I'm afraid.

wp-polyfill is now a dependency of the packaged wp-a11y which means that during the unit tests the full polyfill browser tests are printed out which cause two tests to fail. The packaged scripts also contain various version strings that are package versions which I'm not sure are exposed.

Will need a bit of time to write an elegant way to unit test these as I don't think removing wp-a11y altogether from unit tests would be good. Having an elegant solution for that means we can also start adding unit tests for the other packages which right now aren't included at all in tests/phpunit/tests/dependencies/scripts.php I believe.

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


4 months ago

This ticket was mentioned in Slack in #core-js by omarreiss. View the logs.


4 months ago

#23 @desrosj
4 months ago

  • Milestone changed from 5.2 to 5.3

This ticket has not received any attention during the 5.2 cycle. With beta 1 tomorrow, going to punt this to 5.3.

@sstoqnov
3 months ago

Replace wp-a11y.js with @wordpress/a11y & fix failing tests

@sstoqnov
3 months ago

Remove wp-a11y from build process

#24 @sstoqnov
3 months ago

  • Keywords has-unit-tests added; needs-refresh removed

Hi,

I've used the previous patches and in 45066.8 the failing tests are fixed.

I was not sure if it ok to remove the wp-a11y from Gruntfile.js, so I've attached another patch 45066.9, where it's removed.

Note: See TracTickets for help on using tickets.