Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#47069 closed defect (bug) (fixed)

The admin bar on the front end has reduced functionality and bugs when jQuery not being used

Reported by: afercia's profile afercia Owned by: audrasjb's profile audrasjb
Milestone: 5.3.1 Priority: normal
Severity: normal Version: 5.0
Component: Toolbar Keywords: has-screenshots early has-patch needs-testing
Focuses: accessibility, javascript Cc:

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

Twenty Seventeen.png (62.9 KB) - added by afercia 5 years ago.
Twenty Seventeen: pressing Enter opens the menu
Twenty Nineteen.png (76.5 KB) - added by afercia 5 years 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 5 years ago.
Mobile: tapping the menu triggers navigation instead of opening the menu dropdown
missing search icon.png (85.8 KB) - added by afercia 5 years ago.
Missing icon within the search input
47069.diff (584 bytes) - added by audrasjb 5 years ago.
First step: enqueue jQuery if the Admin Bar is displayed
admin-bar-hoverintent.diff (4.3 KB) - added by afercia 5 years ago.
admin-bar-no-jquery-wip.diff (13.6 KB) - added by afercia 5 years ago.
47069.2.diff (29.7 KB) - added by dinhtungdu 5 years ago.
06dac1be2b73076d44b0364c30d5e5c9.gif (229.0 KB) - added by audrasjb 5 years ago.
Testing patch and everything works as expected
47069.3.diff (29.0 KB) - added by dinhtungdu 5 years ago.
47069.4.diff (28.7 KB) - added by dinhtungdu 5 years ago.
47069.5.diff (28.8 KB) - added by dinhtungdu 5 years ago.
47069.6.diff (29.2 KB) - added by dinhtungdu 5 years ago.
47069.7.diff (705 bytes) - added by afercia 4 years ago.
47069.8.diff (10.2 KB) - added by afercia 4 years ago.
47069.9.diff (33.0 KB) - added by dinhtungdu 4 years ago.
Replace jQuery hoverIntent with vanilla JS hoverintent. Use scrollIntoView for smooth scrolling.
47069.10.diff (28.7 KB) - added by azaozz 4 years ago.
47069.11.diff (32.4 KB) - added by dinhtungdu 4 years ago.
47069.12.diff (15.5 KB) - added by azaozz 4 years ago.
47069.13.diff (16.5 KB) - added by dinhtungdu 4 years ago.
47069.14.diff (16.4 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (110)

@afercia
5 years ago

Twenty Seventeen: pressing Enter opens the menu

@afercia
5 years ago

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

@afercia
5 years ago

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

@afercia
5 years ago

Missing icon within the search input

#1 @afercia
5 years ago

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

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

  • Description modified (diff)

#5 @desrosj
5 years ago

  • Keywords needs-patch has-screenshots added

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


5 years ago

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

First step: enqueue jQuery if the Admin Bar is displayed

#8 @audrasjb
5 years ago

  • Keywords has-patch added; needs-patch removed

#9 @audrasjb
5 years ago

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

#10 @afercia
5 years 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.


5 years ago

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


5 years ago

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


5 years ago

#14 @westonruter
5 years 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
5 years 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.


5 years ago

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

  • Description modified (diff)

#19 @afercia
5 years ago

  • Description modified (diff)

#20 @afercia
5 years 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 5 years ago by afercia (previous) (diff)

@dinhtungdu
5 years ago

#21 @dinhtungdu
5 years 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 5 years ago by dinhtungdu (previous) (diff)

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


5 years ago

#23 @audrasjb
5 years ago

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

#24 @audrasjb
5 years ago

  • Keywords needs-testing removed

@audrasjb
5 years ago

Testing patch and everything works as expected

#25 @audrasjb
5 years ago

  • Keywords needs-refresh added

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

Last edited 5 years ago by audrasjb (previous) (diff)

@dinhtungdu
5 years ago

@dinhtungdu
5 years ago

@dinhtungdu
5 years ago

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


5 years ago

@dinhtungdu
5 years ago

#28 @dinhtungdu
5 years 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.


5 years ago

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


5 years ago

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


4 years ago

#32 @audrasjb
4 years 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
4 years ago

#33 @afercia
4 years 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.


4 years ago

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


4 years ago

#36 @audrasjb
4 years 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
4 years 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
4 years 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 4 years ago by azaozz (previous) (diff)

#39 @afercia
4 years 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
4 years ago

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

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


4 years ago

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


4 years ago

#44 @audrasjb
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

  • Keywords early added

#49 @azaozz
4 years 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
4 years ago

#50 follow-up: @dinhtungdu
4 years 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
4 years 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 4 years ago by afercia (previous) (diff)

#52 in reply to: ↑ 51 @azaozz
4 years ago

Replying to afercia:

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

#53 @afercia
4 years 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
4 years ago

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

#55 in reply to: ↑ 50 ; follow-up: @azaozz
4 years 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
4 years 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
4 years 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 4 years ago by azaozz (previous) (diff)

@dinhtungdu
4 years ago

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

#58 @azaozz
4 years 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
4 years ago

#59 @azaozz
4 years 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.


4 years ago

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


4 years ago

#62 follow-up: @dinhtungdu
4 years ago

@azaozz I agree that we need to keep the jQuery hoverIntent, but I think we should use the vanilla JS version for menu and revision to prevent loading 2 versions of hoverIntent in the admin dashboard.

Update: 47069.11.diff use vanilla JS hoverintent for admin menu and revision.

Last edited 4 years ago by dinhtungdu (previous) (diff)

@dinhtungdu
4 years ago

#63 @afercia
4 years ago

Keep the old hoverIntent.js and add the new, no-dependencies one.

Seems the most reasonable approach to me. It would be nice there was a deprecation policy for this kind of things :)

#64 in reply to: ↑ 62 ; follow-up: @azaozz
4 years ago

Replying to dinhtungdu:

@azaozz I agree that we need to keep the jQuery hoverIntent, but I think we should use the vanilla JS version for menu and revision to prevent loading 2 versions of hoverIntent in the admin dashboard.

Not sure that's "wise" :)

As I mentioned before:

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.

Currently the jQuery hoverIntent is loaded everywhere in wp-admin. Some plugins probably assume it is "always there". For best back-compat we cannot stop loading it...

Ideally we should make a jQuery "wrapper" for the new hoverintent.js (and load it with the old hoverintent script-handle so it stays compatible). This doesn't seem like a particularly complex task, such wrapper may already exist somewhere.

Given the short time left for the 5.3.1 milestone, thinking for now we should continue to load both hoverintent versions, and look at making a wrapper in 5.4.

@dinhtungdu could you test/confirm 47069.10.diff works as expected please :)

#65 in reply to: ↑ 64 @dinhtungdu
4 years ago

Replying to azaozz:

Currently the jQuery hoverIntent is loaded everywhere in wp-admin. Some plugins probably assume it is "always there". For best back-compat we cannot stop loading it...

I see :)

@dinhtungdu could you test/confirm 47069.10.diff works as expected please :)

I tested it and I can confirm that it works :)

#66 @azaozz
4 years ago

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

In 46872:

Fix the admin toolbar js when jQuery is not present and replace the jQuery based hoverIntent.js with a native implementation. Introduces the "hoverintent" (no dependencies) package.

Props dinhtungdu, audrasjb, azaozz.
Fixes #47069.

#67 @azaozz
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.3 branch.

#68 @azaozz
4 years ago

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

In 46873:

Fix the admin toolbar js when jQuery is not present and replace the jQuery based hoverIntent.js with a native implementation. Introduces the "hoverintent" (no dependencies) package.

Props dinhtungdu, audrasjb, azaozz.
Merges [46872] to the 5.3 branch.
Fixes #47069.

#69 @SergeyBiryukov
4 years ago

In 46874:

Coding Standards: Fix WPCS and JSHint issues in [46872].

See #47069.

#70 @SergeyBiryukov
4 years ago

In 46875:

Coding Standards: Fix WPCS and JSHint issues in [46872].

Merges [46874] to the 5.3 branch.
See #47069.

#71 @azaozz
4 years ago

  • Summary changed from Twenty Nineteen: the admin bar on the front end has reduced functionalities and bugs due to jQuery not being used to The admin bar on the front end has reduced functionality and bugs when jQuery not being used

#72 @azaozz
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There seem to be some errors in the "jQuery free" admin-bar.js. Caused by the way jQuery silences errors when a particular node doesn't exist. For example: $( '#some-element' ).on( 'click', ... ) will not throw error when a node with that ID doesn't exist but the equivalent document.getElementById( 'some-element' ).addEventListener() will.

Patch coming up.

Last edited 4 years ago by azaozz (previous) (diff)

#73 follow-up: @dinhtungdu
4 years ago

@azaozz I see. I didn't test cases when users filter the content of the admin-bar 😬.

I'm thinking about checking variables before attaching events to them but it doesn't seem to be the best solution.

Update: the logout menu is not checked before using.

Last edited 4 years ago by dinhtungdu (previous) (diff)

#74 in reply to: ↑ 73 @azaozz
4 years ago

Replying to dinhtungdu:

I'm thinking about checking variables before attaching events to them but it doesn't seem to be the best solution.

Yeah, it's either that or perhaps wrapping the whole thing in a try {} catch () {}. Checking node existence seems cleaner, perhaps? :)

Also not really sure about the Element.prototype.matches() polyfill: https://developer.mozilla.org/en-US/docs/Web/API/Element/matches. It's not a bad idea, but might bring some compatibility issues eventually?

This will run on the front-end for logged-in users. Generally the browsers they use should conform to the minimal browser support for wp-admin (IE11, newer Edge, Chromium and Firefox) but having some "error silencing" for old browsers (to emulate jQuery behaviour) may be a good idea.

For example looking at the browser compat table at the bottom of https://developer.mozilla.org/en-US/docs/Web/API/Element, there's no window.Element for old Edge and Chrome for Android.

Last edited 4 years ago by azaozz (previous) (diff)

@azaozz
4 years ago

#75 follow-up: @azaozz
4 years ago

  • Keywords needs-testing added

In 47069.12.diff:

  • Silence errors (similarly to jQuery) when adding event listeners.
  • Add "fallbacks" in functions that use element.classList.
  • Some coding standards and readability fixes.

@dinhtungdu sorry for the late patch but could you please have a look at this asap so it can be added tomorrow :)

#76 in reply to: ↑ 75 ; follow-up: @dinhtungdu
4 years ago

Replying to azaozz:

@dinhtungdu sorry for the late patch but could you please have a look at this asap so it can be added tomorrow :)

@azaozz It's 9 am here so it's no problem :D. The patch works great for me. I have some questions about the code:

  • Why do we need to check for window.Element exist or not?
  • The comment for hasClass function is not updated.
  • The indentation of some functions is too deep for me, should we try to fix them with early return style? (I can patch). IMO, the small indentation level increases readability.
Last edited 4 years ago by dinhtungdu (previous) (diff)

@dinhtungdu
4 years ago

#77 @dinhtungdu
4 years ago

In 47069.13.diff :

  • Change version in comment to 5.3.1.
  • Update documentation for hasClass function.

#78 in reply to: ↑ 76 @azaozz
4 years ago

Replying to dinhtungdu:

@azaozz It's 9 am here so it's no problem :D. The patch works great for me. I have some questions about the code:

Ah, that's great! :)

Why do we need to check for window.Element exist or not?

Yeah, probably not needed especially after it tests for querySelectorAll at the top (added it mostly for completeness after looking at the (big) compatibility table at https://developer.mozilla.org/en-US/docs/Web/API/Element).

The indentation of some functions is too deep for me, should we try to fix them with early return style?

Sure, lets reduce it. Removing the window.Element check and returning early in few functions should reduce indentation :)

Patch coming up.

@azaozz
4 years ago

#79 follow-up: @azaozz
4 years ago

  • Keywords 2nd-opinion removed

In 47069.14.diff: remove check for window.Element and reduce indentation in few places by returning early.

This seems ready, @dinhtungdu could you have another look please and lets add that :)

#80 in reply to: ↑ 79 @westi
4 years ago

Replying to azaozz:

In 47069.14.diff: remove check for window.Element and reduce indentation in few places by returning early.

This seems ready, @dinhtungdu could you have another look please and lets add that :)

This works well on my test site with TwentySeventeen which was previously showing errors.

#81 follow-up: @dinhtungdu
4 years ago

@azaozz Tested and everything works as expected.

@westi Do you remember what those errors are?

#82 in reply to: ↑ 81 ; follow-up: @westi
4 years ago

Replying to dinhtungdu:

@azaozz Tested and everything works as expected.

@westi Do you remember what those errors are?

TypeError: adminBarLogout is nulladmin-bar.js:131:3
    <anonymous> .../wp-includes/js/admin-bar.js?ver=5.3.1-RC1-46877:131

Running the 5.3 Branch from svn with SCRIPT_DEBUG enabled

Last edited 4 years ago by westi (previous) (diff)

#83 @azaozz
4 years ago

@westi @dinhtungdu thanks for looking at such short notice! Lets add this :)

#84 @azaozz
4 years ago

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

In 46883:

Fixes in admin-bar.js:

  • Silence errors when a node doesn't exist similarly to jQuery.
  • Add "feature testing" and fallbacks for old browsers as this may run on the front-end.
  • Improve inline docs.

Props dinhtungdu, azaozz.
Fixes #47069.

#85 in reply to: ↑ 82 @dinhtungdu
4 years ago

Replying to westi:

TypeError: adminBarLogout is nulladmin-bar.js:131:3
    <anonymous> .../wp-includes/js/admin-bar.js?ver=5.3.1-RC1-46877:131

Thanks! That's my mistake and azaozz fixed it.

Last edited 4 years ago by dinhtungdu (previous) (diff)

#86 @azaozz
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for merging to the 5.3 branch.

#87 @azaozz
4 years ago

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

In 46884:

Fixes in admin-bar.js:

  • Silence errors when a node doesn't exist similarly to jQuery.
  • Add "feature testing" and fallbacks for old browsers as this may run on the front-end.
  • Improve inline docs.

Props dinhtungdu, azaozz.
Merges [46883] to the 5.3 branch.
Fixes #47069.

#88 @aduth
4 years ago

With the changes in r46872 (notably to the package-lock.json file), I see local changes on a fresh clone of wordpress-develop after running npm install. Unsure if this differs based on the version of npm used, but in my case I'm using the latest version (6.13.4).

At least on observation of the diff, it appears to be a simple shift via alphabetization.

diff --git a/package-lock.json b/package-lock.json
index bf9b85069a..89bf380ace 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -10731,6 +10731,11 @@
 			"integrity": "sha512-7T/BxH19zbcCTa8XkMlbK5lTo1WtgkFi3GvdWEyNuc4Vex7/9Dqbnpsf4JMydcfj9HCg4zUWFTL3Za6lapg5/w==",
 			"dev": true
 		},
+		"hoverintent": {
+			"version": "2.2.1",
+			"resolved": "https://registry.npmjs.org/hoverintent/-/hoverintent-2.2.1.tgz",
+			"integrity": "sha512-VyU54L1xW5rSqpsv/LJ6ecymGXsXXeGs9iVEKot4kKBCq5UodSAuy3DqX686LZxEpaMEfeCHPu4LndsMX5Q9eQ=="
+		},
 		"hpack.js": {
 			"version": "2.1.6",
 			"resolved": "https://registry.npmjs.org/hpack.js/-/hpack.js-2.1.6.tgz",
@@ -13217,11 +13222,6 @@
 				"jquery": ">=1.7"
 			}
 		},
-		"hoverintent": {
-			"version": "2.2.1",
-			"resolved": "https://registry.npmjs.org/hoverintent/-/hoverintent-2.2.1.tgz",
-			"integrity": "sha512-VyU54L1xW5rSqpsv/LJ6ecymGXsXXeGs9iVEKot4kKBCq5UodSAuy3DqX686LZxEpaMEfeCHPu4LndsMX5Q9eQ=="
-		},
 		"jquery-hoverintent": {
 			"version": "1.8.3",
 			"resolved": "https://registry.npmjs.org/jquery-hoverintent/-/jquery-hoverintent-1.8.3.tgz",

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


3 years ago

Note: See TracTickets for help on using tickets.