Rewrite jQuery .show() and .hide()

There are some strong choices in this patch that may not appeal to all, so I'm holding off on comments/docs in the code until there's a direction we feel safe to move forward with.

This overrides the jQuery hide, show, fadeIn and toggle functions. The implementation of these functions are part of a newly introduced drupalUtils object, which is inspired by #3179551: Provide no-library equivalents of common/useful jQuery functions, an issue with the goal of providing vanilla versions of frequently used jQuery functions that don't have 1:1 equivalents.

There are several additional changes as some code & tests make assumptions about what hide() and show() do, so they are explicitly looking for style="display: none;". This suggests that this may require some nontrivial refactoring for many contrib modules, and not sure how much is reasonable to expect.

Also note that the new versions are only called if jQuery hide()/show() are called with no arguments. At the moment, additional args will just call the original jQuery version and trigger a deprecation message. If theres a demonstrated need to accept these additional args in the replacements that can happen, but wanted a discussion before directing too much time to such a thing.

As said in the bof, to me we don't need a dedicated JS API for that, we could enforce the CSS rules but on the JS side to me what's necessary
is for people to only do:

element.hidden = booleanValue;

don't even need a polyfill for IE https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/hidden

I agree with #4 that we should promote using a toggle of the hidden attribute. But, for backwards compatibility, I could see it beneficial to reduce major code refactoring.

EDIT: so after re-reading, I think we just need to drop the drupalUtils function and let the jQuery function replacement do its job.

1. +++ b/core/core.libraries.yml
   @@ -50,6 +50,14 @@ drupalSettings:
   +drupalUtils:
   +  version: VERSION
   +  js:
   +    misc/drupalUtils.js: {}
   +  css:
   +    component:
   +      misc/drupalUtils.css: {}
   +
   

   Per #4 I don't believe we need to have a JS API and folks can just set the element properly, but still have the other library for jQuery support.

2. +++ b/core/misc/drupalUtils.css
   @@ -0,0 +1,8 @@
   +/**
   + * @file
   + * Styles used by drupalUtils.js.
   + */
   +
   +[hidden] {
   +  display: none !important;
   +}
   

   We shouldn't need this CSS rule as browsers will hide it from view.

   Are we worried some browsers do not properly implement this? Based on https://caniuse.com/?search=hidden, a browser would need to be ~10 years old to need this CSS.

3. +++ b/core/misc/jquery.drupalUtils.shim.es6.js
   @@ -0,0 +1,68 @@
   +        $$.hide(item, ...args);
   ...
   +        $$.show(item);
   ...
   +          $$.toggle(item, ...args);
   ...
   +  $.fn.fadeIn = function (...args) {
   +    this.css('display', 'none');
   +    this.removeAttr('hidden');
   +
   +    return originalFadeIn.apply(this, args);
   

   We should just replace this with directly calls to attribute manipulation.

   We even do that at the last example, without the utility.

4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceFileTest.php
   @@ -75,9 +75,10 @@ public function testMediaFileSource() {
   +    $log_message = $page->find('css', '.field--name-revision-log-message');
   +    $this->assertTrue($log_message->hasAttribute('hidden'));
   ...
   +    $this->assertFalse($log_message->hasAttribute('hidden'));
   

   Does the WebDriver assertion support checking visibility?

5. +++ b/core/modules/system/tests/modules/js_webassert_test/src/Form/JsWebAssertTestForm.php
   index 0000000000..fe964488ba
   --- /dev/null
   
   --- /dev/null
   +++ b/core/themes/stable/css/core/drupalUtils.css
   
   +++ b/core/themes/stable/css/core/drupalUtils.css
   +++ b/core/themes/stable/css/core/drupalUtils.css
   @@ -0,0 +1,8 @@
   
   @@ -0,0 +1,8 @@
   +/**
   + * @file
   
   +++ b/core/themes/stable/stable.info.yml
   @@ -80,6 +80,11 @@ libraries-override:
   +  core/drupalUtils:
   +    css:
   +      component:
   +        misc/drupalUtils.css: css/core/drupalUtils.css
   

   A benefit of not using CSS is not having to copy it into stable and have these overrides

we could use toogleattribute directly: https://developer.mozilla.org/en-US/docs/Web/API/Element/toggleAttribute (with polyfill for IE that we can load with a "nomodule" attribute on the script tag)

Ah! I totally missed toggleAttribute 🤦🏼‍♂️ I knew there had to be something

maybe hidden attribute isn't a good replacement for this, as it is said in the mdn docs: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden setting a display: value in CSS overrides the hidden attribute, so it's not reliable.

So jQuery simplified this method a while ago and now it only checks the css display value. Another thing it does it optimize this when called with multiple elements (to change a list of elements it's better for performance to do that in 2 steps, one step reads from the DOM, the other write to the DOM).

As long as most our use of show/hide target a single element we'll be fine but if we have a collection of elements to show/hide it might get us in performance troubles.

one thing we could do it create a new attribute data-hidden that essentially do display:none !important but i'm not too sure we should change the meaning of the hidden attribute directly like this.

lots of failures using just hidden :)

I got same results in Chrome Firefox and Safari for that codepen.

https://codepen.io/finnsky/pen/eYxgLQO

`hidden` actually hide div with class CSS specificity.
But failure in cascade selector and obviously when !important.

Hi all!

From my point of view we should not optionally use these functions as such. And those scripts that continue to be used are given a warning. Perhaps less strict. So that these warnings can pass functional tests.

For reliable hiding, I added [data-hidden] as suggested.

And reworked layout-builder.js for an example of work.

I also think that .show() and .hide() are not the only functions that can be handled this way. So I renamed the script.

I need a review of the approach here

Seems to have open threads but appears to have a failure in 5277, can the other MR be closed or hidden?

To test replace
"lint:core-js-passing": "node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json .",
by
"lint:core-js-passing": "node ./node_modules/eslint/bin/eslint.js --config=.eslintrc.passing.json .",

and run yarn lint:core-js-passing

Hiding patches and old MRs for clarity

Current MR 5277 appears to have a test failure and may need a rebase (maybe it fixes the test)