Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#45066 closed enhancement (fixed)

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

Reported by: omarreiss's profile omarreiss Owned by: omarreiss's profile 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:

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

Download all attachments as: .zip

Change History (58)

#1 @herregroen
5 years 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
5 years ago

#2 @afercia
5 years ago

  • Description modified (diff)
  • Focuses accessibility added

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

#3 @omarreiss
5 years ago

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

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

#4 @ocean90
5 years 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
5 years 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
5 years ago

#6 @desrosj
5 years 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
5 years ago

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

@herregroen
5 years ago

@herregroen
5 years ago

#9 @herregroen
5 years 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
5 years ago

#10 @herregroen
5 years 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
5 years 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
5 years ago

#12 @herregroen
5 years 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
5 years 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
5 years 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
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#16 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

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


5 years ago

#18 @desrosj
5 years 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
5 years 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
5 years 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.


5 years ago

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


5 years ago

#23 @desrosj
5 years 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
5 years ago

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

@sstoqnov
5 years ago

Remove wp-a11y from build process

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


5 years ago

#26 @audrasjb
5 years 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
5 years 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
5 years 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
5 years 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.


5 years ago

#31 @sstoqnov
5 years ago

Correct both patches have to be merged.

@whyisjake
5 years ago

combined diff

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


5 years ago

@whyisjake
5 years ago

Hotfix for the failing tests.

#33 @SergeyBiryukov
5 years 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
5 years 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
5 years ago

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

#36 @afercia
5 years 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
5 years 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
5 years 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
5 years ago

In 46181:

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

See #45066.

#40 @whyisjake
5 years ago

Thanks y'all for the fix!

#41 @Hareesh Pillai
4 years 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.


4 years ago

#43 @desrosj
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to investigate the TypeError above.

#44 @ocean90
4 years 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.