WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 9 hours ago

#47069 reopened defect (bug)

Twenty Nineteen: the admin bar on the front end has reduced functionalities and bugs due to jQuery not being used

Reported by: afercia Owned by: audrasjb
Milestone: 5.3.1 Priority: normal
Severity: normal Version: 5.0
Component: Toolbar Keywords: has-screenshots early has-patch 2nd-opinion
Focuses: accessibility, javascript Cc:
PR Number:

Description (last modified by afercia)

Twenty Nineteen [Edit: and also the next official theme Twenty Twenty] doesn't use jQuery on the front end. This is a welcomed effort to make Twenty Nineteen not depend on jQuery and use vanilla JavaScript instead.

Unfortunately, seems it hasn't been well considered that the WordPress admin bar has some functionalities that depend on jQuery.

The admin-bar.js file is split in two main parts: the original one uses vanilla JavaScript. The other part includes functionalities that were added over time and that depend on jQuery. A check for jQuery determines which part of the file is going to be used. As jQuery is undefined in the Twenty Nineteen front end, the old part of the file is used and many functionalities are missing.

Most importantly:

  • keyboard interaction with the menu: pressing Enter on a menu top level item doesn't open the menu any longer and instead navigates to the link destination, making the menu unusable with keyboard (this is nothing new, see https://core.trac.wordpress.org/ticket/19088#comment:26)
  • same happens on mobile: tapping a menu top level item triggers navigation instead of opening the menu, making the menu unusable
  • hoverIntent on the menu top level items can't work without jQuery: it was added 8 years ago in [18488] / #18299 together with other improvements
  • the search input field icon is missing

I can't think of good ways to fix this other than rewriting the whole admin-bar.js file in vanilla JavaScript. Worth noting a few functionalities are basically duplicated in the two parts. Other functionalities should be rewritten instead.

Considering that WordPress has officially ended support for old Internet Explorer versions since two years, this could be a good opportunity to make the admin bar use vanilla JavaScript.

Any thoughts and feedback very welcome.

Attachments (17)

Twenty Seventeen.png (62.9 KB) - added by afercia 7 months ago.
Twenty Seventeen: pressing Enter opens the menu
Twenty Nineteen.png (76.5 KB) - added by afercia 7 months ago.
Twenty Nineteen: pressing Enter doesn't open the menu and navigates to wp-admin/post-new.php
19 mobile admin bar.gif (43.8 KB) - added by afercia 7 months ago.
Mobile: tapping the menu triggers navigation instead of opening the menu dropdown
missing search icon.png (85.8 KB) - added by afercia 7 months ago.
Missing icon within the search input
47069.diff (584 bytes) - added by audrasjb 7 months ago.
First step: enqueue jQuery if the Admin Bar is displayed
admin-bar-hoverintent.diff (4.3 KB) - added by afercia 3 months ago.
admin-bar-no-jquery-wip.diff (13.6 KB) - added by afercia 3 months ago.
47069.2.diff (29.7 KB) - added by dinhtungdu 3 months ago.
06dac1be2b73076d44b0364c30d5e5c9.gif (229.0 KB) - added by audrasjb 3 months ago.
Testing patch and everything works as expected
47069.3.diff (29.0 KB) - added by dinhtungdu 3 months ago.
47069.4.diff (28.7 KB) - added by dinhtungdu 3 months ago.
47069.5.diff (28.8 KB) - added by dinhtungdu 2 months ago.
47069.6.diff (29.2 KB) - added by dinhtungdu 2 months ago.
47069.7.diff (705 bytes) - added by afercia 2 months ago.
47069.8.diff (10.2 KB) - added by afercia 7 weeks ago.
47069.9.diff (33.0 KB) - added by dinhtungdu 7 weeks ago.
Replace jQuery hoverIntent with vanilla JS hoverintent. Use scrollIntoView for smooth scrolling.
47069.10.diff (28.7 KB) - added by azaozz 2 weeks ago.

Download all attachments as: .zip

Change History (78)

@afercia
7 months ago

Twenty Seventeen: pressing Enter opens the menu

@afercia
7 months ago

Twenty Nineteen: pressing Enter doesn't open the menu and navigates to wp-admin/post-new.php

@afercia
7 months ago

Mobile: tapping the menu triggers navigation instead of opening the menu dropdown

@afercia
7 months ago

Missing icon within the search input

#1 @afercia
7 months ago

See also #34668, related to the admin bar menu when used with screen readers.

#2 @afercia
7 months ago

For some history on the reasons why the "click-to-open (Tab to select, Enter to open/close)" behavior was implemented on the admin bar menu, see #19088, specifically starting from ticket:19088#comment:34. Various opinions were then collected on a Make post, see https://make.wordpress.org/accessibility/2011/11/28/do-people-think-it-is-better-for-the/

#3 @afercia
7 months ago

One more thing to consider: the admin bar is rendered in the footer since (if I'm not wrong) its introduction in [15671]. Not sure why it's also moved with JS, see ticket:14772#comment:71 and see d.body.appendChild( aB ) in the admin-bar.js file.

Regardless, WordPress 5.2 is going to introduce the new wp_body_open hook. For the future, it would be worth considering to move the admin bar at the top of the markup, to make the visual order match the DOM order. See also #47053.

#4 @afercia
7 months ago

  • Description modified (diff)

#5 @desrosj
7 months ago

  • Keywords needs-patch has-screenshots added

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


7 months ago

#7 @afercia
7 months ago

  • Milestone changed from Awaiting Review to 5.3

Discussed during today's accessibility bug scrub. Agree to move to the 5.3 milestone for research and exploration. Two possible options came out from the bug scrub:

  • either enqueue jQuery in the front end when the admin bar is displayed
  • or rewrite the whole file admin-bar.js in vanilla JavaScript (with no jQuery methods)

A first step would be: research if a non-jQuery equivalent of hoverintent.js exists.

@audrasjb
7 months ago

First step: enqueue jQuery if the Admin Bar is displayed

#8 @audrasjb
7 months ago

  • Keywords has-patch added; needs-patch removed

#9 @audrasjb
4 months ago

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

#10 @afercia
3 months ago

A first step would be: research if a non-jQuery equivalent of hoverintent.js exists.

There's a plain JavaScript, MIT licensed, hoverintent see https://github.com/tristen/hoverintent

I think this would allow to refactor admin-bar.js and make it jQuery-free keeping the current functionalities. @azaozz do you think it is worth it? Twenty Nineteen (and I guess more themes in the future) don't use jQuery and it would be nice to respect their intent when logged-in users navigate the front end pages.

This ticket was mentioned in Slack in #core-themes by audrasjb. View the logs.


3 months ago

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


3 months ago

This ticket was mentioned in Slack in #themereview by aristath. View the logs.


3 months ago

#14 @westonruter
3 months ago

Instead of using hoverintent to cause the dropdown to appear, a pure CSS alternative to this would be :focus-within: https://caniuse.com/#feat=css-focus-within

In the AMP plugin this is used to make the admin bar's submenus keyboard accessible: https://github.com/ampproject/amp-wp/blame/1.2.2/assets/css/src/admin-bar.css (instances of .hover were replaced with :focus-within)

The only apparent downside to this is the submenu appears immediately upon tabbing to the element, as opposed to hovering for a bit or hitting enter (which IMO is harder to discover/use?). But, it works when JS is turned off or the JS failed to load.

#15 @afercia
3 months ago

@westonruter yup, but the main purpose of hoverintent is to add that brief delay. See discussion on [18488] / #18299.

Also, focus-within has no support in Internet Explorer 11 which is still a browser that WordPress officially supports. It would require the PostCSS focus within and the focus-within polyfill to work, which doesn't seem ideal to me.

Still, there won't be any delay when moving the mouse over the toolbar: all submenus would appear immediately, which is a very annoying experience.

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


3 months ago

#17 @afercia
3 months ago

Worth noting also Twenty Twenty won't use jQuery. Twenty Twenty is in the works and there's an issue and a pending PR to move from jQuery to plain JavaScript. See:

https://github.com/WordPress/twentytwenty/issues/12
https://github.com/WordPress/twentytwenty/pull/101

It appears this ticket is even more relevant now. During last weekly accessibility meeting on Slack, @aristath and @dinhtungdu expressed interest in working on a patch. It would be great to have a solution in time for WordPress 5.3.

During the meeting it was also noted that in the responsive view the admin bar menu dropdowns are broken because the touchevents necessary to make them work are based on jQuery. Tapping a top level item in the admin bar menu triggers navigation instead of opening the dropdown.

#18 @afercia
3 months ago

  • Description modified (diff)

#19 @afercia
3 months ago

  • Description modified (diff)

#20 @afercia
3 months ago

Sorry I don't have bandwidth to work on this ticket. I shared a couple test patches I was working on a few months ago. They're just proof of concepts I was playing with, not ready for anything else other than testing. Maybe they can help to get started.

admin-bar-hoverintent.diff uses the no-jQuery hoverintent from https://github.com/tristen/hoverintent. Seems to me it works fine. Should be replaced with the minified version.

admin-bar-no-jquery-wip.diff is a work in progress I was testing to progressively remove all the jQuery bits and re-implement them in plain JavaScript.

Last edited 3 months ago by afercia (previous) (diff)

@dinhtungdu
3 months ago

#21 @dinhtungdu
3 months ago

  • Keywords needs-testing added

47069.2.diff is my attempt to rewrite admin-bar.js using vanilla JS only without any external dependency. All features in the jQuery version have been ported.
APIs used in the file are checked with caniuse.com.

Last edited 3 months ago by dinhtungdu (previous) (diff)

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


3 months ago

#23 @audrasjb
3 months ago

Great job @dinhtungdu !
Everything is working fine on my side 👍

#24 @audrasjb
3 months ago

  • Keywords needs-testing removed

@audrasjb
3 months ago

Testing patch and everything works as expected

#25 @audrasjb
3 months ago

  • Keywords needs-refresh added

Looks like there is some coding standards errors though (I've got whitespaces warnings on my side).

Last edited 3 months ago by audrasjb (previous) (diff)

@dinhtungdu
3 months ago

@dinhtungdu
3 months ago

@dinhtungdu
2 months ago

#26 @dinhtungdu
2 months ago

  • Keywords needs-testing added; needs-refresh removed

47069.5.diff fixed some logic errors showed by jshint.

This patch needs cross-browser and cross-platform testing. Things to test:

Desktop:

  • nojs class is removed after the DOM is loaded.
  • Scroll page to top when clicking on the admin bar.
  • Focusing on #adminbar-search adds .adminbar-focused to #adminbarsearch. Unfocusing removes it.
  • HoverIntent: the hover event doesn't fire instantly but delayed for 180ms.
  • When a hash is present in the URL, the admin bar is hidden.

Keyboard navigation:

  • Pressing Enter on the Skip to toolbar button focuses the admin bar.
  • Pressing Enter on the menu opens the dropdown.
  • Pressing Escape on the menu and sub-menu closes the dropdown.

Touch devices:

  • hover class is removed when the user touches outside the menu items.
  • Tapping on the menu opens the dropdown menu. Tapping again close the dropdown.

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


2 months ago

@dinhtungdu
2 months ago

#28 @dinhtungdu
2 months ago

47069.6.diff addresses WordPress JS coding standard and adds some minor tweaks.

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


2 months ago

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


2 months ago

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


2 months ago

#32 @audrasjb
2 months ago

  • Keywords commit added

As discussed during Accessibility bug scrub, let's commit the jQuery option for the moment and reopen it for a best solution in 5.4.

@afercia
2 months ago

#33 @afercia
2 months ago

  • Keywords needs-testing commit removed

47069.7.diff takes the minimal approach and just enqueues jQuery as dependency of admin-bar.

For the future WP 5.4, the accessibility team strongly recommends refactoring admin-bar in plain JavaScript and make this ticket a task. For now, at this point of the 5.3 release, the minimal approach seems the only wise option.

This adds ~105 KB (minified but not gzipped) to the front end. Of course only when logged in and the user preference "Show Toolbar when viewing site" is on. Of course, theme that already use jQuery won't see any difference.

Worth reminding that for themes that don't use jQuery some of the functionalities of the admin bar aren't working. For example: hoverintent and, most notably, the admin bar menu on mobile is broken. It always worked this way, maybe because a few years ago it was pretty common for themes to always use jQuery.

Today, more and more themes don't use jQuery.

Twenty Nineteen and also the new Twenty Twenty don't. Also other teams in the ecosystem are more and more "jQuery free": I tested a few themes from 2016 and since they don't use jQuery, the admin bar is not fully functional.

In light of the release of Twenty Twenty this issue seems pretty urgent. I'd propose to bring this ticket to the attention of a larger audience during today's dev chat on Slack.

pinging @francina as 5.3 Release Coordinator
pinging @davidakennedy and @laurelfulford as maintainers of the Bundled Theme component
pinging @azaozz as 5.3 Core Tech focus lead
pinging @ianbelanger as 5.3 Default Theme Wrangler
pinging @anlino as 5.3 Default Theme Designer

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


2 months ago

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


2 months ago

#36 @audrasjb
2 months ago

  • Keywords commit added

Thanks @afercia, 47069.7.diff works fine.
Totally agree with everything shared above. We should work on this early during 5.4 new cycle. Thanks @dinhtungdu for working so much on this ticket 👍

By the way, @afercia’s current patch is ready to go. Re-adding commit keyword.

#37 @afercia
2 months ago

In 46440:

Accessibility: Script Loader: Add jQuery as dependency of admin-bar.

On the front end, themes that don't use jQuery make the admin bar fallback to the non-jQuery implementation. Some important features miss from the non-jQuery admin-bar.js part, for example hoverintent and, most importantly, the touch events for the mobile menu don't work at all.

Enqueueing jQuery is the simplest option for now. For the future, a complete rewriting of admin-bar.js in plain JavaScript is highly recommended.

See #47069.

#38 @azaozz
2 months ago

Sorry I couldn't review this earlier (been sick for a while). The reason the admin-bar doesn't have jQuery as dependency is that (at the time) some themes were/are adding their own versions of jQuery on the front-end, and it becomes "big mess" when we also add the version from core.

The "mess" mostly relates to jQuery plugins that may pick and use the wrong jQuery version, and fail.

That's why the admin bar js has a fall-back mode to work without jQuery. Ideally this mode should be fixed and not jQuery added in all cases.

Alternatively this can be fixed in the affected themes.

Last edited 2 months ago by azaozz (previous) (diff)

#39 @afercia
2 months ago

That's why the admin bar js has a fall-back mode to work without jQuery. Ideally this mode should be fixed

I think we all agree on this point. That's the reason why all the prevalent opinion on this ticket is that the best option is to refactor admin-bar.js in plain JavaScript and adding the "enhancements" that currently work only in the jQuery part. Most notably: hoverintent and the admin bar menu touch events on mobile. There is an initial patch from @dinhtungdu that hasn't been reviewed yet. I also have to note the complete lack of feedback from the toolbar component maintainers for the entire life of this ticket. This isn't greatly helpful.

Regardless, worth noting again that with Twenty Twenty the admin bar on the front end will be broken, much like it has been broken with twenty Nineteen for a year or so and with all the themes that don't use jQuery since forever. Honestly, this doesn't seem the best service WordPress can offer to users.

#40 @dinhtungdu
2 months ago

@afercia I didn't notice that we have a toolbar component. Can I promote myself to become a maintainer?

#41 @afercia
2 months ago

@dinhtungdu sure you can! Best place to ask is next weekly dev chat on Slack on Wednesday at 20:00 UTC :)

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


8 weeks ago

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


7 weeks ago

#44 @audrasjb
7 weeks ago

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

RC1 is approaching. Closing this ticket as fixed for milestone 5.3.

#45 @azaozz
7 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Looking at the possible problems with always enqueueing the core version of jQuery in all themes, this should be reverted for now. If it is needed for a particular theme, lets add it in the theme.

#46 @afercia
7 weeks ago

  • Keywords needs-patch added; has-patch commit removed
  • Milestone changed from 5.3 to Future Release

Agreed to revert this change for now, as there are valid concerns for potential jQuery versions conflicts.

Not sure this should be a themes responsibility though. Let's revisit a best approach in the next release cycle, preferably refactoring admin-bar.js in plain JavaScript so that all the functionalities are available regardless of jQuery.

#47 @afercia
7 weeks ago

In 46550:

Accessibility: Script Loader: Remove jQuery as dependency of admin-bar after [46440].

A better approach needs to be explored, as there are valid concerns for potential conflicts between different jQuery versions added by themes or plugins.

See #47069.

#48 @audrasjb
7 weeks ago

  • Keywords early added

#49 @azaozz
7 weeks ago

  • Milestone changed from Future Release to 5.3.1

It would be great if work continues to make the admin bar work without using jQuery. 47069.6.diff looks good but seems to need a bit more work and more testing.

Setting this tentatively for 5.3.1 as it would be a nice bug fix for some themes.

@afercia
7 weeks ago

#50 follow-up: @dinhtungdu
7 weeks ago

@azaozz Yeah, 47069.6.diff definitely needs more testing. I'm more than welcome to receive feedback and provide fixes for it :)

#51 follow-up: @afercia
7 weeks ago

  • Keywords commit dev-feedback has-patch added; needs-patch removed

[46550] brought in some unrelated, unintended CSS changes. They pertain to #47477 which is unrelated to this ticket.

47069.8.diff partially reverts [46550] removing all the CSS changes. Note: I'd still recommend to commit the CSS changes, see https://core.trac.wordpress.org/ticket/47477#comment:67 but they should be agreed on and anyway pertain to #47477 :)

Being in Release Candidate period, 47069.8.diff needs a sign-off from another committer.

Last edited 7 weeks ago by afercia (previous) (diff)

#52 in reply to: ↑ 51 @azaozz
7 weeks ago

Replying to afercia:

47069.8.diff looks good, +1 to revert the accidental changes during RC.

#53 @afercia
7 weeks ago

In 46559:

Script Loader: Partially revert [46550] as it brought in unrelated CSS changes.

[46550] was meant to revert [46440] but it also merged some unrelated CSS changes.

See #47069.

#54 @afercia
7 weeks ago

  • Keywords needs-patch added; commit dev-feedback has-patch removed

#55 in reply to: ↑ 50 ; follow-up: @azaozz
7 weeks ago

Replying to dinhtungdu:

Great! :)

Some initial thoughts:

  • Perhaps it would be good to "extract" the native hoverIntent into a separate file that can replace the jQuery based version? It's not necessary to be an "exact copy" of how it works with jQuery, just needs to have the same or very similar functionality.

#56 in reply to: ↑ 55 @dinhtungdu
7 weeks ago

Replying to azaozz:

  • Replacing hoverIntent: I agree! Only 3 files are using hoverIntent including admin-bar.js. I will work on replacing it this week.
  • scrollIntoView is an experimental API, and many browsers don't have support for smooth behavior including Safari and iOS Safari/Chrome. So we should use scrollToTop() for now.

#57 @azaozz
7 weeks ago

many browsers don't have support for smooth behavior including Safari and iOS...

Yeah, I know. Safari is the new IE6! :) Eventually it will get added there. Doing full-page animations in js is quite CPU-intensive and may have the opposite effect, get so ridiculously choppy.

Thinking it would be a better option to leave that for the native browser implementation, then when this is added to Safari, it will "just work" there too.

Only 3 files are using hoverIntent..

Yes, but one of them is common.js that gets loaded everywhere in wp-admin. So hoverIntent is available everywhere too.

Last edited 7 weeks ago by azaozz (previous) (diff)

@dinhtungdu
7 weeks ago

Replace jQuery hoverIntent with vanilla JS hoverintent. Use scrollIntoView for smooth scrolling.

#58 @azaozz
2 weeks ago

Looking at 47069.9.diff, works pretty well in the admin bar.

We cannot remove the old jQuery hoverIntent.js. It is likely used in a lot of plugins and expected to always be there as it is loaded on all wp-admin screens. The options there are:

  1. Add a jQuery wrapper for the new hoverintent.js (and load it with the old hoverintent script-handle so it stays compatible).
  2. Keep the old hoverIntent.js and add the new, no-dependencies one.

Thinking that perhaps the second makes more sense as there will be no eventual compatibility issues from adding a wrapper.

@azaozz
2 weeks ago

#59 @azaozz
2 weeks ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

In 47069.10.diff: Virtually the same as 47069.9.diff except it doesn't remove the old jQuery based hoverIntent.js.

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


2 weeks ago

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


9 hours ago

Note: See TracTickets for help on using tickets.