My team is seeing that, on some developer's local machines, offcanvas.js's dialog event handlers are not being called, which causes Outside In modals to open as normal dialogs.

Unfortunately, I can't provide replication steps as this is an intermittent issue and only shows up for some users. That said, I have found that binding the dialog events after the document is ready fixes this issue consistently, which seems harmless to add. I've uploaded gifs of the problem before/after the patch, if it helps.

CommentFileSizeAuthor
#12 interdiff.txt566 bytesGrandmaGlassesRopeMan
#12 2831924-12.patch4.32 KBGrandmaGlassesRopeMan
#9 interdiff-7-9.txt574 bytestedbow
#9 2831924-9.patch4.27 KBtedbow
#7 2831924-7.patch4.25 KBtedbow
#5 outside-in-early-event-binding-fix-2831924-5.patch3.93 KBDevin Carlson
outside-in-after-binding.gif1.3 MBsamuel.mortenson
outside-in-before-event-binding.gif1.14 MBsamuel.mortenson
outside-in-early-event-binding-fix.patch3.57 KBsamuel.mortenson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

@samuel.mortenson thanks for the patch!

Looks good to me

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Reviewed & tested by the community » Needs review

Can't see from the patch but is this in Drupal.behaviors? That's already bound to document.ready.
If that's still not enough, we have a dedicated domready library to use

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

Can't see from the patch but is this in Drupal.behaviors?

No, I believe this was outside a behavior, which is likely why it was being bound early. I can either use jQuery.once to bind the event one time in a Drupal behavior, or use a different domready library. What is the dedicated library?

Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson commented
FileSize
outside-in-early-event-binding-fix-2831924-5.patch3.93 KB

A reroll of the original patch.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2831924-7.patch4.25 KB

Ok, this patch using attach behaviors.

@tim.plunkett I wasn't sure how to use the drupal specific dom ready library.

@samuel.mortenson if could test it on the local setup that was breaking that would be great.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented 
+++ b/core/modules/outside_in/js/offcanvas.js
@@ -100,48 +100,60 @@
+  Drupal.behaviors.offCanvasEvents = {
+    attach: function () {
+      $(window).on({

It looks like this patch is working for me, but could this event be bound multiple times if Drupal.attachBehaviors() is called multiple times? Should we use jQuery.once?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2831924-9.patch4.27 KB
interdiff-7-9.txt574 bytes

@samuel.mortenson yep. fixed

tkoleary’s picture

tkoleary CreditAttribution: tkoleary at Acquia commented
Issue tags: +JavaScript
droplet’s picture

droplet CreditAttribution: droplet commented 
+++ b/core/modules/outside_in/js/offcanvas.js
@@ -100,48 +100,60 @@
+      $(window).once('off-canvas').on({
...
+            $(document).off('.offcanvas');
+            $(window).off('.offcanvas');

Make it more consistency if we can.

If not, can we jot a comment there to avoid another same request in the future :)

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
2831924-12.patch4.32 KB
interdiff.txt566 bytes

Some explanation added for #11.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented 
+++ b/core/modules/outside_in/js/offcanvas.js
@@ -147,6 +147,7 @@
+            // Remove all *.offcanvas events
             $(document).off('.offcanvas');
             $(window).off('.offcanvas');
             $mainCanvasWrapper.css('padding-' + edge, 0);

I think this makes clear why we are using the "." here and not just "offcavnas"

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

Ok. RTBC. The randomness of this issue doesn't so it doesn't seem like we can add a test for it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Agreed that a test won't actually work in this context. Looks like @droplet's feedback was addressed.

Committed and pushed to 8.4.x and backported to 8.3.x, thanks!

  • webchick committed fc14e92 on 8.4.x 
    Issue #2831924 by tedbow, samuel.mortenson, drpal, Devin Carlson, tim....
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Issue tags: +DevDaysSeville

  • webchick committed 393da1c on 8.3.x 
    Issue #2831924 by tedbow, samuel.mortenson, drpal, Devin Carlson, tim....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Component: outside_in.module » settings_tray.module

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