Problem/Motivation

Currently all functions within off-canvas.es6.js are private scoped. Some themes, like Bootstrap, already have their own modal JavaScript which do not rely on jQuery UI. Due to the private scope design custom projects do not have the ability to override its default behavior.

Proposed resolution

Rewrite off-canvas.es6.js to follow a "module pattern", exposing functions publicly, allowing other JavaScript libraries to modify or replace functionality.

API changes

- Exposed previously private methods to public. @see patch Drupal.offCanvas

CommentFileSizeAuthor
#102 interdiff-88-102.txt936 bytestedbow
#102 2830882-102-skip-test.patch22.26 KBtedbow
#99 2830882-99-withOUT-testing-fix.patch21.9 KBtedbow
#99 interdiff-88-99.txt1.71 KBtedbow
#99 2830882-99-with-testing-fix.patch23.05 KBtedbow
#93 interdiff-74-88.txt5.79 KBCottser
#88 2830882-88.patch21.35 KBdrpal
#88 interdiff-2830882-84-88.txt5.71 KBdrpal
#84 2830882-84.patch21.18 KBdroplet
#84 interdiff.patch4.41 KBdroplet
#83 interdiff.patch1.33 KBdroplet
#83 2830882-82.patch21.02 KBdroplet
#78 not_broken_settings_tray.png45.33 KBtim.plunkett
#78 broken_settings_tray.png46.35 KBtim.plunkett
#77 interdiff.txt446 bytesCottser
#73 2830882-73.patch21.06 KBtedbow
#73 interdiff-65-73.txt1.46 KBtedbow
#23 2830882-23.patch5.36 KBtedbow
#29 2830882-29-off-canvas-abstract.patch5.98 KBtedbow
#29 interdiff-2830882-25-29.txt2.87 KBtedbow
#31 interdiff-2830882-29-31.txt530 bytestedbow
#31 2830882-32.patch5.98 KBtedbow
#42 2830882-42.patch16.12 KBtedbow
#44 interdiff-2830882-42-44.txt1.14 KBtedbow
#44 2830882-44.patch16.1 KBtedbow
#45 interdiff-2830882-44-45.txt2.73 KBtedbow
#45 2830882-45.patch16.14 KBtedbow
#47 interdiff-45-47.txt14.22 KBtedbow
#47 2830882-47.patch17.04 KBtedbow
#48 interdiff-47-48.txt6.94 KBtedbow
#48 2830882-48.patch17.6 KBtedbow
#49 interdiff.patch14.24 KBdroplet
#49 abstract_off_cavs_js-2830882-49.patch19.8 KBdroplet
#58 2830882-58.patch21.02 KBdrpal
#65 interdiff.txt2.89 KBAdita
#65 2830882-65.patch21.03 KBAdita
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

markcarver created an issue. See original summary.

RajabNatshah’s picture

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:

markcarver’s picture

Issue summary: View changes

I'd like to reiterate in this issue that we're not attempting to change what has already been implemented with the Settings Tray module.

Rather just fixing the existing implementation so it works with contrib rather than assuming contrib uses/loads jQuery UI.

This is likely very doable if using the proper APIs, maybe a little bit of additional custom/3rd party code will be needed, but nothing like what @RajabNatshah suggests in #2 (which isn't what this issue is about).

RajabNatshah’s picture

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

Rewarded :)

tedbow’s picture

@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 8.3.x does not work with core dialog library(non modal)

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.

markcarver’s picture

FTR, I merely created this issue to get feedback from core [component] maintainer(s) on whether this is a valid issue. I will gladly close this issue as a dup of the Bootstrap one if it turns out to be something that needs to be fixed on our end. I just don't have time (at the moment) to really dig into the nuances of core/contrib JS relations.

It should be noted that there are indeed existing overrides for:
http://cgit.drupalcode.org/bootstrap/tree/js/misc/ajax.js
http://cgit.drupalcode.org/bootstrap/tree/js/misc/dialog.js
http://cgit.drupalcode.org/bootstrap/tree/js/misc/dialog.ajax.js

So I'm a little unsure why it would work for "modals", but not "dialogs" types.

nod_’s picture

Category: Bug report » Task

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.

tedbow’s picture

@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.

tkoleary’s picture

Status: Active » Postponed

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.

markcarver’s picture

Status: Postponed » Active

That is super scope creep and not really the problem for this particular issue.

Yes, while that's an ideal goal, it's completely unrealistic given that core would have to introduce a lot of it's own APIs to compensate for the lack of jQuery UI.

Regardless, the only problem here is that the outside_in module is experimental.

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).

tkoleary’s picture

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.

tedbow’s picture

Title: Abstract outside_in (Settings Tray) JS to be jQuery UI independent » Abstract outside_in.js (Settings Tray) JS to be jQuery UI independent

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drpal’s picture

Status: Active » Postponed
Issue tags: +DevDaysSeville

#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.

tedbow’s picture

Status: Postponed » Active

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.

tedbow’s picture

Status: Active » Closed (works as designed)
markcarver’s picture

Status: Closed (works as designed) » Active

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

No it's not. It's directly calling $().dialog rather than going through Drupal's provided Drupal.dialog API. 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).

In other words it completely replaces the JS for dialogs.

Bootstrap comes with it's own modal JS, we don't supply it. So, no, it doesn't completely "replace" anything. It simply overrides the minimum methods that Drupal provides via it's JS API used to create modals.

Also data-dialog-options that are not respected

That is because Bootstrap's modals do not have the concept of mutable "options". They are set upon initialization + data attributes. Setting arbitrary things like width/height or positions after the fact are totally a jQuery UI thing.

drpal’s picture

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.

markcarver’s picture

Title: Abstract outside_in.js (Settings Tray) JS to be jQuery UI independent » Abstract outside_in.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly

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.

This makes the assumption that jQuery UI will always be present, which isn't the case and thus, causes fatal errors when not present (i.e. Error: TypeError: $(…).dialog is not a function).

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

I've changed the title of this issue to properly reflect what this issue is about. It's not about making it "independent" from jQuery UI specifically, but rather using proper Drupal specified APIs for dialogs.

If you feel there should be separate issues created to extend Drupal.dialog, fine. I'm not stopping anyone from doing that.

It doesn't change the fact that the current implementation of doesn't work well in contrib, especially when an external framework is involved that supplies it's own modal component.

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).

There is a reason we have to code JS in a theme abstract way: so it works with all themes, not just core.

samuel.mortenson’s picture

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’s picture

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

https://www.drupal.org/node/2497313

You're making the assumption that Drupal.dialog is synonymous with jQuery UI.

That is not true.

It is a standalone Drupal specific implementation to mimic a HTML5 dialog element polyfill by simply wrapping jQuery UI (since that's what core has been bundling with since 7.x).

That's all.

There is absolutely nothing mandated in Drupal that says: "to use dialogs, you must use jQuery UI".

It's just the current "standard implementation".

In fact, contrib has already pioneered replacing core provided libraries/assets for years now in a little project called:
https://www.drupal.org/project/jquery_update

Newer versions of jQuery/jQuery UI have had newer APIs/features that didn't always work with what core originally provided. This problem is highly prevalent in JS and well known through out the community. It is why this experimental module's JS needs to be abstracted so that key aspects of it can be replaced as needed.

The concept of using a different library for dialogs/modals is no more different than that of using a different version of jQuery. Consider TypeError: $(...).on is not a function when jQuery rolled out its newer binding methods. While it was technically still the same library, it was fundamentally different in many regards.

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

Which is already mostly abstracted enough to where the relevant areas can easily be replaced/extended to utilize Bootstrap's $(...).modal instead of jQuery UI's $(...).dialog (which we have already done).

What is outside_in doing wrong that core dialog isn't already doing?

It's directly invoking $(...).dialog in localized functions that makes it impossible to easily override by contrib. Such as: http://cgit.drupalcode.org/drupal/tree/core/modules/outside_in/js/off-ca....

This means that contrib would have to override and accumulate the entire technical debt of the file provided by this core module (which essentially duplicates code/efforts).

Hardcoding calls to specific jQuery plugins inside local functions binds the entire file to one assumption/library/API.

As someone who is extremely well versed in "overriding what core provides", I can assure you that taking on core's technical debt in contrib is a PITA.

I see no documentation or code that would enable developers to override that dependency.

Again, aside from the fact that any contrib project can extend or override libraries?

Just because "core doesn't do it" doesn't mean it doesn't happen in contrib.

In fact, there is a ton of code in core that's primary intended purpose is to support contrib's extension of core.

JS/libraries is definitely one of those areas that occurs quite frequently. Especially when external front-end frameworks are involved.

---

As a side note:

I find it quite interesting that this is fourth Acquia individual commenting on this issue. It almost seems like y'all have some sort of personal stake and internal deadline for this or something o_O

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).

I should not have to "code to prove technical merit". The maintainers of this module should be able to take comments from any person who reports an issue with it and fix it without technical help from the reporter.

I do not maintain this module, not do I wish to.

Simply ignoring what people report in an effort to "push something through" is how quality of code starts to degrade and, in fact, why the experimental and core gate policies were created for in the first place.

---

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

I don't want to see this get swept under the rug anymore than anyone else.

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.

samuel.mortenson’s picture

@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!

tedbow’s picture

Status: Active » Needs review
FileSize
5.36 KB

@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 8.3.x does not work with core dialog library(non modal). 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?

tedbow’s picture

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.

markcarver’s picture

@tedbow, thank you for the patch in #23!

I think this will help everyone involved know what we're all trying to talk about now :D

And it does show exactly what I have been trying to say: extend Drupal.dialog API/implementation so other code (like outside_in) can abstract the relevant dialog bits and not tie it to a specific library (jQuery UI).

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

---

@samuel.mortenson, I appreciate your explanation. I was merely making an observation and, yes, starting to feel little ganged up on. I'm sure that wasn't the case, but that is why I mentioned it.

---

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...Added container() to Drupal.dialog

Yes! In Bootstrap, we'd likely just return $element since it doesn't have the concept of a "widget" container like in jQuery UI.

Replaced calls $element.dialog('option', $options) with dialog.options($options, newoptions) These options will be jQuery dialog options but this how the API is explicitly setup to work. ... Added options() to Drupal.dialog

Yes! Bootstrap's modal options are limited, specific and [officially] immutable once the modal has been initialized. I'll discuss more below.

Moved to handleDialogResize() to dialog.handleDialogResize(). This has JQuery UI specific logic for handling heights.

Yes! Bootstrap's modal has its own resize/positioning handler in newer versions of the plugin.

---

Fix I talked about here #2831237-5: Bootstrap 8.3.x does not work with core dialog library(non modal). That issue doesn't provide a patch I was pretty sure it wasn't the best solution

Setting show: true is a temporary fix, yes. However, you are right in the sense that there is likely more work to be done on this (which I will be doing in that issue of the base theme).

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.

Correct. There are several methods in Bootstrap's modal plugin that handle repositioning/adjustments of a modal. Any specific solution will need to be made in the project that is overriding this.

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.

Correct. As I mentioned above, there is no concept of immutably setting options after a modal has been initialized.

That being said, it doesn't mean the base theme cannot provide a bridge (of sorts) to, as you suggest, translate these options and manually set them via the plugin's modal instance.

Doing this will be tricky and not all options will apply or even make sense to translate. Regardless, this is also something that should be focused in whatever code is overriding this.

The point being: at least all this can now happen with the above patch :D

markcarver’s picture

Assigned: Unassigned » markcarver
Status: Needs review » Needs work

P.S.

While I can see that this patch is on the right track, it will definitely need more work.

I will review this patch in more depth later this week when I have some time.

I will also try to expand on it as the need arises and flush out some of the documentation with the APIs as well.

droplet’s picture

@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.

drpal’s picture

  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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.98 KB
2.87 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 29: 2830882-29-off-canvas-abstract.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
530 bytes
5.98 KB

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

sorry for not running the tests locally

Status: Needs review » Needs work

The last submitted patch, 31: 2830882-32.patch, failed testing.

nod_’s picture

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?

markcarver’s picture

If it's not in the spec, it won't be on our API.

That's absurd.

  1. The "spec" referenced is indicating specifically about a <dialog> element and the attributes it supports; it mentions nothing about JavaScript specifically (albeit there are some arbitrary JS looking methods, but that isn't the point).
  2. The Drupal.dialog API is already in our namespace. It belongs to Drupal, not something that mimics a spec and is in reality just a rather hypothetical and incomplete "polyfill" implementation in the first place.
  3. Even if the "spec" were merely suggesting what it supports, it would only be a minimum or required specification. As with almost everything else in JS, there is absolutely nothing saying it cannot be extended.

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

No, because it's still manually invoking $(...).dialog (which isn't available) and causes a fatal error. Thus, the entire file would have to be overridden and the technical debt of the file would have to be taken on by contrib.

nod_’s picture

  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.

markcarver’s picture

Assigned: markcarver » Unassigned
Status: Needs work » Needs review

Dialog is a DOM API, not a JS API

Um, yeah. That's what I just said.... o_O

As the person who created the current Drupal.dialog, 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.

Um. No it isn't. It doesn't handle any of the current dialog stacks (like the spec defines).

Again it's a DOM API exposed to JS. What says it cannot be extended is me.

Because putting a completely custom "polyfill" in Drupal's namespace/API makes perfect sense.... better not touch it, it's "special"... sigh.

If it was meant to be just be a "polyfill", then it shouldn't have been in Drupal's namespace to begin with and should have been added as a standalone asset.

Why are you saying it cannot be extended? Using yourself as answer is not acceptable. The only "reasoning" that I can think of is that you want it to appear like a normal polyfill, but it's not; it's completely custom (you even say that it was you who created it). It doesn't look anything like google's polyfill or others out there.

It seems we agree that "abstracting" in this case means adding APIs, just not to Drupal.dialog directly.

No, "we" don't. That's what you and @drpal "agree" with.

I'm just calling Drupal.dialog what it is: a completely custom/Drupal specific API that only mimics said "spec/polyfill".

This is the place to do it and your argument about why we can't is absolutely ridiculous.

It's in Drupal's namespace; it's a Drupal API.

nod_’s picture

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.

tedbow’s picture

Title: Abstract outside_in.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly » Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly

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

tedbow’s picture

Status: Needs review » Closed (works as designed)

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.
  3. @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.

  4. I attempted to extend solve this by extending the API in #23
  5. @nod_(core Javascript maintainer) concurring with @droplet(other core Javascript maintainer) in #33, #35, #37
  6. declared the solution in #23 and the idea of extending API was not acceptable.

  7. 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.

markcarver’s picture

Status: Closed (works as designed) » Needs work

@nod_ and @droplet imply that it's "not acceptable" but have given no real reason as to why other than its original intention was to be a simple HTML5 polyfill.

If that were the case, it should have never been added to the Drupal namespace.

The fact remains, whether it was the intention or not, this is a completely custom implementation that can easily be extended.

What isn't acceptable is ignoring this issue in an effort to push through experimental code.

I have clearly outlined what the issue is and why it's important for this to be fixed for contrib that utilizes other modals/dialogs other than jQuery UI.

As it stands now, any contrib project that doesn't utilize jQuery UI would have to copy all JS from this module just to override the poorly constructed code.

This increases the technical debt of the contrib project and forces them to keep parity with any code changes and/or security related issues.

This is not an appropriate way to handle core code.

droplet’s picture

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)

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.12 KB

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

Status: Needs review » Needs work

The last submitted patch, 42: 2830882-42.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
16.1 KB

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.

tedbow’s picture

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.

drpal’s picture

Status: Needs review » Needs work
  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?

tedbow’s picture

Status: Needs work » Needs review
FileSize
14.22 KB
17.04 KB

@drpal thanks for the review.

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

tedbow’s picture

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.

droplet’s picture

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

droplet’s picture

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?

drpal’s picture

+++ 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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Abstract off-cavs.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly » Abstract off-canvas.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly

#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.

Wim Leers’s picture

Issue tags: +blocker
tedbow’s picture

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

Wim Leers’s picture

Title: Abstract off-canvas.js (Settings Tray) JS to utilize Drupal.dialog instead of jQuery UI directly » Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways
Status: Needs review » Needs work

#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 8.3.x does not work with core dialog library(non modal) — 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.

Wim Leers’s picture

@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!

drpal’s picture

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

drpal’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Assigned: Unassigned » droplet

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

droplet’s picture

Assigned: droplet » Unassigned

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

EDITED: I'm +1 to go.

Wim Leers’s picture

Status: Needs review » Needs work

@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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

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.

Adita’s picture

Status: Needs work » Needs review
FileSize
21.03 KB
2.89 KB

adding blank lines...

drpal’s picture

Issue summary: View changes
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

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

Wim Leers’s picture

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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!

droplet’s picture

Status: Needs work » Reviewed & tested by the community

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

drpal’s picture

  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.

tedbow’s picture

The last submitted patch, 65: 2830882-65.patch, failed testing. View results

  • Cottser committed 4833945 on 8.5.x
    Issue #2830882 by tedbow, droplet, Adita, drpal, markcarver, Wim Leers,...

  • Cottser committed 7feedc2 on 8.4.x
    Issue #2830882 by tedbow, droplet, Adita, drpal, markcarver, Wim Leers,...
Cottser’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed
FileSize
446 bytes

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).

tim.plunkett’s picture

Title: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways » NEEDS REVERT: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways
Issue summary: View changes
Status: Fixed » Reviewed & tested by the community
FileSize
46.35 KB
45.33 KB

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.

droplet’s picture

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.

  • catch committed 3c82d90 on 8.4.x
    Revert "Issue #2830882 by tedbow, droplet, Adita, drpal, markcarver, Wim...
catch’s picture

Title: NEEDS REVERT: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways » Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways
Status: Reviewed & tested by the community » Needs review

Reverted both commits for now.

  • catch committed 3f4174f on 8.5.x
    Revert "Issue #2830882 by tedbow, droplet, Adita, drpal, markcarver, Wim...
droplet’s picture

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.

droplet’s picture

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

tedbow’s picture

@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.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

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.

Cottser’s picture

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 :)

drpal’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.71 KB
21.35 KB
+++ 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 }) { //... }

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

Confirmed that the latest patch works with Layout Builder

  • Cottser committed aa76f96 on 8.5.x
    Issue #2830882 by tedbow, droplet, drpal, Adita, tim.plunkett,...

  • Cottser committed 3704c99 on 8.4.x
    Issue #2830882 by tedbow, droplet, drpal, Adita, tim.plunkett,...
Cottser’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
5.79 KB

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.

tim.plunkett’s picture

+++ 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!

  • Cottser committed 925f92b on 8.5.x
    Revert "Issue #2830882 by tedbow, droplet, drpal, Adita, tim.plunkett,...

  • Cottser committed bbb5143 on 8.4.x
    Revert "Issue #2830882 by tedbow, droplet, drpal, Adita, tim.plunkett,...
Cottser’s picture

Status: Fixed » Needs review

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

xjm’s picture

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

droplet’s picture

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

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

tedbow’s picture

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.

tedbow’s picture

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.

droplet’s picture

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.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

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.

  • xjm committed dc5ca92 on 8.5.x
    Issue #2830882 by tedbow, droplet, drpal, Adita, Cottser, tim.plunkett,...

  • xjm committed d9ee7ae on 8.4.x
    Issue #2830882 by tedbow, droplet, drpal, Adita, Cottser, tim.plunkett,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

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

xjm’s picture