Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#102 | interdiff-88-102.txt | 936 bytes | tedbow |
#102 | 2830882-102-skip-test.patch | 22.26 KB | tedbow |
#99 | 2830882-99-withOUT-testing-fix.patch | 21.9 KB | tedbow |
#99 | interdiff-88-99.txt | 1.71 KB | tedbow |
#99 | 2830882-99-with-testing-fix.patch | 23.05 KB | tedbow |
Comments
Comment #2
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedI'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:
Comment #3
markhalliwellI'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).
Comment #4
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedOk then .. Just Independent and the proper APIs.
Back to your track @markcarver .......
I think it's the right one.
Rewarded :)
Comment #5
tedbow@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.
Comment #6
markhalliwellFTR, 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.
Comment #7
nod_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.
Comment #8
tedbow@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.
Comment #9
tkoleary CreditAttribution: tkoleary at Acquia commentedThis 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.
Comment #10
markhalliwellThat 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).
Comment #11
tkoleary CreditAttribution: tkoleary at Acquia commentedYes, 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.
Comment #12
tedbowThere 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.
Comment #14
GrandmaGlassesRopeMan#7
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.
Comment #15
tedbowThis 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
@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
incore/modules/quickedit/js/views/AppView.js
usage ofcloseOnEscape
.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
ajax.js
dialog.js
dialog.ajax.js
In other words it completely replaces the JS for dialogs. Currently in bootstrap on "modal"
dialogType
works not "dialog". Alsodata-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.
Comment #16
tedbowComment #17
markhalliwellNo it's not. It's directly calling
$().dialog
rather than going through Drupal's providedDrupal.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).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.
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.
Comment #18
GrandmaGlassesRopeManI 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.
Comment #19
markhalliwellThe 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.
Comment #20
samuel.mortensonWhere is it formalized that Drupal's dialog can work without jQuery.ui? The libraries definition requires it:
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.
Comment #21
markhalliwellhttps://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.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).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.
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.
Comment #22
samuel.mortenson@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!
Comment #23
tedbow@markcarver
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.
While hope it doesn't get removed I do agree with you that policy is there for a good reason.
The deadline is simply the deadline for experimental modules to get into core.
From https://www.drupal.org/core/experimental#requirements
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
Ok sounds like a good next step.
re @markcarver
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:
$element.dialog('widget');
withdialog.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.$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.I have been testing against a clone bootstrap: https://github.com/tedbow/bootstrap-theme/tree/8.3-off-canvas
It contains changes
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?
Comment #24
tedbowForgot 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.
Comment #25
markhalliwell@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.
---
Yes! In Bootstrap, we'd likely just return $element since it doesn't have the concept of a "widget" container like in jQuery UI.
Yes! Bootstrap's modal options are limited, specific and [officially] immutable once the modal has been initialized. I'll discuss more below.
Yes! Bootstrap's modal has its own resize/positioning handler in newer versions of the plugin.
---
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).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.
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
Comment #26
markhalliwellP.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.
Comment #27
droplet CreditAttribution: droplet commented@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
andDrupal.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:
Problem:
Obviously, our API design doesn't allow to override it directly.
Comment #28
GrandmaGlassesRopeManThis should probably be
getContainer
.This should probably be
setOptions
.Documentation.
Documentation.
Comment #29
tedbow@drpal thanks for the review.
This patch fixes those issues.
@markcarver #25
I would say let's leave them in the patch for now till we get consensus on this way.
re @nod_ in #7
@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
also #17
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.
Comment #31
tedbowFor to update call to
dialog.container()
todialog.getContainer()
in dialog.js.sorry for not running the tests locally
Comment #33
nod_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?
Comment #34
markhalliwellThat's absurd.
<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).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.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.Comment #35
nod_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.
Comment #36
markhalliwellUm, yeah. That's what I just said.... o_O
Um. No it isn't. It doesn't handle any of the current dialog stacks (like the spec defines).
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.
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.
Comment #37
nod_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
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.
Comment #38
tedbowI 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:
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 tobuttonPrimaryClass
misc/dialog.js
would cause a fatal error when using Settings Tray because it did not have the new methodsgetContainer
andsetOptions
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
$dialog.dialog('widget')
,$dialog.dialog('option'
and relying on jQuery Dialog specific classes .ui-dialog-*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
Comment #39
tedbowAs the maintainer of this module I am closing this as works as designed.
Here is why:
@markcarver in #17
@markcarver in #19
declared the solution in #23 and the idea of extending API was not acceptable.
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.
Comment #40
markhalliwell@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.
Comment #41
droplet CreditAttribution: droplet commentedI 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
Please look at these code:
You able to obtain most of the data from
dialog:beforecreate
anddialog: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
anddialog:aftercreate
events. (@see #27)Comment #42
tedbowOk 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:
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
Comment #44
tedbowHow 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.
Comment #45
tedbowOk I realized Drupal.offCanvas.render was not actually setting reset functions that could be overridden by for example overriding
This should fix that.
Comment #46
GrandmaGlassesRopeMantemplate strings
template strings
Is this indentation correct?
template strings
template strings
template strings
no unused function params
no unused function params, use something like,
(,,,settings)
no unused function params
no unused function params
looks like the wrong indentation
looks like the wrong indentation
any reason these functions can't just sit on this object?
Comment #47
tedbow@drpal thanks for the review.
Fixed template strings and some other es6 changes.
Still needs work.
Comment #48
tedbowOk 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:
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.
Comment #49
droplet CreditAttribution: droplet commentedCleanup 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
Comment #50
droplet CreditAttribution: droplet commentedand 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()
and other parts will be `afterCreate`. It seems cleaner code?
Comment #51
GrandmaGlassesRopeManIs 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.
Comment #53
Wim Leers#19:
+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.
Comment #54
Wim LeersComment #55
tedbowRe #19
See sum up of why this is still happening in this patch in #39 especially points 3,4,5
Comment #56
Wim Leers#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: — 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 afterDrupal.dialog
. So that at least it resembles the other API, but at the same time it doesn't requireDrupal.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 notjQuery.fn.dialog
. But at #2786459-65: "Offcanvas" tray should be using the existing dialog system, @droplet argued that there should be a newDrupal.Offcanvas
API that would still calljQuery.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 afterDrupal.Dialog
. Interestingly, he also said: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.
Comment #57
Wim Leers@droplet + @nod_, can you create an issue to at minimum document, and preferably fix the
Drupal.Dialog
API to fix this problem:That's the entire reason we're now having to deal with this!
Comment #58
GrandmaGlassesRopeMan- Reroll
- Rename
Drupal.offCanvas.open/close
from feedback in #50- Restructure
Drupal.offCanvas.render
- Additional documentation.
Comment #59
GrandmaGlassesRopeManComment #60
Wim Leers@droplet, did #58 address your concerns? This is blocked on your review.
Comment #61
droplet CreditAttribution: droplet commentedYes #58 is fine and the blocker isn't on me. I'm waiting for others feedback.
EDITED: I'm +1 to go.
Comment #62
Wim Leers@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.
Comment #63
Wim LeersActually, turns out I had a local change that accidentally broke Settings Tray. Turns out it works perfectly!
Comment #64
star-szrIssue 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.
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.
Comment #65
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedadding blank lines...
Comment #66
droplet CreditAttribution: droplet commentedProbably we need to rewrite all stable CORE JS into this pattern :P
Comment #67
GrandmaGlassesRopeManComment #68
tedbow@Adita thanks for the spacing fixes. @droplet and @drpal thanks updating the summary. Looks good!
Comment #69
Wim LeersComment #70
star-szrThese 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!
Comment #71
droplet CreditAttribution: droplet commentedHow about commit this patch and fix docs in #2899724: Fixes for settings tray CSS and JavaScript minor issues?
Comment #72
GrandmaGlassesRopeManThis can probably be a template string, but only a minor nit.
This should be in the multi-line comment format.
Comment #73
tedbow#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
Comment #77
star-szrThe 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).
Comment #78
tim.plunkettWhen 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.
Comment #79
droplet CreditAttribution: droplet commentedClear 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.Comment #81
catchReverted both commits for now.
Comment #83
droplet CreditAttribution: droplet commentedFixed #78
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.
Comment #84
droplet CreditAttribution: droplet commentedI 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
Comment #85
tedbow@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.
so
$(i)
would not return any results and we would get the correct offsets for the elements.Comment #86
tedbowSetting 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.
Comment #87
star-szrThanks 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 :)
Comment #88
GrandmaGlassesRopeManJSDoc 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 }) { //... }
Comment #89
tedbowChanges in #88 looks and gets rid of the
no-unused-vars
errors.Comment #90
tim.plunkettConfirmed that the latest patch works with Layout Builder
Comment #93
star-szrOkay, 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.
Comment #94
tim.plunkettThis 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!
Comment #97
star-szrSeems like this is causing a HEAD fail now :( reverting from both branches for now.
Comment #98
xjmRequeued the environments that failed in HEAD. Thanks @Cottser.
Comment #99
tedbowI 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
Comment #100
droplet CreditAttribution: droplet commented`beforeClose({})` to mute the editors lint error.
Comment #101
tedbowSo 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.
Comment #102
tedbowI 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:
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.
Comment #103
droplet CreditAttribution: droplet commenteddoesn'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.
Comment #104
tedbowLike @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.
Comment #108
xjmThird 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
Comment #109
xjmAh, 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.
Comment #111
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)