WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 6 weeks ago

#45066 closed enhancement (fixed)

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 needs-refresh
Focuses: accessibility, javascript Cc:
PR Number:

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 (14)

45066.2.diff (6.4 KB) - added by herregroen 13 months ago.
45066.3.diff (7.1 KB) - added by desrosj 12 months ago.
45066.4.diff (6.5 KB) - added by herregroen 12 months ago.
45066.5.diff (7.0 KB) - added by herregroen 12 months ago.
45066.6.diff (10.0 KB) - added by herregroen 12 months ago.
45066-indented.diff (65.1 KB) - added by herregroen 12 months ago.
45066.7.diff (6.7 KB) - added by herregroen 12 months ago.
45066.8.diff (6.4 KB) - added by sstoqnov 7 months ago.
Replace wp-a11y.js with @wordpress/a11y & fix failing tests
45066.9.diff (1.4 KB) - added by sstoqnov 7 months ago.
Remove wp-a11y from build process
45066.diff (3.9 KB) - added by whyisjake 2 months ago.
combined diff
45066.hotfix.diff (1.4 KB) - added by whyisjake 2 months ago.
Hotfix for the failing tests.
45066.hotfix.1.diff (1.2 KB) - added by whyisjake 2 months ago.
45066.hotfix.2.diff (1.6 KB) - added by whyisjake 2 months ago.
45066.tests-deps.diff (622 bytes) - added by ocean90 7 weeks ago.

Download all attachments as: .zip

Change History (58)

#1 @herregroen
13 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
13 months ago

#2 @afercia
13 months ago

  • Description modified (diff)
  • Focuses accessibility added

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

#3 @omarreiss
13 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
13 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
12 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
12 months ago

#6 @desrosj
12 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
12 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
12 months ago

@herregroen
12 months ago

#9 @herregroen
12 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
12 months ago

#10 @herregroen
12 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
12 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
12 months ago

#12 @herregroen
12 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
12 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
12 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
11 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#16 @pento
11 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.


11 months ago

#18 @desrosj
11 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
10 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
10 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.


8 months ago

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


8 months ago

#23 @desrosj
8 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
7 months ago

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

@sstoqnov
7 months ago

Remove wp-a11y from build process

#24 @sstoqnov
7 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.

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


2 months ago

#26 @audrasjb
2 months ago

Hello, we looked into this ticket during accessibility bug scrub.
Any update on this ticket? As a reminder, 5.3 beta 1 is on September 23. Do we need more work/test before commit?

#27 @desrosj
2 months ago

  • Keywords needs-refresh added

Thanks for the refresh @sstoqnov!

I applied this patch and did some testing. The tests pass now. However, when I use the admin area, View Details for themes is broken, and there are several console errors that I am seeing throughout the admin area.

#28 @sstoqnov
2 months ago

Hey @desrosj

Thanks for the feedback.
I just applied the patch to the latest trunk code, but the "Theme details" screen looks ok and I don't see any errors in the console.

Would it be possible to provide more information about those errors please?

#29 @afercia
2 months ago

To my understanding 45066.8.diff and 45066.9.diff need to be applied together, correct?

45066.8.diff doesn't apply cleanly for me, using grunt patch.

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


2 months ago

#31 @sstoqnov
2 months ago

Correct both patches have to be merged.

@whyisjake
2 months ago

combined diff

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


2 months ago

@whyisjake
2 months ago

Hotfix for the failing tests.

#33 @SergeyBiryukov
2 months ago

[46167], for reference (missed the ticket):

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

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.

Props omarreiss, herregroen, desrosj, ocean90, afercia, sstoqnov

#34 @whyisjake
2 months ago

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

In 46169:

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

Continuation of [46167]. This fixes the tests that ended up broken following the previous commit.

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.

Props, garrett-eclipse

Fixes #45066

#35 @garrett-eclipse
2 months ago

@whyisjake I believe this is resulting in plugins not able to install see #48071

#36 @afercia
2 months ago

I think $scripts->add( 'wp-a11y' ... needs to be removed from script-loader.php and after that npm install && grunt build? Otherwise, the old wp-a11y is still enqueued and gives a 404 in many pages (not only plugins).

#37 @afercia
2 months ago

In 46179:

Script Loader: Remove old wp-a11y from the loaded scripts after [46167] and [46169].

Props garrett-eclipse, sstoqnov.
See #45066.
Fixes #48071.

#38 @SergeyBiryukov
2 months ago

In 46180:

Tests: Update unit tests to account for the removal of old wp-a11y from the loaded scripts in [46179].

Props sstoqnov.
See #45066, #48071.

#39 @SergeyBiryukov
2 months ago

In 46181:

Docs: Format multi-line comment in [46180] per the documentation standards.

See #45066.

#40 @whyisjake
2 months ago

Thanks y'all for the fix!

#41 @Hareesh Pillai
2 months ago

Although all the QUnit tests are passing, I observe the following console error when running the unit tests.
Uncaught TypeError: external_this_wp_domReady_default(...) is not a function on line 208 of the a11y.js file.

This ticket was mentioned in Slack in #core by hareesh-pillai. View the logs.


8 weeks ago

#43 @desrosj
8 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to investigate the TypeError above.

#44 @ocean90
6 weeks ago

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

In 46388:

QUnit: Include script dependencies for wp-a11y script to fix a TypeError.

Also, add missing message argument to Customizer notifications to ensure tests are passing.

Fixes #45066.

Note: See TracTickets for help on using tickets.