Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways

I'm in with this issue.
We could have the outside_in "Settings tray" module use a stand alone library.
Something like http://lab.hakim.se/meny/?p=right will be super. as it will be theme, JS, CSS Independent.

I think a library like https://github.com/hakimel/meny support many potions.

Meny can be positioned on any side of the screen:

• top
• right
• bottom
• left

Ok then .. Just Independent and the proper APIs.
Back to your track @markcarver .......
I think it's the right one.

Rewarded :)

@markcarver thanks for filling the issue.

Right now I don't think Bootstrap works with the core dialog system which Settings Tray is based off of. #2831237: Bootstrap modal does not work well with jQuery UI dialog

I don't think we should abstract Settings tray around from the core dialog system. So I think that should be solved first because for Bootstrap to work with this module it will have to work with the core dialog system.

It's a valid issue. This is similar to what droplet wanted from the start, see #2786459-63: "Offcanvas" tray should be using the existing dialog system and #2786459-65: "Offcanvas" tray should be using the existing dialog system, and I agree with that.

I did push to use dialogs because I wanted to avoid adding more code for an experimental module before we tried it out with the current stuff available in core.

@nod_ think you meant link to another issue instead of same 1 twice ;)

My opinion is that tray with this module works well with the existing dialog system. If want to abstract it away from JqueryUI that should happen on the dialog library level not 1 core that uses the library.

This is something that should really be included in a comprehensive re-work of all "front end admin' things which might involve moving off jquery UI for all those things.

Because of that, its JS was quickly created that invokes jQuery UI dialog methods directly rather than abstracting its dialog invocations using Drupal.dialog (which is an API we actually already have).

Yes, that's true and I *think* that is within the scope of #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it, but @tedbow should confirm that.

There are 2 javascript files:

offcanvas.js

This will be added to the regular system under /core/misc/dialog in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
I don't think this needs to be abstracted from jQuery UI because the rest of the files under /core/misc/dialog are not. For instance multiple calls to $element.dialog(). So if I theme like Bootstrap is going to replace the libraries drupal.dialog and drupal.dialog.ajax like it appears to be in \Drupal\bootstrap\Plugin\Alter\LibraryInfo::alter it will also need provide the "offcanvas" functionality.

outside_in.js

This deals with the edit mode. I haven't had a chance to look at how to see if this might be abstracted.

right now I am focused #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it .
I changed title of this issue to reflect that I think should only deal with outside_in.js if anything is going to be abstracted away.

#7

I did push to use dialogs because I wanted to avoid adding more code for an experimental module before we tried it out with the current stuff available in core.

I think this makes a lot of sense. Trying to spin out this code to be totally independent seems like a pretty large task right now. Since we're already dependent on JQuery UI for the dialog system, it makes sense for this module to be as well. I'll mark this as postponed for the time being; this should probably deserve another look if we get to refactoring out the dependency on JQuery UI dialogs.

This module is experimental but it needs not to be soon.

The JavaScript in this module is dependent on jQueryUI in the same way the existing core dialog system is dependent on jQueryUI.

I don't think the decision to abstract the dialog system from jQueryUI, if it is good idea, should be made on 1 usage dialog portion of the dialog system.

Right now are dialog system is tied to jQueryUI Dialog to the point where

you can use the data-dialog-options attribute - which accepts encoded json of any valid option accepted by jQuery UI Dialog.

@see Change record: Ajax commands for opening and closing Dialogs and generic Dialog Controller added to core

There are other parts of core that depend on this also, discardDialog in core/modules/quickedit/js/views/AppView.js usage of closeOnEscape.

If this is bad idea it should tackled on the dialog system level not this module. It has larger BC implications.

In bootstrap theme to abstract the dialog system away from JQuery UI it has to replace

1. ajax.js
2. dialog.js
3. dialog.ajax.js

In other words it completely replaces the JS for dialogs. Currently in bootstrap on "modal" dialogType works not "dialog". Also data-dialog-options that are not respected.

I think if I theme does this it is the themes responsibility to also replace any JS that integrates or becomes part dialog system in minor version of Drupal 8.

I want to circle back to the original request in this issue, to make settings tray independent from jQueryUI components. In the contrib theme, Bootstrap, jQueryUI is not available this causing this module to not behave correctly.

We had originally rolled our own solution to display the settings tray but opted to utilize the existing jQueryUI dialog system for simplicity and consistency across core JavaScript.

If we continue to postpone other issues on the argument that we should further abstract settings tray away from its dependency on the jQueryUI dialog system then we run the risk of this module being removed, missing it’s stable target.

Additionally if the issue is that core has not done a sufficient job in abstracting away the dependency on jQueryUI, then this is not the place for that argument. There should be an additional issue created outlining the desire to sufficiently abstract core’s JavaScript away from the hard dependencies currently in place.

Where is it formalized that Drupal's dialog can work without jQuery.ui? The libraries definition requires it:

drupal.dialog:
  version: VERSION
  js:
    misc/dialog/dialog.js: {}
    misc/dialog/dialog.position.js: {}
    misc/dialog/dialog.jquery-ui.js: {}
  dependencies:
    - core/jquery
    - core/drupal
    - core/drupalSettings
    - core/drupal.debounce
    - core/drupal.displace
    - core/jquery.ui.dialog

And almost all the dialog JS in core/misc/dialog calls $(...).dialog as well:

http://cgit.drupalcode.org/drupal/tree/core/misc/dialog/dialog.js#n84
http://cgit.drupalcode.org/drupal/tree/core/misc/dialog/dialog.ajax.js#n208
http://cgit.drupalcode.org/drupal/tree/core/misc/dialog/dialog.jquery-ui...
http://cgit.drupalcode.org/drupal/tree/core/misc/dialog/dialog.position....

What is outside_in doing wrong that core dialog isn't already doing? It seems like the jQuery UI dependency in core is quite explicit, and I see no documentation or code that would enable developers to override that dependency.

@markcarver I'm new to this issue so I posted #20 to understand the problem better, thanks for posting more detail. Bootstrap has a valid reason for overriding the core modal, and what I would hope is that whatever solution maintainers come up with for the issue makes it easier to do what Bootstrap is doing, not harder. That said, if just making Outside In call Drupal.dialog is simple, and can help push things forward, I think it's a good plan! I think working on minimal-effort patch is a good next-step.

I'm sorry for dog-piling the issue, I actually worried about that and want to make it transparent that while I work at Acquia, my goals/deadlines are primarily coming from my role as a contrib-maintainer. :-) I maintain Moderation Note and Moderation Sidebar, which are both stuck in development-release-limbo as I use the offcanvas dialog and have to depend on Outside In. As a result, I'd love to see the offcanvas dialog moved into /core/misc so I can do stable releases of my modules. Finding a resolution to this issue is a requirement for that, so I'm here to help!

@markcarver

I raised many valid points as to why this experimental module's JS is not abstract enough for contrib and pointed out the very specific problems and offered many different solution (several times).

You have raised valid points. I was also trying to make valid points. Maybe I failed. :( I did start to work on patch after reading your points in #19 but hadn't commented back to that effect. sorry.

If that "runs the risk of this module being removed", then good. The policy is doing part of its job: ensuring that we don't release unstable code (or code that can easily break when certain conditions aren't met).

While hope it doesn't get removed I do agree with you that policy is there for a good reason.

This all being said, when is this supposed "deadline"?

The deadline is simply the deadline for experimental modules to get into core.

From https://www.drupal.org/core/experimental#requirements

A timeframe for completing the remaining work (typically two minor releases),

It maybe stated elsewhere but this is what I found offhand. I am not sure if it is stated in more concrete terms elsewhere.

Settings Tray was introduced in 8.2 so by this standard would have to get stable 8.4. While stable doesn't mean no issues an issue like this I think would need to be resolved one way or the other by then.

Re @samuel.mortenson in #22

I would hope is that whatever solution maintainers come up with for the issue makes it easier to do what Bootstrap is doing, not harder. That said, if just making Outside In call Drupal.dialog is simple, and can help push things forward, I think it's a good plan! I think working on minimal-effort patch is a good next-step.

Ok sounds like a good next step.

re @markcarver

However, it is clear that I will have to look into abstracting this myself because my comments do no seem to be making much of an impact.

I will give it a shot.

Here is patch. I started working on after reading @markcarver's comment #19

@markcarver if you have time let me know if this direction you were thinking

Here is what the patch does:

1. Replace calls to $element.dialog('widget'); with dialog.container();. In JQuery Dialog case the $element is contained within a surrounding div. If another library doesn't have this they can probably just return $element.
2. Added container() to Drupal.dialog
3. Replaced calls $element.dialog('option', $options) with <code>dialog.options($options, newoptions) These options will be jQuery dialog options but this how the API is explicitly setup to work.
4. Added options() to Drupal.dialog
5. Moved to handleDialogResize() to dialog.handleDialogResize(). This has JQuery UI specific logic for handling heights.

I have been testing against a clone bootstrap: https://github.com/tedbow/bootstrap-theme/tree/8.3-off-canvas

It contains changes

1. Fix I talked about here #2831237-5: Bootstrap modal does not work well with jQuery UI dialog. That issue doesn't provide a patch I was pretty sure it wasn't the best solution
2. Added dialog.handleDialogResize() which does nothing because think if bootstrap modal would need anything it would be bootstrap specific, as probably with any other dialog system.
3. Added dialog.options() which does nothing because bootstrap library(just dialog extended?) doesn't support jQuery UI Dialog options. I think if non-jQueryUI dialog system wanted to support the options it would it's responsibility to translate the options sent to dialog creation and options() to the relevant options for the specific library.

With the minimal changes above bootstrap will work Settings Tray in the sense that "Quick Edit" links and the forms will open in the bootstrap modal, and they work. Also the interaction provided in outside_in.js as far as making block clickable also works.

I think for bootstrap or another theme that wasn't using jQuery Dialog to get the full non-modal dialog and actual off-canvas/sidebar type interaction there would need to be work on the theme itself. @markcarver, others, does that sound right?

Forgot to mention in #23 that the changes to core/misc/dialog/dialog.js should probably be separate issue. (though would love to just commit here)

Just wanted to add them here while we figure out if this is a good solution.

@see #7

I said No to expose random new APIs to Drupal.dialog. This is still designed for CORE and will not help any of other libs.

the CORE usages have a common issue. What CORE does:
- overrides Drupal.dialog.open and Drupal.dialog.close to extend.
- (
most of them forget to trigger the event inside these functions. Either CORE and Contrib will be lost in control.
e.g.:
- #2559241: Closing an #ajax dialog triggers Javascript errors when scrolling
- the problems still remaining in quickedit module
)

What we should do in 99% cases:
- override the code BETWEEN the events inside above 2 methods:

    function openDialog(settings) {
      settings = $.extend({}, drupalSettings.dialog, options, settings);
      $(window).trigger('dialog:beforecreate', [dialog, $element, settings]);

// HERE
      $element.dialog(settings);
// HERE END
      dialog.open = true;
      $(window).trigger('dialog:aftercreate', [dialog, $element, settings]);
    }

    function closeDialog(value) {
      $(window).trigger('dialog:beforeclose', [dialog, $element]);
// HERE
      $element.dialog('close');
// HERE END
      dialog.returnValue = value;
      dialog.open = false;
      $(window).trigger('dialog:afterclose', [dialog, $element]);
    }

Problem:
Obviously, our API design doesn't allow to override it directly.

1. +++ b/core/misc/dialog/dialog.js
   @@ -74,7 +74,10 @@
   +      container: getContainer,
   

   This should probably be getContainer.

2. +++ b/core/misc/dialog/dialog.js
   @@ -74,7 +74,10 @@
   +      options: setOptions,
   

   This should probably be setOptions.

3. +++ b/core/misc/dialog/dialog.js
   @@ -94,6 +97,38 @@
   +    function getContainer() {
   

   Documentation.

4. +++ b/core/misc/dialog/dialog.js
   @@ -94,6 +97,38 @@
   +    function setOptions($options) {
   

   Documentation.

@drpal thanks for the review.
This patch fixes those issues.

@markcarver #25

Yes, I think this extension of methods should be moved to a separate issue (because it doesn't really pertain to outside_in specifically).

I would say let's leave them in the patch for now till we get consensus on this way.

re @nod_ in #7

It's a valid issue. This is similar to what droplet wanted from the start, see #2786459-63: "Offcanvas" tray should be using the existing dialog system and #2786459-65: "Offcanvas" tray should be using the existing dialog system and I agree with that.

@droplet's comments there refer to not using our current dialog system at all if I read them correctly. After reading @markcarver's comments, who opened this issue, I don't think that is what he is asking for.

@see #10

Because of that, its JS was quickly created that invokes jQuery UI dialog methods directly rather than abstracting its dialog invocations using Drupal.dialog (which is an API we actually already have).

also #17

If this API needs to be extended to wrap more of jQuery UI, then so be it. The primary point of this issue is to not call $().dialog (which won't be loaded in Bootstrap), but rather Drupal specific methods (that will be loaded).

The biggest misconception about "abstracting" JS is that one has to "remove existing functionality/reliance". Wrong.

This module can keep it's dependence on jQuery UI's dialogs.

I have never said it couldn't.

The only thing this module shouldn't be doing is calling $.fn.dialog directly, in any of its JS.

I really don't want to sideline this issue into having this module not use jQuery UI dialogs. This dialogs are working well and practically I don't think has complained about our using them.

For to update call to dialog.container() to dialog.getContainer() in dialog.js.

sorry for not running the tests locally

I'm with @droplet here. We have to find a different solution than adding API to the dialog object. If it's not in the spec, it won't be on our API. That's why I mentioned #2786459-63: "Offcanvas" tray should be using the existing dialog system way back where droplet talks about an off-canvas API that could be overridden more easily, which is kind of what's happening here with trying to fankenstein Drupal.dialog.

@markcarver what's proposed in #27 would work for you?

1. Dialog is a DOM API, not a JS API, it's already available in chrome. It's not like it's just for the fun of it, look at: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog You'll notice that all the positioning and styling is done in CSS only. On our side the polyfill works similarly to https://github.com/GoogleChrome/dialog-polyfill we had to add stuff all over the place to support sizing/placing from js only since CSS-only is useless to place and resize jquery ui dialogs.
2. As the person who created the current Drupal.dialog #1667742-95: Add abstracted dialog to core (resolves accessibility bug) I'm here to tell you it is actually something that mimics the spec. And it's pretty complete if you ask me. Might not be "correct" per the spec, but it's correct enough.
3. Again it's a DOM API exposed to JS. What says it cannot be extended is me.

If more things are needed and need to be exposed => new API that makes use of the Drupal.dialog API. The issue title is pretty clear. It seems we agree that "abstracting" in this case means adding APIs, just not to Drupal.dialog directly.

You know what, I thought I earned more credit than that.

We didn't use a polyfill because they were crap at the time and had a lot of a11y issues, which jqueryui didn't have. the aim was always to have something similar to

Drupal.dialog = function (dialog) {
  dialogPolyfill.registerDialog(dialog);
  return dialog;
};

And replace most/all positioning code with CSS. What I'm saying is add stuff to something else than the dialog API because otherwise it'll be really painful to make it evolve to where I want it to be.

I'll leave it at that and won't change the status since lately started to get some motivation back and this is not helping. In any case I don't agree with the technical direction of the latest patches which go against what I was aiming for regarding Drupal.dialog.

I think should keep this to whether it is ok for off-canvas.js to directly call jQuery UI Dialog only methods.

@nod_ in #7 brought up previous arguments about whether we should be using jQuery UI Dialog at all. I personally don't think we should switch but if so this issue is not about that.

I would like current patch to work but I do see problems with the approach.

Point #1: Practical problem of adding new methods to an existing API

Ok, so I now see some practical problems with the approach I took in the patch so far. This problem is in addition to the problems that @nod_ and @droplet were referring to.(not weighing in on that now)

If as in that patch we add new methods to Drupal.dialog this does introduce another backwards compatibility problem.

For example lets say:

1. Theme X overrides misc/dialog.js to make a small change but the version does actually use jQuery UI Dialogs. Let's say as an example they want to add an additional class to buttonPrimaryClass
2. As 8.4.x is now(without this patch) using this theme would could enable Setting Tray module and it would just work.
3. With this patch Theme X's version of misc/dialog.js would cause a fatal error when using Settings Tray because it did not have the new methods getContainer and setOptions

I am not sure what the BC policy is Javascript things we are saying are API's. This says very little about Javascript: https://www.drupal.org/core/d8-bc-policy

Point #2: Interdependence of Javascript in core Libraries

I think the larger question that this issue points to is the question of whether core guarantees that if you override Javascript files in a Library X but don't change it's public API it guarantees(or even attempts) that another core Library Y that depends Library X will work even if you change the implementation of Library X.

Or in others words should a one library in core only ever interact with another core library through its public API and make no assumptions about the implementation.

I would say that this is not true and it is not how core libraries, at least that ones that deal with dialogs, are currently written.

For instance

1. drupal.dialog.ajax library is dependent on drupal.dialog
2. drupal.dialog provides a public API
3. drupal.dialog uses jQuery UI Dialogs internally
4. drupal.dialog.ajax uses the Public API provided by drupal.dialog
5. drupal.dialog.ajax also makes the assumption that the dialogs produced by drupal.dialog will using jQuery dialog by: using $dialog.dialog('widget'), $dialog.dialog('option' and relying on jQuery Dialog specific classes .ui-dialog-*
6. Because of this if you override drupal.dialog to make it not use jQuery UI dialogs you have to override drupal.dialog.ajax

ln other words drupal.dialog.ajax makes assumptions of how drupal.dialog will create the dialog.

In an ideal world yes, it would great to be able to update drupal.dialog to add new methods to it that would enable drupal.dialog.ajax and any other library to not have to assume jQuery Dialogs are being used. but that would cause other problems such as mentioned in point 1.

Point #3: drupal.off_canvas is following existing pattern

Everything mentioned in point #2 about the relationship between drupal.dialog and drupal.dialog.ajax can be said about the the relationship between drupal.off_canvas and drupal.dialog. Especially the last point in the list:

Because of this if you override drupal.dialog to make it not use jQuery UI dialogs you have to override drupal.off_canvas

As the maintainer of this module I am closing this as works as designed.

Here is why:

1. The original purpose of this issue was to abstract the libraries provided by the Settings Tray module in manner that they interact the drupal.dialog through only the API provided by that library and not make assumptions that the dialogs are implemented using jQuery UI Dialogs
2. The only way to do that is to extend the API provided by drupal.dialog library.

@markcarver in #17

If this API needs to be extended to wrap more of jQuery UI, then so be it.

@markcarver in #19

There should be abstracted methods in core's Drupal.dialog API that allows for contrib to remove/extend as needed.

3. I attempted to extend solve this by extending the API in #23
4. @nod_(core Javascript maintainer) concurring with @droplet(other core Javascript maintainer) in #33, #35, #37

declared the solution in #23 and the idea of extending API was not acceptable.

5. As I documented #38 the drupal.dialog.ajax makes the same exact assumption in that dialogs will be JQuery UI Dialogs

In short it is not possible abstract it totally away from jQuery without extending the dialog API which is not an option.

@markcarver sorry about this but I don't see a way around this given those facts.

This module provides 2 Javascript files this only affects offcanvas.js which hopefully will be integrated into the core dialog system in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it

The other Javascript functionality in the module which will be left after #2784443 which is in outside_in.js does not depend on the dialog being jQuery UI.

I don't only focus on "custom" implementation or not. I'm looking to a more better solid pattern for everything, not single tailored-made for "Setting Tray" or Bootstrap Theme.

Now, because of one experimental module, we adding THREE public API. Tomorrow, another module request to add another THREE. jQuery UI has over 30 APIs for Dialog ( http://api.jqueryui.com/dialog/ ). The diff modules required diff APIs. We should keep the CORE libs lightweight as possible as we can until it really hit the limit or it's a very common pattern everywhere!

(Note: If you looked into SettingTray, you required more than THREE to get rid of jQuery UI)

I gave some patterns example in previous comments. SettingTray should have its own solid pattern first to remove the debt.

There're few way to sort of the problem: All in module-level

#1. Using the current event system in SettingTray.
You only need to override the SettingTray event. Quick example:
dialogContentResize.off-canvas
resize.off-canvas

** Refactoring the SettingTray first if we can't hack it cleanly.

#2. Solid pattern in SettingTray

// Based on https://www.drupal.org/node/2786459#comment-11551215
// Overrided into Bootstrap
Offcanvas.open = function () {
    $('#myModal').modal('show')
};

Offcanvas.close = function () {
    $('#myModal').modal('hide')
};

Offcanvas.handleDialogResize = function() {}

$('#myModal').on('shown.bs.modal', function (e) {
    BootstrapTailorMade.handleDialogResize()
});

Please look at these code:

    function openDialog(settings) {
      settings = $.extend({}, drupalSettings.dialog, options, settings);

      $(window).trigger('dialog:beforecreate', [dialog, $element, settings]);
      $element.dialog(settings);
      dialog.open = true;
      $(window).trigger('dialog:aftercreate', [dialog, $element, settings]);
    }

You able to obtain most of the data from dialog:beforecreate and dialog:aftercreate

Technically as I said before, we should refactor the Drupal.dialog also. We opened the hole to access jQuery UI API. It was wrong at the beginning. It just more debt over the time. (We also made a mistake to write some bad pattern in the CORE, so everyone is copy and paste own style)

IMO, to override Drupal.dialog.APIs also wrong. You never know what other modules doing on the dialog:beforecreate and dialog:aftercreate events. (@see #27)

Ok so I chatted with @droplet because I wasn't really understanding what he was getting at #41

I think the gist of his point in our chatted, sorry for getting this wrong:

• Yes he still is not in favor of expanding Drupal.dialog API for the reason
  

  Now, because of one experimental module, we adding THREE public API. Tomorrow, another module request to add another THREE. jQuery UI has over 30 APIs for Dialog ( http://api.jqueryui.com/dialog/ ). The diff modules required diff APIs. We should keep the CORE libs lightweight as possible as we can until it really hit the limit or it's a very common pattern everywhere!

• But let's make Drupal.offCanvas something that is overridable.
• The Settings Tray's version of Drupal.offCanvas will still assume jQuery UI. off-canvas.js is strictly positioning and this hard to across different dialog implementations
• Bootstrap or other theme that don't use JQuery will have override certain function in Drupal.offCanvas to make work with their implementations

So here is the start to the patch. It should pass all current test.

I have testing out what Bootstrap might have to do in this scenario here https://github.com/tedbow/bootstrap-theme/tree/off_canvas_override_2

This probably needs a lot work but hopefully gets across the idea.

Not doing an interdiff because I started droplets patch he sent which was totally new idea

So here is the start to the patch. It should pass all current test.

How young and naive I was.. 🙃

Ok actually ran the test locally this time.

there is bunch more clean to do but hopefully get the tests passing.

Ok I realized Drupal.offCanvas.render was not actually setting reset functions that could be overridden by for example overriding

Drupal.offCanvas.handleDialogResize = function () {
    // Do resize stuff
  };

This should fix that.

1. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -31,27 +18,28 @@
   +        my: offCanvasDialog.getEdge() + ' top',
   

   template strings

2. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -31,27 +18,28 @@
   +        at: offCanvasDialog.getEdge() + ' top' + (offsets.top !== 0 ? '+' + offsets.top : ''),
   

   template strings

3. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -31,27 +18,28 @@
   +        .dialog('option', adjustedOptions)
   

   Is this indentation correct?

4. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -85,17 +74,19 @@
   +    var mainCanvasPadding = $mainCanvasWrapper.css('padding-' + offCanvasDialog.getEdge());
   

   template strings

5. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -85,17 +74,19 @@
   +      $mainCanvasWrapper.css('padding-' + offCanvasDialog.getEdge(), width + 'px');
   

   template strings

6. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -85,17 +74,19 @@
   +      $container.attr('data-offset-' + offCanvasDialog.getEdge(), width);
   

   template strings

7. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -108,49 +99,88 @@
   +    isOffCanvas: function (dialog, $element, settings) {
   

   no unused function params

8. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -108,49 +99,88 @@
   +    open: function (event, dialog, $element, settings) {
   

   no unused function params, use something like, (,,,settings)

9. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -108,49 +99,88 @@
   +    close: function (event, dialog, $element) {
   

   no unused function params

10. +++ b/core/modules/outside_in/js/off-canvas.es6.js
    @@ -108,49 +99,88 @@
    +    render: function (event, dialog, $element, settings) {
    

    no unused function params

11. +++ b/core/modules/outside_in/js/off-canvas.es6.js
    @@ -108,49 +99,88 @@
    +          .on('dialogresize.off-canvas', eventData, debounce(this.bodyPadding, 100))
    

    looks like the wrong indentation

12. +++ b/core/modules/outside_in/js/off-canvas.es6.js
    @@ -108,49 +99,88 @@
    +          .on('resize.off-canvas scroll.off-canvas', eventData, debounce(this.resetSize, 100))
    

    looks like the wrong indentation

13. +++ b/core/modules/outside_in/js/off-canvas.es6.js
    @@ -108,49 +99,88 @@
    +    handleDialogResize: handleDialogResize,
    

    any reason these functions can't just sit on this object?

@drpal thanks for the review.

Fixed template strings and some other es6 changes.
Still needs work.

Ok here is some more code cleanup.

so re #46
1-7. fixed
8-10. So my original thought on leaving the parameters in is that then if you override the function then you know what is sent to. Even though we are not using say event another implementation of render() may. Using (,,,settings) gives and eslint error "Formal parameter name expected"

We can change the parameter argument order so the unused ones are last but like the idea of having the calls to render() and open() having the same parameters in the same order. And also that pass on all the arguments received from the 'dialog:*' events here:

$(window).once('off-canvas').on({
        'dialog:beforecreate': (event, dialog, $element, settings) => {
          if (Drupal.offCanvas.isOffCanvas($element, dialog, settings)) {
            Drupal.offCanvas.open(event, dialog, $element, settings);
          }
        },
        'dialog:aftercreate': (event, dialog, $element, settings) => {
          if (Drupal.offCanvas.isOffCanvas($element, dialog, settings)) {
            Drupal.offCanvas.render(event, dialog, $element, settings);
          }
        },
        'dialog:beforeclose': (event, dialog, $element) => {
          if (Drupal.offCanvas.isOffCanvas($element, dialog)) {
            Drupal.offCanvas.close(event, dialog, $element);
          }
        },
      });

11-12 fixed
13. No. That is the code @droplet had originally suggested but he had just given me something quick so he might not have updated that. Left for now.

Cleanup only.

I agree to keep the args and we need a way to handle it better. Either change the ESLint, ignore it or better documentation. For example:

https://github.com/airbnb/javascript/issues/726#issuecomment-183026561

and I'm looking to change `Drupal.offCanvas.open` to `Drupal.offCanvas.beforeCreate` and `close` to `beforeClose`. that's matching the event name.

I also think `Drupal.offCanvas.render` can split into 2 methods.

Only keep below oneline in .render()

$('.ui-dialog-off-canvas, .ui-dialog-off-canvas .ui-dialog-titlebar').toggleClass('ui-dialog-empty-title', !settings.title);

and other parts will be `afterCreate`. It seems cleaner code?

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -117,7 +30,7 @@
+    isOffCanvas($element, dialog) {

Is this pattern for method definitions something we want to enforce? It's probably worth creating an adjustment to our eslint overrides to make sure this happens.

#19:

The only thing this module shouldn't be doing is calling $.fn.dialog directly, in any of its JS.

+1!

Let's get this going again, it looks like this is a blocker for #2784443 — see #2784443-111: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it.

Re #19

The only thing this module shouldn't be doing is calling $.fn.dialog directly, in any of its JS.

See sum up of why this is still happening in this patch in #39 especially points 3,4,5

#53 was based on a quick skim. Now really trying to get this issue back on track.

So what @markcarver said at #3 is not true: This is likely very doable if using the proper APIs — the "proper APIs" don't exist yet. Also note that the Bootstrap theme's override of Drupal.dialog also only works for modal dialogs, it doesn't work for non-modal dialogs: #2831237: Bootstrap modal does not work well with jQuery UI dialog — so Bootstrap's override is broken.

@nod_ stressed in #7 that he agrees with @markarver. But doing so would require changing Drupal.dialog, which we are not allowed to do. However, at #2786459-65: "Offcanvas" tray should be using the existing dialog system, @droplet proposed to create a new API, Drupal.Offcanvas, mirrored after Drupal.dialog. So that at least it resembles the other API, but at the same time it doesn't require Drupal.dialog to be changed. @nod_ agreed with that at #2786459-66: "Offcanvas" tray should be using the existing dialog system.

But at #17 and #19, @markcarver expresses his preference for this to be an expansion made as a change to the Drupal.dialog API, which violates @nod_'s requirements.

Effectively, @markcarver is arguing that Settings Tray's JS should only call Drupal.dialog, and not jQuery.fn.dialog. But at #2786459-65: "Offcanvas" tray should be using the existing dialog system, @droplet argued that there should be a new Drupal.Offcanvas API that would still call jQuery.fn.dialog()!

@tedbow tried to implement what @markcarver wanted in #23, to the satisfaction of @markcarver in #25, but @droplet shot it down in #27, referring again to @nod_'s comment in #7 and thus to @droplet's own comment at #2786459-65: "Offcanvas" tray should be using the existing dialog system. So… @droplet+@nod_ are disagreeing with @markcarver. @tedbow reached a similar conclusion in #29.
At #33, @nod_ again confirmed this.
And at #34, @markcarver very strongly expresses his disagreement with @nod+@droplet. At #35, @nod_ elaborates on his stance. #36 and #37 are more disagreeing.

In #38, @tedbow explained the problems. In #39, he explained how this cannot continue.

In #40, @markcarver disagreed firmly.

In #41, @droplet re-iterated what he said in #2786459-65: "Offcanvas" tray should be using the existing dialog system, explaining he'd like to see this refactored to have a Drupal.OffCanvas API mirrored after Drupal.Dialog. Interestingly, he also said:

Technically as I said before, we should refactor the Drupal.dialog also. We opened the hole to access jQuery UI API. It was wrong at the beginning. It just more debt over the time.

Which means a BC break for Drupal.Dialog due to a wrong design decision, which is how/why Settings Tray did things these way in the first place!

Then in #42 through #48, @tedbow tried to implement what @droplet was asking for. In #49 + #50, @droplet is basically +1'ing the current patch, and asking for some more changes.

Therefore I think we're actually almost at the finish line? Let's address the feedback in #50, and get this to RTBC? It does look like this would significantly improve the JS quality!

Issue title updated to reflect the direction of this issue since #42.

@droplet + @nod_, can you create an issue to at minimum document, and preferably fix the Drupal.Dialog API to fix this problem:

Technically as I said before, we should refactor the Drupal.dialog also. We opened the hole to access jQuery UI API. It was wrong at the beginning. It just more debt over the time.

That's the entire reason we're now having to deal with this!

- Reroll
- Rename Drupal.offCanvas.open/close from feedback in #50
- Restructure Drupal.offCanvas.render
- Additional documentation.

@droplet, did #58 address your concerns? This is blocked on your review.

Yes #58 is fine and the blocker isn't on me. I'm waiting for others feedback.

EDITED: I'm +1 to go.

@droplet Thanks for clarifying!

The code looks massively improved, and I wanted to RTBC, but in manual testing I found that this broke Settings Tray's functionality: you can enable/disable edit mode, but clicking on blocks no longer opens up the Settings Tray.

Actually, turns out I had a local change that accidentally broke Settings Tray. Turns out it works perfectly!

Issue summary needs updating, lots of TBD and the summary doesn't seem to reflect the latest patches. The issue retitle (and @Wim Leers' recap in #56) help but I would like to see a better summary of what problem(s) we are solving here.

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -9,156 +9,226 @@
+     */
+    minDisplaceWidth: 768,
+    /**
+     * Wrapper used to position off-canvas dialog.
+     * @type {jQuery}
+     */
+    $mainCanvasWrapper: $('[data-off-canvas-main-canvas]'),
+    /**

Minor but can we please add blank lines in between the commas and docblocks inside this object? This file is now rather dense to parse visually.

adding blank lines...

Probably we need to rewrite all stable CORE JS into this pattern :P

@Adita thanks for the spacing fixes. @droplet and @drpal thanks updating the summary. Looks good!

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -9,156 +9,239 @@
   /**
...
+   * Attaches off-canvas dialog behaviors.
    *
...
+   * @type {Drupal~behavior}
    *
...
+   * @prop {Drupal~behaviorAttach} attach
+   *   Attaches event listeners for off-canvas dialogs.
    */

These docs should still be on the behavior, and the new JS module should have its own docblock - with a @namespace if I'm not mistaken.

There are other docs issues here that can be addressed in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it because this issue is not yet moving the code into core, so it's not subject to all the core gates. But to me, this one is pretty important.

Other than that, this is looking good. I reviewed all the code changes one by one and the refactor looks solid.

PS. Thanks for the issue summary update, much better!

How about commit this patch and fix docs in #2899724: Fixes for settings tray CSS and JavaScript minor issues?

1. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -9,156 +9,239 @@
   +      settings.dialogClass += ' ui-dialog-off-canvas';
   

   This can probably be a template string, but only a minor nit.

2. +++ b/core/modules/outside_in/js/off-canvas.es6.js
   @@ -9,156 +9,239 @@
   +      // Applies initial height to dialog based on window height.
   

   This should be in the multi-line comment format.

#70 fixed comments

#72.1 leaving b/c will be removed in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
#72.2 fixed

The compiled/transpiled JavaScript was slightly off, the git hooks were complaining so I fixed on commit since it was just whitespace by building the JS (attaching the interdiff).

Committed and pushed 48339450ee to 8.5.x and 7feedc23c9 to 8.4.x. Thanks!

@droplet I held back on asking for a lot of docs fixes here but I thought that one was too important because it results in docs that are wrong (behavior docs on the new object) and missing (behavior docs).

When loading a sufficiently long form in the tray, it cannot scroll anymore.
Also, something is wrong with the placement of the "Save" button.

This picture is of editing the Tools menu, and trying to scroll down to the save, which is impossible now.

For reference, this is with that commit reverted.

I think this should be reverted until it can be tested with longer forms.

Clear caches? Needs a step to reproduce the problem.

EDIT:

It seems like a problem on macbook (no scrollbar), `git checkout 2174766ce5` before #2826722: Add a 'fence' around settings tray with aggressive CSS reset. still have the same issue.

Reverted both commits for now.

Fixed #78

+++ b/core/modules/outside_in/js/off-canvas.js
@@ -6,106 +6,123 @@
+            Drupal.offCanvas.beforeCreate(settings);
...
+            Drupal.offCanvas.render(settings);
...
+            Drupal.offCanvas.beforeClose(event, dialog, $element);

Just notice the patch after #49 different than my expectation. While it's reverted, I want to discuss more on it.

I actually expected we passing the same params into above functions.

I add back the params because these methods are public APIs and we need extra info for advanced usage (while you override it)

- dialog
It contains the instance (Drupal.dialog) methods

- $element
instance element

- setting
instance settings

@droplet thanks the additional parameters you added back look correct. Look like these were lost between #49 and #58

I can't reproduced the problem in #78 though. I tried applying #73 and applying #84 I can always save the "Tools" block no matter what the size the window is having to scroll.

I was able to reproduce #78 and the latest patch fixes it. The fix also makes sense because it we get the offsets wrong it will mess up the scrolloffset which will not allow scrolling to the bottom of the page.
Before we were effectively doing this.

$offsets.each((i, e) => {
    offset += $(i).outerHeight();
});

so $(i) would not return any results and we would get the correct offsets for the elements.

Setting back to RTBC

@droplet's changes in #84 make it so that you can override the API and you will get the other arguments that are relevant to the dialog events. Even though we don't need to those arguments it makes sense to send them because we can't be sure what the overridden implementations will need and we should give them as much context as possible.

@tim.plunkett thanks for reporting the scroll problem.

@droplet thanks for the fix and the other changes.

Thanks everyone, looks like this could have used more manual testing. I didn't manually test it myself. I should be able to do that testing today and hopefully commit the revised patch :)

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -9,101 +9,220 @@
+    beforeCreate(dialog, $element, settings) {

JSDoc is incorrect here. Additionally this triggers the no-unused-vars rule from eslint.

This is a slightly different approach,

Drupal.offCanvas.beforeCreate({ dialog, $element, settings }); -> beforeCreate({ settings }) { //... }

Changes in #88 looks and gets rid of the no-unused-vars errors.

Confirmed that the latest patch works with Layout Builder

Okay, let's try this again :) gave this a whirl myself and looks good. Do the changes from #88 mean some JSDoc updates are needed now that the parameters are being passed in differently? If so, that can happen in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it.

Committed and pushed aa76f9674f to 8.5.x and 3704c99104 to 8.4.x. Thanks!

Attaching an interdiff that shows all the changes since the revert.

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -134,8 +134,8 @@
-      $offsets.each(($offset) => {
-        offset += $($offset).outerHeight();
+      $offsets.each((i, e) => {
+        offset += $(e).outerHeight();

This was the cause of the bug I hit, it seems. Checking for the height on the wrong parameter.

Thanks for the revert, and the quick turnaround!

Seems like this is causing a HEAD fail now :( reverting from both branches for now.

Requeued the environments that failed in HEAD. Thanks @Cottser.

I believe this is related to #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails. Here are 2 patches have #88 and test OutsideInBlockFormTest 30 times.

1 patch has the testing fix from #2901626

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -9,101 +9,220 @@
+    beforeClose() {

`beforeClose({})` to mute the editors lint error.

So 2830882-99-withOUT-testing-fix.patch passed which means the test OutsideInBlockFormTest was run 30x and no failure with the exact same changes in #88 and using php 7 and mysql 5.5 which caused the head failure here https://www.drupal.org/pift-ci-job/739306

This makes me think it was random. I would still encourages to get #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails fixed because as the test results should there without the fix there is much more of chance of the random fail.

UPDATE: queue up a bunch of tests see if the random fail will happen.

I think this issue should be committed because there 2 things going in Settings Tray module.

1. the off-canvas dialog. This relatively simple JS and is much easier to test. It has it's own JS test OffCanvasTest which has never to my knowledge had a random test failure.
2. The Settings Tray module Edit mode interaction which is much more complicated JS and much harder to test. It also has a test separated out OutsideInBlockFormTest.

The test involves other parts of core that 2 my knowledge have never been tested thoroughly with JS in core.
Those parts are:

1. The dialog system: no tests at all before #2844261: Allow dialog links to specify a renderer in addition to a dialog type and not thoroughly tested
2. Contextual links: still not tested outside of this module where links are actually clicked
3. Toolbar: simple js tests
4. Quick Edit: still not tested outside this module
5. Ajax Form submits: I think this has JS tests though doubt in combination with dialogs
6. Ajax redirects: not sure about JS tests

Given all that it very rarely fails and when it does it seems random somehow connected to slow test bots. It is probably the only test gets the random failure because it is the most complicated JS test.

We also have likely fix: #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails

I don't think 1 should be hold 2. Especially since Settings Tray is still not beta(though hopefully soon). In #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it we will only be moving OffCanvasTest into stable core. Again this test has never randomly failed.

This issue could be committed with a skip test call in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks() and @todo to remove it in #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails. Now other test method in this class fails randomly

Uploading a patch for this.

doesn't seems like the test broken by this patch. we have no bugs fix here. Even the code refactoring won't make the script faster. (Yeah, that might affect the memory usage..)

+1 to go.

Like @droplet mentioned in #103 it seems very unlikely this patch broke the test.
Either of these patches applies and could be committed

#88 which was patch that got committed before. #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails was committed so even less likely \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks would fail

Or #102 which is #88 plus it skips OutsideInBlockFormTest::testBlocks(). If that got committed we would want to then create follow up. #99 proves that even with animation css fix it would randomly fail. But I again this patch does cause the test failure it is just that Testbots have been slow today.

I am going to RTBC since #88 has already been committed. and was reverted for a random test failure not caused by this patch.

Third time is the charm?

I didn't review this; just un-reverted it essentially. Edit: Without the skip-o-test. We will find out if this is a mistake. :P

Ah, BAH, look at #99. The test runs rolled up with #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails still had some fails.

Changing to new settings_tray.module component. @drpal thanks for script help! :)